Skip to content

#1063: bound BigDecimal→BigInteger expansion in objectToBigInteger (completes CVE-2026-59171 fix)#1067

Open
mechko wants to merge 3 commits into
stleary:max-number-length-configfrom
mechko:1063-biginteger-exponent-dos
Open

#1063: bound BigDecimal→BigInteger expansion in objectToBigInteger (completes CVE-2026-59171 fix)#1067
mechko wants to merge 3 commits into
stleary:max-number-length-configfrom
mechko:1063-biginteger-exponent-dos

Conversation

@mechko

@mechko mechko commented Jul 14, 2026

Copy link
Copy Markdown

Follow-up to #1065; see this comment on #1063 for the residual vector. Targets max-number-length-config per @stleary's request.

Problem

JSONObject.objectToBigInteger calls BigDecimal.toBigInteger() with no bound on the resulting magnitude. A short exponent-notation literal such as 1e100000000 (11 chars — well under the maxNumberLength guard) is stored compactly as a BigDecimal at parse time, then expanded to a ~100 000 000-digit BigInteger when the caller invokes getBigInteger / optBigInteger, stalling the thread for tens of seconds or throwing OutOfMemoryError.

Repro against max-number-length-config @ 90563eb (pre-patch):

$ timeout 30 java -Xmx512m -cp target/classes:. Repro ; echo EXIT=$?
parsed: java.math.BigDecimal
EXIT=124          # killed after 30 s

Fix

Before calling .toBigInteger(), reject any BigDecimal whose integer part would exceed ParserConfiguration.DEFAULT_MAX_NUMBER_LENGTH decimal digits:

if ((long) bd.precision() - bd.scale() > ParserConfiguration.DEFAULT_MAX_NUMBER_LENGTH) {
    return defaultValue;
}

Applied at both sink sites in objectToBigInteger (JSONObject.java:1402 and :1432). precision() and scale() are O(1) metadata reads, so the check is free. JSONArray.{get,opt}BigInteger delegate to the same helper, so all four public accessors are covered.

Uses the DEFAULT_MAX_NUMBER_LENGTH constant introduced on this branch so the parse-time and accessor-time ceilings stay in sync. The accessors don't carry a ParserConfiguration, so per-instance configuration is left as follow-up (#1066) to keep this change minimal for the release window.

Behaviour change

For a value whose integer part exceeds DEFAULT_MAX_NUMBER_LENGTH (1000) digits:

Call Before After
optBigInteger(key, dflt) returns (slowly / OOM for large exp) returns dflt
getBigInteger(key) returns (slowly / OOM for large exp) throws JSONException("… is not a BigInteger …")

Matches how the method already handles other unconvertible values (non-finite doubles, unparseable strings). The Double/Float branch needs no guard — max finite double ≈ 1.8e308 → 309 digits.

Verification

Post-patch, same reproducer:

parsed: java.math.BigDecimal
threw: org.json.JSONException: JSONObject["x"] is not a BigInteger (class java.math.BigDecimal : 1E+100000000).
elapsed ms: 0
optBigInteger: null

mvn test: 788 run, 0 fail, 0 error, 6 skipped (pre-existing).

Tests

Added JSONObjectTest.getBigIntegerHugeExponentReturnsDefault (@Test(timeout = 5000) so a regression fails fast rather than hanging CI):

  • {"x":1e100000000}optBigInteger returns null, getBigInteger throws, both in 0 ms
  • String path put("x", "1e100000000") behaves identically
  • JSONArray accessor covered
  • Boundary: {"x":1e999} still returns 10^999 correctly

🤖 Generated with Claude Code

…teger

Completes the CVE-2026-59171 fix started in ab92bb9 / stleary#1065. The
1000-char length guard in stringToValue admits short exponent-notation
literals (e.g. 1e100000000, 11 chars) which are stored compactly as
BigDecimal and only expand when getBigInteger/optBigInteger calls
BigDecimal.toBigInteger(), materialising ~10^8 digits and stalling the
thread or throwing OOM.

Guard both toBigInteger() sites in objectToBigInteger by rejecting any
BigDecimal whose integer part would exceed
ParserConfiguration.DEFAULT_MAX_NUMBER_LENGTH decimal digits
(precision() - scale(), both O(1) reads). Returns defaultValue on
overflow, matching the method's existing behaviour for non-finite and
unparseable values.

