Conversation
…ad ECIES code Entries of connector bundles (lib/*, native/*), of IOUtil.unjar and of the maven plugin's own resources were resolved with new File(dir, name), so an entry such as lib/../../x was written outside of the target directory (CodeQL java/zipslip). IOUtil.resolveEntry now rejects any entry whose canonical path leaves the directory, and the four extraction sites use it. ECIESEncryptor (AES/CBC with an IV taken from the ECDH secret) was only reachable through OpenICFServerAdapter.initialiseEncryptor(), which nothing calls and which dereferences a null HandshakeMessage; both are removed. Also widens the contract tests' loop counters to long to match the long MAX_ITERATIONS parameter, and makes the ObjectPool wait loop's exits explicit - it only ever left by returning or throwing.
vharseko
added
security
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…izer The check compared the canonical path with "equals the directory or starts with its prefix"; CodeQL's path-injection sanitizer only credits a lone startsWith on a normalised path that guards the use. A trailing separator on both sides makes one startsWith cover the directory itself as well. resolveEntry now returns the canonical file, so the bundle temp directory is canonicalised too before its parent directories are walked.
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.
Closes 11 of the 21 open high-severity CodeQL alerts (the ones that can be fixed without a compatibility impact):
java/zipslip#7 #8 #9 #10,java/weak-cryptographic-algorithm#3 #4,java/comparison-with-wider-type#16 #17 #18 #19,java/unreachable-exit-in-loop#23.Zip slip
new File(dir, entry.getName())let an archive entry such aslib/../../xland outside of the directory it is extracted into:LocalConnectorInfoManagerImpl.copyStreamToFile(stream, name)— thelib/*andnative/*entries of a connector bundle are expanded intojava.io.tmpdir/bundle-<random>/. A crafted bundle wrote outside of that directory (the new test reproduced it: the file showed up injava.io.tmpdir). Whoever can drop a bundle into the bundle directory can run code as the server anyway, so this is defence in depth rather than a privilege gain, but it costs one line.IOUtil.unjar— public utility, no callers left in this repository.ConnectorInfoReportMojo/DocBookResourceMojo— extract the plugin's own jar at build time; guarded for uniformity.All four go through the new
IOUtil.resolveEntry(dir, entryName), which throwsIOExceptionwhen the entry's canonical path leavesdir. The directory itself passes: the plugin strips theshared/prefix, so its directory entry resolves to an empty name — the first cut rejected that and would have aborted the resource copy; covered byresolveEntryAcceptsTheDirectoryItself.Dead ECIES code
ECIESEncryptor(AES/CBC/PKCS5 with the IV taken from the ECDH secret) was only referenced fromOpenICFServerAdapter.initialiseEncryptor(), which nothing calls and which starts withHandshakeMessage message = null; message.getPublicKey(). Both are removed, together with the round-trip test inSecurityUtilTest.keyPairstays (it goes into the handshake message);SecurityUtil.doECDHstays as public API.Small ones
AuthenticationApiOpTests:for (int i …; i < getLongTestParam(MAX_ITERATIONS, 1); …)compared anintwith along; the counter islongnow.ObjectPool.borrowObjectNoTest:do { … } while (nanos > 0)— the body throws onnanos <= 0before the condition is ever evaluated, so the loop only left by returning or throwing; nowwhile (true)with a comment saying so. Same behaviour,ObjectPoolTestsgreen.Tests
IOUtilsTests:resolveEntryinside / directory itself / escaping (IOException);unjarrefuses../escaped.txtand does not write it.LocalConnectorInfoManagerTests.testRejectsBundleEntryEscapingTempDirectory: bundle withlib/ok.jarfollowed bylib/../../escaped-<uuid>.jar(the regular entry first, so thatlib/exists and the escape resolves) →ConfigurationException, nothing written tojava.io.tmpdir.longcounters have no tests: the plugin has no test harness and extracts its own artifact, and the counter change needsMAX_ITERATIONS > Integer.MAX_VALUE.Local runs: connector-framework 189, connector-framework-internal 470, connector-framework-server 28, all green; contract module and maven plugin compile.
Left open on purpose (separate decisions)
EncryptorImpl(update submodules #1 update opendj submodule #2 Bump org.apache.maven:maven-core from 3.0.4 to 3.8.1 in /OpenICF-maven-plugin #11 Bump com.jcraft:jsch from 0.1.53 to 0.1.54 in /OpenICF-ssh-connector #12): the default encryptor is the legacy wire format shared with the .NET connector server; the random one (in-memoryGuardedString) can move to AES/GCM without compatibility impact — separate PR.PasswordDecryptorDESede (#5 Merge submodules history in tree #6), LDAP{SHA}/{MD5}schemes (ADD Docker build images, test, release #27 optimise docker images size #28), SHA-1 connector key hash (Add IT test #25 FIX OpenIDM compatibility #26): formats dictated by external systems or existing configuration; candidates for dismissal with a recorded reason, or for a migration with dual support.