From 0333ef4a52b479067459bfaa4bde3f74bed47c32 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Fri, 24 Jul 2026 20:59:55 +0300 Subject: [PATCH 1/3] Fix java/tainted-arithmetic CodeQL alerts 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. --- .../org/forgerock/opendj/ldap/ByteSequenceReader.java | 6 +++++- .../java/org/forgerock/opendj/ldap/GeneralizedTime.java | 9 ++++++++- .../java/org/opends/server/replication/common/CSN.java | 5 +++-- 3 files changed, 16 insertions(+), 4 deletions(-) diff --git a/opendj-core/src/main/java/org/forgerock/opendj/ldap/ByteSequenceReader.java b/opendj-core/src/main/java/org/forgerock/opendj/ldap/ByteSequenceReader.java index c09d546c79..2331d15aed 100644 --- a/opendj-core/src/main/java/org/forgerock/opendj/ldap/ByteSequenceReader.java +++ b/opendj-core/src/main/java/org/forgerock/opendj/ldap/ByteSequenceReader.java @@ -499,7 +499,11 @@ public byte peek(int offset) { * of the underlying byte sequence. */ public void skip(final int length) { - position(pos + length); + final long newPos = (long) pos + length; + if (newPos < 0 || newPos > sequence.length()) { + throw new IndexOutOfBoundsException(); + } + position((int) newPos); } @Override diff --git a/opendj-core/src/main/java/org/forgerock/opendj/ldap/GeneralizedTime.java b/opendj-core/src/main/java/org/forgerock/opendj/ldap/GeneralizedTime.java index 6045ac8090..c125ef8e40 100644 --- a/opendj-core/src/main/java/org/forgerock/opendj/ldap/GeneralizedTime.java +++ b/opendj-core/src/main/java/org/forgerock/opendj/ldap/GeneralizedTime.java @@ -12,6 +12,7 @@ * information: "Portions Copyright [year] [name of copyright owner]". * * Copyright 2012-2016 ForgeRock AS. + * Portions Copyrighted 2026 3A Systems, LLC. */ package org.forgerock.opendj.ldap; @@ -653,7 +654,13 @@ private static GeneralizedTime finishDecodingFraction(final String value, final throw new LocalizedIllegalArgumentException(message); } - final Double fractionValue = Double.parseDouble(fractionBuffer.toString()); + final double fractionValue = Double.parseDouble(fractionBuffer.toString()); + if (!(fractionValue >= 0.0d && fractionValue < 1.0d)) { + // Cannot happen: the buffer contains "0." followed by decimal digits. + final LocalizableMessage message = + WARN_ATTR_SYNTAX_GENERALIZED_TIME_ILLEGAL_TIME.get(value, fractionBuffer); + throw new LocalizedIllegalArgumentException(message); + } final int additionalMilliseconds = (int) Math.round(fractionValue * multiplier); try { diff --git a/opendj-server-legacy/src/main/java/org/opends/server/replication/common/CSN.java b/opendj-server-legacy/src/main/java/org/opends/server/replication/common/CSN.java index 24f2c04726..37668854f4 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/replication/common/CSN.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/replication/common/CSN.java @@ -13,6 +13,7 @@ * * Copyright 2006-2009 Sun Microsystems, Inc. * Portions Copyright 2013-2015 ForgeRock AS. + * Portions Copyrighted 2026 3A Systems, LLC. */ package org.opends.server.replication.common; @@ -319,11 +320,11 @@ else if (csn1.timeStamp != csn2.timeStamp) } else if (csn1.seqnum != csn2.seqnum) { - return csn1.seqnum - csn2.seqnum; + return Integer.compare(csn1.seqnum, csn2.seqnum); } else { - return csn1.serverId - csn2.serverId; + return Integer.compare(csn1.serverId, csn2.serverId); } } From 2d46e7524bdcc2ee11ea3c87b87b9dfe681f903e Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Mon, 27 Jul 2026 15:58:41 +0300 Subject: [PATCH 2/3] Address review: validate scaled fraction in GeneralizedTime, add tests - GeneralizedTime: guard the value actually consumed - Math.round(fraction * multiplier) must stay in [0, multiplier) - instead of the parsed fraction: parseDouble rounds "0." + 17 nines to exactly 1.0, and 16 nines scale and round to 1000 ms; both previously passed schema validation and failed later in getTimeInMillis() with an unlocalized IllegalArgumentException. - Tests: GeneralizedTimeTest rejects 16/17-nines fractions, CSNTest covers extreme-seqnum ordering and transitivity, ByteSequenceReaderTest covers skip() overflow. - Align header wording with repo convention (Portions Copyright). --- .../opendj/ldap/GeneralizedTime.java | 12 +++++---- .../opendj/ldap/ByteSequenceReaderTest.java | 9 +++++++ .../opendj/ldap/GeneralizedTimeTest.java | 7 ++++- .../opends/server/replication/common/CSN.java | 2 +- .../server/replication/common/CSNTest.java | 27 ++++++++++++++++--- 5 files changed, 47 insertions(+), 10 deletions(-) diff --git a/opendj-core/src/main/java/org/forgerock/opendj/ldap/GeneralizedTime.java b/opendj-core/src/main/java/org/forgerock/opendj/ldap/GeneralizedTime.java index c125ef8e40..5ca61f18d9 100644 --- a/opendj-core/src/main/java/org/forgerock/opendj/ldap/GeneralizedTime.java +++ b/opendj-core/src/main/java/org/forgerock/opendj/ldap/GeneralizedTime.java @@ -12,7 +12,7 @@ * information: "Portions Copyright [year] [name of copyright owner]". * * Copyright 2012-2016 ForgeRock AS. - * Portions Copyrighted 2026 3A Systems, LLC. + * Portions Copyright 2026 3A Systems, LLC. */ package org.forgerock.opendj.ldap; @@ -655,20 +655,22 @@ private static GeneralizedTime finishDecodingFraction(final String value, final } final double fractionValue = Double.parseDouble(fractionBuffer.toString()); - if (!(fractionValue >= 0.0d && fractionValue < 1.0d)) { - // Cannot happen: the buffer contains "0." followed by decimal digits. + final long additionalMilliseconds = Math.round(fractionValue * multiplier); + if (additionalMilliseconds < 0 || additionalMilliseconds >= multiplier) { + // Parsing and scaling both round up: "0." followed by 17 nines parses as exactly 1.0, + // and with 16 nines the rounded product is still 1000. The calendar below only + // validates lazily, so reject the out-of-range value here. final LocalizableMessage message = WARN_ATTR_SYNTAX_GENERALIZED_TIME_ILLEGAL_TIME.get(value, fractionBuffer); throw new LocalizedIllegalArgumentException(message); } - final int additionalMilliseconds = (int) Math.round(fractionValue * multiplier); try { final GregorianCalendar calendar = new GregorianCalendar(); calendar.setLenient(false); calendar.setTimeZone(timeZone); calendar.set(year, month, day, hour, minute, second); - calendar.set(Calendar.MILLISECOND, additionalMilliseconds); + calendar.set(Calendar.MILLISECOND, (int) additionalMilliseconds); return new GeneralizedTime(calendar, null, Long.MIN_VALUE, value); } catch (final Exception e) { // This should only happen if the provided date wasn't legal diff --git a/opendj-core/src/test/java/org/forgerock/opendj/ldap/ByteSequenceReaderTest.java b/opendj-core/src/test/java/org/forgerock/opendj/ldap/ByteSequenceReaderTest.java index 2b60f18902..d7f33581e9 100644 --- a/opendj-core/src/test/java/org/forgerock/opendj/ldap/ByteSequenceReaderTest.java +++ b/opendj-core/src/test/java/org/forgerock/opendj/ldap/ByteSequenceReaderTest.java @@ -13,6 +13,7 @@ * * Copyright 2009 Sun Microsystems, Inc. * Portions Copyright 2014-2015 ForgeRock AS. + * Portions Copyright 2026 3A Systems, LLC. */ package org.forgerock.opendj.ldap; @@ -347,6 +348,14 @@ public void testSkip(ByteSequenceReader reader, byte[] ba) { reader.skip(1); } + @Test(dataProvider = "readerProvider", expectedExceptions = IndexOutOfBoundsException.class) + public void testSkipOverflow(ByteSequenceReader reader, byte[] ba) { + reader.rewind(); + reader.skip(ba.length); + // pos + Integer.MAX_VALUE overflows int; must throw, not wrap around. + reader.skip(Integer.MAX_VALUE); + } + @Test(dataProvider = "readerProvider") public void testPeek(ByteSequenceReader reader, byte[] ba) { reader.rewind(); diff --git a/opendj-core/src/test/java/org/forgerock/opendj/ldap/GeneralizedTimeTest.java b/opendj-core/src/test/java/org/forgerock/opendj/ldap/GeneralizedTimeTest.java index a08e02026c..4066fa4cce 100644 --- a/opendj-core/src/test/java/org/forgerock/opendj/ldap/GeneralizedTimeTest.java +++ b/opendj-core/src/test/java/org/forgerock/opendj/ldap/GeneralizedTimeTest.java @@ -13,6 +13,7 @@ * * Copyright 2009 Sun Microsystems, Inc. * Portions copyright 2012-2015 ForgeRock AS. + * Portions Copyright 2026 3A Systems, LLC. */ package org.forgerock.opendj.ldap; @@ -67,7 +68,11 @@ public Object[][] invalidStrings() { { "2006122a235959Z" }, { "20060031235959Z" }, { "20061331235959Z" }, { "20062231235959Z" }, { "20061232235959Z" }, { "2006123123595aZ" }, { "200a1231235959Z" }, { "2006j231235959Z" }, { "200612-1235959Z" }, - { "20061231#35959Z" }, { "2006" }, }; + { "20061231#35959Z" }, { "2006" }, + // Double.parseDouble rounds 17 nines up to exactly 1.0. + { "20240101000000.99999999999999999Z" }, + // 16 nines stay below 1.0 but scale and round to 1000 milliseconds. + { "20240101000000.9999999999999999Z" }, }; } @Test diff --git a/opendj-server-legacy/src/main/java/org/opends/server/replication/common/CSN.java b/opendj-server-legacy/src/main/java/org/opends/server/replication/common/CSN.java index 37668854f4..0026223370 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/replication/common/CSN.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/replication/common/CSN.java @@ -13,7 +13,7 @@ * * Copyright 2006-2009 Sun Microsystems, Inc. * Portions Copyright 2013-2015 ForgeRock AS. - * Portions Copyrighted 2026 3A Systems, LLC. + * Portions Copyright 2026 3A Systems, LLC. */ package org.opends.server.replication.common; diff --git a/opendj-server-legacy/src/test/java/org/opends/server/replication/common/CSNTest.java b/opendj-server-legacy/src/test/java/org/opends/server/replication/common/CSNTest.java index 307f399afd..1c65a05d1b 100644 --- a/opendj-server-legacy/src/test/java/org/opends/server/replication/common/CSNTest.java +++ b/opendj-server-legacy/src/test/java/org/opends/server/replication/common/CSNTest.java @@ -13,6 +13,7 @@ * * Copyright 2006-2009 Sun Microsystems, Inc. * Portions Copyright 2013-2016 ForgeRock AS. + * Portions Copyright 2026 3A Systems, LLC. */ package org.opends.server.replication.common; @@ -77,9 +78,9 @@ public void csnEncodeDecode(long time, int seq, int id, String str) throws Excep @DataProvider(name = "createCSN") public Object[][] createCSNData() { - long time[] = {1, TimeThread.getTime()}; - int seq[] = {0, 123}; - int id [] = {1, 45}; + long time[] = {1, TimeThread.getTime(), 1, 1}; + int seq[] = {0, 123, Integer.MIN_VALUE, Integer.MAX_VALUE - 1}; + int id [] = {1, 45, 1, 1}; Object[][] obj = new Object[time.length][5]; for (int i=0; i 0); } + /** + * Test that {@link CSN#compare(CSN, CSN)} orders extreme seqnums correctly and + * transitively: subtraction-based comparison overflowed for MIN_VALUE vs + * MAX_VALUE at equal timestamps and inverted the sign, which is fatal for + * TreeMaps keyed on CSN. + */ + @Test + public void csnCompareExtremeSeqnums() throws Exception + { + final CSN minSeqnum = new CSN(1, Integer.MIN_VALUE, 1); + final CSN zeroSeqnum = new CSN(1, 0, 1); + final CSN maxSeqnum = new CSN(1, Integer.MAX_VALUE, 1); + + assertTrue(CSN.compare(minSeqnum, zeroSeqnum) < 0); + assertTrue(CSN.compare(zeroSeqnum, maxSeqnum) < 0); + assertTrue(CSN.compare(minSeqnum, maxSeqnum) < 0); + assertTrue(CSN.compare(maxSeqnum, minSeqnum) > 0); + assertEquals(CSN.compare(maxSeqnum, maxSeqnum), 0); + } + /** Test {@link CSN#isOlderThan(CSN)} method. */ @Test(dataProvider = "createCSN") public void csnOlder(CSN csn1, CSN csn2, CSN csn3, CSN csn4, CSN csn5) throws Exception From 2a6df5b866a4d151ad230b02c94ac0f591971d62 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Mon, 27 Jul 2026 16:15:57 +0300 Subject: [PATCH 3/3] Validate fraction before scaling so CodeQL recognizes the barrier The range check ran after the multiplication, so the merge-ref analysis re-raised java/tainted-arithmetic on the same expression (alert 1225) and flagged the now-moved Double.parseDouble with java/uncaught-number-format-exception (alert 1226). Guard the parsed fraction to [0, 1) before scaling and catch NumberFormatException with the documented LocalizedIllegalArgumentException. The post-round check remains for the 16-nines case, where 0.9999999999999999 * 1000 still rounds to 1000. --- .../opendj/ldap/GeneralizedTime.java | 22 ++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/opendj-core/src/main/java/org/forgerock/opendj/ldap/GeneralizedTime.java b/opendj-core/src/main/java/org/forgerock/opendj/ldap/GeneralizedTime.java index 5ca61f18d9..d58dbcfc91 100644 --- a/opendj-core/src/main/java/org/forgerock/opendj/ldap/GeneralizedTime.java +++ b/opendj-core/src/main/java/org/forgerock/opendj/ldap/GeneralizedTime.java @@ -654,12 +654,24 @@ private static GeneralizedTime finishDecodingFraction(final String value, final throw new LocalizedIllegalArgumentException(message); } - final double fractionValue = Double.parseDouble(fractionBuffer.toString()); + final double fractionValue; + try { + fractionValue = Double.parseDouble(fractionBuffer.toString()); + } catch (final NumberFormatException e) { + final LocalizableMessage message = + WARN_ATTR_SYNTAX_GENERALIZED_TIME_ILLEGAL_TIME.get(value, fractionBuffer); + throw new LocalizedIllegalArgumentException(message, e); + } + if (fractionValue < 0.0d || fractionValue >= 1.0d) { + // "0." followed by 17 nines parses as exactly 1.0, so reject before scaling. + final LocalizableMessage message = + WARN_ATTR_SYNTAX_GENERALIZED_TIME_ILLEGAL_TIME.get(value, fractionBuffer); + throw new LocalizedIllegalArgumentException(message); + } final long additionalMilliseconds = Math.round(fractionValue * multiplier); - if (additionalMilliseconds < 0 || additionalMilliseconds >= multiplier) { - // Parsing and scaling both round up: "0." followed by 17 nines parses as exactly 1.0, - // and with 16 nines the rounded product is still 1000. The calendar below only - // validates lazily, so reject the out-of-range value here. + if (additionalMilliseconds >= multiplier) { + // 16 nines: 0.9999999999999999 * 1000 still rounds up to 1000. The calendar below + // only validates lazily, so reject the out-of-range value here. final LocalizableMessage message = WARN_ATTR_SYNTAX_GENERALIZED_TIME_ILLEGAL_TIME.get(value, fractionBuffer); throw new LocalizedIllegalArgumentException(message);