[Bug]signed integer overflow in yajl_parse_integer (CWE-190)

[1820893135-pixel]

Summary

yajl_parse_integer() has a signed integer overflow when parsing overly long integers. The function accumulates digits with ret *= 10, but its MAX_VALUE_TO_MULTIPLY bounds check happens before ret *= 10. When the intermediate value is exactly equal to MAX_VALUE_TO_MULTIPLY (= LLONG_MAX / 10 rounded up = 922337203685477587), the next iteration performs ret *= 10 first, which already overflows LLONG_MAX (9223372036854775807); only then is the range check done. UBSan catches it (signed integer overflow: 922337203685477587 * 10 cannot be represented). Because YAJL handles this with errno = ERANGE + a clamped return, the overflowed value itself is "corrected" to LLONG_MAX/LLONG_MIN, so the practical impact is usually low severity — but it can escalate where UBSan halt-on-error is enabled or where callers use the integer value for subsequent array indexing/allocation sizes.

• Affected versions: YAJL 2.1.1 / HEAD 5e3a785 (2015-09-24)
• Severity: Low (non-fatal under UBSan recover mode; downstream misuse cannot be ruled out)
• CWE: CWE-190 (Integer Overflow or Wraparound)

Detail

Affected code

The bug lives in src/yajl_parser.c:

/* src/yajl_parser.c:36-57 */
#define MAX_VALUE_TO_MULTIPLY ((LLONG_MAX / 10) + (LLONG_MAX % 10))

 /* same semantics as strtol */
long long
yajl_parse_integer(const unsigned char *number, unsigned int length)
{
    long long ret  = 0;
    long sign = 1;
    const unsigned char *pos = number;
    if (*pos == '-') { pos++; sign = -1; }
    if (*pos == '+') { pos++; }

    while (pos < number + length) {
        if ( ret > MAX_VALUE_TO_MULTIPLY ) {     /* check 1: before the multiply */
            errno = ERANGE;
            return sign == 1 ? LLONG_MAX : LLONG_MIN;
        }
        ret *= 10;                               /* <-- overflow point */
        if (LLONG_MAX - ret < (*pos - '0')) {    /* check 2: after the multiply */
            errno = ERANGE;
            return sign == 1 ? LLONG_MAX : LLONG_MIN;
        }
        if (*pos < '0' || *pos > '9') {
            errno = ERANGE;
            return sign == 1 ? LLONG_MAX : LLONG_MIN;
        }
        ret += (*pos++ - '0');
    }

    return sign * ret;
}

Root cause: check 1 uses strict ret > MAX_VALUE_TO_MULTIPLY. When ret == MAX_VALUE_TO_MULTIPLY (i.e. 922337203685477587), check 1 does not intercept and ret *= 10 executes directly:

ret = 922337203685477587
ret * 10 = 9223372036854775870
LLONG_MAX = 9223372036854775807

9223372036854775870 > 9223372036854775807, so the signed multiply overflows, triggering UBSan's signed-integer-overflow. Check 2 later returns ERANGE because LLONG_MAX - ret is negative, but the overflow has already occurred — an abort under -fno-sanitize-recover=all, and undefined behavior per the C standard regardless.

Call chain:

yajl_parse_integer (yajl_parser.c:49)
  └─ yajl_do_parse (yajl_parser.c:287)
       └─ yajl_parse (yajl.c:130)
            └─ yajl_tree_parse / yajl_parse user entry point

POC

Triggering input (22 bytes, cfg 0x40 routes through the DOM path, 9223372036854775870 has 19 digits):

# Input bytes (hex): 40 5b 39 32 32 33 33 37 32 30 33 36 38 35 34 37 37 35 38 37 30 5d
# i.e. @[9223372036854775870]

# Repro command (UBSan halt-on-error)
UBSAN_OPTIONS=halt_on_error=1 ./yajl_fuzzer -runs=0 verify/repro_integer_overflow.bin

Pure API repro:

#include <yajl/yajl_parser.h>
int main(void) {
    const unsigned char *num = (const unsigned char *)"9223372036854775870";
    errno = 0;
    long long v = yajl_parse_integer(num, 19);
    /* triggers signed integer overflow, reported by UBSan */
    printf("%lld\n", v);
    return 0;
}

Trigger result

runtime error: signed integer overflow: 922337203685477587 * 10 cannot be represented in type 'long long'

# with halt_on_error=1 the process terminates with a non-zero exit code

• Replay exit code (recover mode): 0 (UBSan recovers by default, non-fatal)
• Replay exit code (halt_on_error): non-zero, process terminates
• Deterministic: yes

Suggested fix

1. Move the check before ret *= 10 and switch to divide-then-verify (divide before multiply) to eliminate the overflow entirely:

while (pos < number + length) {
    if (*pos < '0' || *pos > '9') {
        errno = ERANGE;
        return sign == 1 ? LLONG_MAX : LLONG_MIN;
    }
    if (ret > (LLONG_MAX - (*pos - '0')) / 10) {  /* full pre-check before the multiply */
        errno = ERANGE;
        return sign == 1 ? LLONG_MAX : LLONG_MIN;
    }
    ret = ret * 10 + (*pos++ - '0');
}

2. Simply reuse strtoll (matches the semantics claimed in the comment; the standard implementation already handles the boundary correctly):

#include <stdlib.h>
#include <errno.h>

long long
yajl_parse_integer(const unsigned char *number, unsigned int length)
{
    char buf[32];
    if (length >= sizeof(buf)) { errno = ERANGE; return LLONG_MAX; }
    memcpy(buf, number, length);
    buf[length] = '\0';
    return strtoll(buf, NULL, 10);
}

3. If the loop accumulation must stay, at minimum tighten the boundary from ret > MAX_VALUE_TO_MULTIPLY to ret > (LLONG_MAX - 9) / 10, checked before ret *= 10.

[robohack]

This report does demonstrate a new bug. But it is still AI slop. :-/

However as mentioned it is of relatively little consequence since even with UBSAN in use signed integer overflow can effectively be turned into an implementation defined operation with most modern compilers (e.g. using the -fwrapv compiler flag).

Of course hand-rolling an integer parser with overflow and underflow detection is silly when C99 has strtoll(). However the original reason strtoll() was avoided was to avoid locale specific (LC_NUMERIC) handling, which would be an undesirable extension of the JSON number format. However that can be avoided by temporarily changing the process' locale to "C".

Overall though this report shows too many signs of "AI" slop. The first suggested fix was obviously never tested since it actually just moves the UB operation to the new check.

The second suggested fix is obviously not complete as it fails to correctly check for underflow of a signed but too many digits value.

I didn't bother considering the third fix because using strtoll(), correctly, is the best option.

Note that suggested fixes should also include new tests that show the fixes have worked.

[1820893135-pixel]

Fixed in #265 (the bound before ret *= 10 is now LLONG_MAX / 10; the previous value was 7 too large, so ret == 922337203685477587 was accepted and the multiplication overflowed).