Covers JSONObject.getBigInteger/optBigInteger and
JSONArray.getBigInteger/optBigInteger (all delegate to this helper).

Adds JSONObjectTest.getBigIntegerHugeExponentReturnsDefault with a 5s
timeout so a regression fails fast rather than hanging CI.

Co-Authored-By: Claude <noreply@anthropic.com>
…ud java:S108)

Co-Authored-By: Claude <noreply@anthropic.com>
@sonarqubecloud

Copy link
Copy Markdown

@stleary

stleary commented Jul 15, 2026

Copy link
Copy Markdown
Owner

@mechko Looks good, but a bit more work is needed.

  1. Shouldn't objectToBigDecimal() also be updated and tested?
  2. ParserConfiguration now allows the user to set an arbitrary maxNumberLength for large numbers. It defaults to 1000, but the user can set a different value, and also disable large number checking by setting maxNumberLength to -1. The (opt|get)Big(Integer|Decimal) methods need to include an optional JSONParserConfiguration instance, and the tests should cover all config options.
    Here is an example of how the new API methods should work:
    BigInteger bigInteger = jsonObject.getBigInteger(
        myKey, 
        new JSONParserConfiguration().withMaxNumberLength(2000));

The existing API methods should be updated to call the new methods with a default config instance. For example:

static BigInteger objectToBigInteger(Object val, BigInteger defaultValue) {
    return objectToBigInteger(val, defaultValue, new JSONParserConfiguration());
}

Leaving this PR open for a day in case you have time to address this. Otherwise, I can add the changes in a day or so. Let me know if you have any questions.

@mechko

mechko commented Jul 15, 2026

Copy link
Copy Markdown
Author

@stleary To your first point, from what I can see (and the LLM I'm using), nothing in objectToBigDecimal is problematic in a DoS sense, like nothing expands the value: BigDecimal keeps 1e100000000 as {unscaled=1, scale=-1e8}
(~few dozen bytes) end-to-end, so getBigDecimal on that input is already O(1) and safe.

However, we could introduce a similar guard there (to keep it symmetric), but it would change the behaviour without the resource-exhaustion angle we have in objectToBigInteger(rejecting e.g. 1e2000 from getBigDecimal then).

Let me know what's your preference (leaving it the way it is, or implementing a similar guard for reasons of symmetry).

The other refactoring you mentioned can be implemented easily I assume, I can do that as well.

@stleary

stleary commented Jul 15, 2026

Copy link
Copy Markdown
Owner

@mechko, OK to leave objectToBigDecimal() as is.
Please do implement the optional JSONConfiguration parameter for objectToBigInteger().

…/optBigInteger

Per review on stleary#1067:
- objectToBigInteger(val, dflt, JSONParserConfiguration) uses
  cfg.getMaxNumberLength() for the digit-count guard; -1 disables it.
  Existing 2-arg form delegates with a default config.
- New public overloads on JSONObject and JSONArray:
  getBigInteger(key, cfg) / optBigInteger(key, dflt, cfg).
  Existing methods delegate with a default config.
- objectToBigDecimal left unchanged (no expansion path; agreed on PR).
- Tests cover default (1000), raised (2000), lowered (5), disabled (-1),
  null config, and JSONArray overloads.

Co-Authored-By: Claude <noreply@anthropic.com>
@mechko

mechko commented Jul 16, 2026

Copy link
Copy Markdown
Author

@stleary Done — pushed in 703ac34.

  • objectToBigInteger(Object, BigInteger, JSONParserConfiguration) added;
    guard now uses cfg.getMaxNumberLength() and skips when it's
    UNDEFINED_MAXIMUM_NUMBER_LENGTH. The existing 2-arg form delegates
    with new JSONParserConfiguration() as in your example.
  • New public overloads: JSONObject.getBigInteger(String, cfg),
    JSONObject.optBigInteger(String, BigInteger, cfg),
    JSONArray.getBigInteger(int, cfg),
    JSONArray.optBigInteger(int, BigInteger, cfg). Existing methods
    delegate with a default config.
  • objectToBigDecimal() left as-is per your last comment.
  • Tests cover default (1000), raised limit (2000), lowered limit (5),
    disabled (-1), null config, and the JSONArray overloads.

mvn test: 789 pass. Happy to adjust anything else.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants