Skip to content

Fix java/tainted-arithmetic CodeQL alerts#765

Open
vharseko wants to merge 1 commit into
OpenIdentityPlatform:masterfrom
vharseko:fix-codeql-java-tainted-arithmetic
Open

Fix java/tainted-arithmetic CodeQL alerts#765
vharseko wants to merge 1 commit into
OpenIdentityPlatform:masterfrom
vharseko:fix-codeql-java-tainted-arithmetic

Conversation

@vharseko

Copy link
Copy Markdown
Member

Fixes all four open java/tainted-arithmetic CodeQL code-scanning alerts (High): #111, #112, #113, #114.

Verified:

  • All 122 tests in GeneralizedTimeTest and ByteSequenceReaderTest (opendj-core) pass.
  • CSN.compare checked directly against the compiled classes: overflow cases now return the correct sign, regular ordering and equality unchanged.

Use Integer.compare instead of subtraction in CSN.compare: seqnum and
serverId come from replication messages and the subtraction could
overflow, inverting the comparison sign. Compute the new position in
ByteSequenceReader.skip in long arithmetic to avoid int overflow, and
validate the parsed fraction range in GeneralizedTime before scaling.
@vharseko
vharseko requested a review from maximthomas July 24, 2026 18:00
@vharseko vharseko added security Security fixes / CodeQL code-scanning alerts bug java Pull requests that update java code replication labels Jul 24, 2026
}

final Double fractionValue = Double.parseDouble(fractionBuffer.toString());
final double fractionValue = Double.parseDouble(fractionBuffer.toString());

@maximthomas maximthomas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CSN.compare() fix is correct and more valuable than the description suggests — the real hazard isn't a flipped sign but comparator non-transitivity, and CSN is a TreeMap key in MsgQueue, PendingChanges, RemotePendingChanges, LDAPReplicationDomain.replayOperations and EntryHistorical:

seqnum MIN_VALUE / 0 / MAX_VALUE at equal timestamps, old code:
  MIN < 0    0 < MAX    but  MIN - MAX = +1  =>  MIN > MAX

Reachable only via CSN.valueOf(ByteSequence) (raw readInt() off the replication wire / changelog) — CSNGenerator guards ++seqnum <= 0, and CSN(String) can't parse one (Integer.parseInt("80000000", 16) throws). Verified: no sign change for non-negative seqnums, and every call site uses only the sign.

Two things to fix in GeneralizedTime before merge.

GeneralizedTime: the "Cannot happen" comment is wrong — the branch is live (Medium)

Double.parseDouble rounds: "0." + 17 nines parses to exactly 1.0. The branch is reachable and load-bearing. Measured at the schema layer (GeneralizedTimeSyntaxImpl.valueIsAcceptable, which catches only LocalizedIllegalArgumentException and never calls getTimeInMillis()):

20240101000000.<n nines>Z base this PR
17 nines schema-accepted, then IllegalArgumentException: MILLISECOND later rejected at schema check

So the change is a real fix, not an assertion. Please correct the comment and the PR body — as written, someone will delete a live branch as dead code.

GeneralizedTime: the guard checks fractionValue, not the value actually used (Medium)

fractionValue < 1.0 doesn't bound additionalMilliseconds. With 16 nines the guard passes but the rounded result is still out of range, so the value survives schema validation and detonates later as an unlocalized IllegalArgumentException — outside the method's declared contract, because the Calendar is evaluated lazily in getTimeInMillis():

Math.round(0.9999999999999999 * 1000) == 1000   // legal Calendar.MILLISECOND is [0,999]
16 nines, base: schema-accepted -> later IllegalArgumentException: MILLISECOND
16 nines, PR:   schema-accepted -> later IllegalArgumentException: MILLISECOND   <- unchanged

Guarding the value actually consumed catches both cases:

final double fractionValue = Double.parseDouble(fractionBuffer.toString());
final long additionalMilliseconds = Math.round(fractionValue * multiplier);
if (additionalMilliseconds < 0 || additionalMilliseconds >= multiplier) {
    throw new LocalizedIllegalArgumentException(
            WARN_ATTR_SYNTAX_GENERALIZED_TIME_ILLEGAL_TIME.get(value, fractionBuffer));
}

Pre-existing, but it's the same defect class the PR is closing, one line away.

No tests for three behaviour-relevant changes (Low)

  • opendj-server-legacy/src/test/java/org/opends/server/replication/common/CSNTest.javacompare() with negative/extreme seqnums plus a transitivity assertion. Line 305 already uses Integer.MAX_VALUE-1, and compareToEquivalentToEquals (line 337) is data-driven, so both are cheap to extend.
  • opendj-core/src/test/java/org/forgerock/opendj/ldap/GeneralizedTimeTest.java — the 17-nines value is now rejected where it previously wasn't. That's the only thing keeping the comment honest.

Nits

  • ByteSequenceReader.skip() is behaviourally a no-op: with pos ∈ [0, 2³¹-1] and length ∈ [0, 2³¹-1] the sum tops out at 2³²-2, so overflow always wraps into [-2³¹, -2] — negative, which position() already rejected. A sweep of the input space found zero old-vs-new divergences in accept/reject. Fine to keep (silences the alert, documents intent), but "pos + length can no longer overflow int" in the PR body implies a defect that wasn't there.
  • The serverId half of the CSN change is cosmetic: both decode paths bound it to [0, 65535] (readShort() & 0xffff; 4 hex chars), so that subtraction could never overflow. Harmless and consistent, just not a fix.
  • Copyright wording: Portions Copyrighted 2026 — repo convention is Portions Copyright 2026 3A Systems, LLC. (74 occurrences vs 9). Not enforced; the plugin is commented out in pom.xml:56.
  • Header applied inconsistently: GeneralizedTime.java and CSN.java got the line, ByteSequenceReader.java was modified but didn't.
  • Message reuse: WARN_ATTR_SYNTAX_GENERALIZED_TIME_ILLEGAL_TIME's second placeholder is an exception/reason everywhere else; passing fractionBuffer renders as "…represents an invalid time (e.g., a date that does not exist): 0.99999999999999999". Serviceable, slightly off-register.

Out of scope

finishDecodingFraction sets the scaled fraction into Calendar.MILLISECOND instead of adding it to the time, so all fractional-minute and fractional-hour values are broken, not just edge cases:

20240101000000.5Z   OK    millis=1704067200500
202401010000.5Z     IllegalArgumentException: MILLISECOND
2024010100.5Z       IllegalArgumentException: MILLISECOND
2024010100.001Z     IllegalArgumentException: MILLISECOND

Pre-existing and unrelated — worth a separate issue. It also means the new guard only ever matters on the seconds path.

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

Labels

bug java Pull requests that update java code replication security Security fixes / CodeQL code-scanning alerts

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants