fix: address CodeQL findings (zip-slip + implicit narrowing)#7576
Merged
Conversation
CodeQL java/zipslip flagged populateFileAndDirLists at lines 750/753; the actual sink is createFiles at line 619 (FileOutputStream) and createDirectories at line 617 (mkdirs), reached via a string-concat correctFileName that didn't normalise '..' segments. correctFileName now resolves each entry against its base directory and rejects any path whose canonical form leaves that base. Both canonicalisations happen on the resolved File, so symlinks, '.' and '..' segments are all collapsed before comparison. The two callers that didn't already throw IOException (checkOverwriteOK and createDirectories) catch and abort the install with the existing error-dialog pattern. A hostile data set containing 'data/../../etc/whatever' would now be refused before any file is written.
CodeQL java/implicit-cast-in-compound-assignment flagged 12 sites in this file where a 'double' bonus was accumulated into an 'int' via +=, which silently inserts (int) at every addition. The intent was always truncating accumulation -- the function returns Integer and uses .intValue() for the formula path -- so the warning is purely about making the cast visible. Fix: write the cast at every site. No behaviour change; the bytecode already contained the same i2d/d2i pair.
Reflective unit test so the same class compiles against both the pre-fix (`private`, no checked exception) and post-fix (package-private, throws IOException) signatures of correctFileName. Verified by hand: temporarily reverting DataInstaller to master and re-running this test yields 1 pass + 2 failures (both '..'-escape cases are silently accepted under the old logic), and restoring the fix flips it to 3 passes -- which makes the test a real regression guard rather than a tautology. correctFileName goes from `private` to package-private to give the test in the same package direct access; reflection was needed only for the cross-state run.
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.
Summary
Closes a couple of CodeQL findings I noticed and adds a regression test for the security-relevant one.
Zip Slip in
DataInstaller(java/zipslip, severity error). Archive extraction resolved entry names with plain string concat (destDir.getAbsolutePath() + suffix) and never normalised the result, so an entry likedata/../../etc/whateverwould have written outside the install directory.correctFileNamenow canonicalises both the base dir and the resolved entry path and refuses any entry that escapes the base. The two callers that didn't already throwIOException(checkOverwriteOK,createDirectories) catch the new exception and abort the install with the existing error-dialog pattern.Implicit narrowing in
SkillModifier(java/implicit-cast-in-compound-assignment, 12 alerts).int bonus += someDoublesilently inserted a narrowing cast at every accumulation. The function returnsIntegerand the formula path already uses.intValue(), so the truncating intent was correct — the warning was about it being invisible. Casts are now written explicitly. No behaviour change; the bytecode already truncated each step.Regression test for the zip-slip fix. Reflective unit test so the same test class can run against both the pre-fix and post-fix signatures of
correctFileName. Verified by hand: with the production fix reverted, the test reports 1 pass + 2 failures (both..-escape cases slip through silently); restoring the fix flips it to 3 passes.correctFileNamegoes fromprivateto package-private so the in-package test can call it directly; reflection was only needed for the cross-state demonstration.Test plan
:test(16,995 unit tests, 0 failures)SkillModifier:pcgen.core.prereq.PreVarTest,pcgen.core.display.SkillCostDisplayTest(11 tests, 0 failures)DataInstallerZipSlipTestpasses against the fixed code; verified by hand that it fails against the pre-fix code (see above)Notes
java/implicit-cast-in-compound-assignmentalerts remain in other files (SkillSitToken,WeaponToken,TotalWeightFacet, …). Out of scope for this PR.