Fix CodeQL warning-severity alerts: weak salt PRNG, unsafe DCL, thread-unsafe date formats - #789
Merged
vharseko merged 1 commit intoJul 30, 2026
Conversation
…d-unsafe date formats * Sha2Crypt: generate crypt salts from a shared SecureRandom instead of instantiating a non-cryptographic java.util.Random for every character. * GSSAPISASLMechanismHandler: publish the login context only after login() has completed, so a concurrent reader cannot obtain an unusable context. * CommonAudit: add the missing format specifier so the configuration entry DN is actually written to the trace log. * ConfigFromConnection: replace the shared static SimpleDateFormat and DateFormat fields with factory methods. This also fixes the UTC time zone being applied from an instance initializer, which left the parser using the default time zone until the first instance was created. * BaseDNCellRenderer: compare the not-available sentinels with equals() rather than reference equality, and make the sentinels final. * AsynchronousTextWriter: reject a null wrapped writer in the constructor and drop the two remaining null checks, which contradicted the unconditional dereferences elsewhere in the class.
vharseko
added
security
maximthomas
approved these changes
Jul 30, 2026
This was referenced Jul 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Triaged the 241 open warning-severity CodeQL alerts and fixed the six that are genuine defects. The remaining clusters are either mechanical hardening (missing
default:in enum switches, resource leaks in installer/upgrade tooling) or false positives (intentional non-short-circuit&, dead-but-harmless constant checks) and are left for follow-ups.Changes
Sha2Crypt(java/random-used-once) — crypt salts were generated with a freshjava.util.Randomper character.java.util.Randomis a linear congruential generator whose output is predictable from observed values (CWE-338), which is not acceptable for a password salt. Replaced with a single sharedSecureRandom. Reached fromsha256Crypt/sha512Crypt, i.e. the live password hashing path.GSSAPISASLMechanismHandler(java/unsafe-double-checked-locking-init-order) —loginContextwas assigned beforelogin()ran, so a thread passing the unsynchronized outer null check could obtain a context whose login had not completed. The context is now published only afterlogin()returns.CommonAudit(java/unused-format-argument) —String.format("... configuration entry:", config.dn())had no placeholder, silently dropping the DN from the trace log.ConfigFromConnection(java/thread-unsafe-dateformat) — the shared staticSimpleDateFormat/DateFormatfields are replaced bynewUtcParser()/newDateFormatter()factory methods. This also fixes a latent bug:setTimeZone("UTC")sat in an instance initializer operating on a static field, soutcParserparsed in the default time zone until the firstConfigFromConnectioninstance was constructed. Call sites inRootMonitoringPanel,ServerDescriptorandUtilitiesare updated.BaseDNCellRenderer(java/reference-equality-on-strings) — the not-available sentinels were compared with==againstpublic static(non-final)Stringfields; this only worked by reference coincidence. Switched toequals()and made the sentinelsfinal.AsynchronousTextWriter(java/dereferenced-value-may-be-null) —writer.writeRecord()was called unconditionally at one point and null-checked a few lines later. The class already dereferenceswriterunconditionally inflush(),getBytesWritten()and the writer thread, and all five call sites pass a freshly constructed writer, so the constructor now rejectsnulland the two vestigial checks are removed.Verification
mvn -o -pl opendj-server-legacy -am compile— passes.opendj-server-legacydisables surefire (tests run through failsafe against a real server), so the unit-test route does not cover these classes. TheSha2Cryptchange was verified directly against the built classes:$5$/$6$round-trip over 200 iterations, salt shape (8 chars fromB64T), and 2000 consecutive salts all distinct.Note for reviewers:
Sha2Cryptdeliberately zeroes the key array passed to it (Arrays.fill(keyBytes, (byte) 0), upstream commons-codec behaviour), so any test calling it twice must supply a fresh password array each time.