Guard paths to which output can be written #184
Conversation
drivenflywheel
force-pushed
the
guardOutputPaths
branch
from
035fc00
to
ebddc5d
Compare
drivenflywheel
force-pushed
the
guardOutputPaths
branch
from
ebddc5d
to
90a4ba7
Compare
drivenflywheel
changed the title
drivenflywheel
force-pushed
the
guardOutputPaths
branch
from
52747b8
to
479b646
Compare
| @@ -77,7 +77,7 @@ private Response processRegisterPeer(String peerKey, String dirName) { | |||
| } | |||
|
|
|||
| final Set<String> set = new HashSet<>(); | |||
| set.add(KeyManipulator.getDefaultDirectoryKey(peerKey)); | |||
| set.add(KeyManipulator.getDefaultDirectoryKey("#" + peerKey)); | |||
There was a problem hiding this comment.
The specific code change on this line has no functional impact (the prefix gets stripped by the KeyManipulator.getDefaultDirectory method).
We have existing and sufficient SSRF mitigation on lines 70-73, which restricts the hosts that can be registered through this endpoint. The "#" prefix added here is used to trigger the CodeQL RequestForgery.HostnameSanitizer flag, which indicates that we've sanitized the hostname to prevent a SSRF attack. This flag is considered when evaluating dataflow paths for SSRF / cwe-918 vulnerability, and we trip the flag to signal that this code path (already) includes hostname sanitation.
There was a problem hiding this comment.
Agreed.
Would it be worth adding a quick test case for the KeyManipulator that proves this behavior jic functionality gets changes somewhere down the road?
| */ | ||
| static String filePathIsWithinBaseDirectory(String requiredBase, String filePath) throws IllegalArgumentException { | ||
| // probably an overly simplistic test | ||
| if (filePath.contains("..")) { |
There was a problem hiding this comment.
The check on this line is a simple guard that satisfies a CodeQL path sanitizer check, our sanization is considered (by CodeQL scans) sufficient to defeat Uncontrolled data used in path expression" vulnerability.
Lines 1099 to 1103 perform more thorough validation to ensure that we consider the filePath resolves to a location we trust (and doesn't try to perform path traversal to an unexpected location), but we need the guard on line 1096 so line 1100 isn't mistakenly flagged as vulnerable code.
This change has no production impact: no existing production code paths reach this method, but it's reachable by legacy public static main(String[] args) methods in numerous Place implementations that call the ServiceProviderPlace.mainRunner method for ad hoc testing.
| try { | ||
| safePath = filePathIsWithinBaseDirectory(getBaseOutputDir(), fn); | ||
| if (Executrix.writeDataToFile(payload.data(), safePath)) { | ||
| logger.debug("Wrote attachment output to " + safePath); |
There was a problem hiding this comment.
I'd be ok with updating these to parameterized logging.
| * @throws IllegalArgumentException if the filePath contains illegal characters or is outside the required base | ||
| * directory | ||
| */ | ||
| static String filePathIsWithinBaseDirectory(String requiredBase, String filePath) throws IllegalArgumentException { |
There was a problem hiding this comment.
Worth protecting against null/empty inputs?
| * | ||
| * @param requiredBase required base directory | ||
| * @param filePath file path to be tested | ||
| * @return the normalized file path is within the required base directory |
There was a problem hiding this comment.
@return the normalized file path if within the required base directory
When I read it initially, my mind went to a boolean return.
| @@ -77,7 +77,7 @@ private Response processRegisterPeer(String peerKey, String dirName) { | |||
| } | |||
|
|
|||
| final Set<String> set = new HashSet<>(); | |||
| set.add(KeyManipulator.getDefaultDirectoryKey(peerKey)); | |||
| set.add(KeyManipulator.getDefaultDirectoryKey("#" + peerKey)); | |||
There was a problem hiding this comment.
Agreed.
Would it be worth adding a quick test case for the KeyManipulator that proves this behavior jic functionality gets changes somewhere down the road?
drivenflywheel
force-pushed
the
guardOutputPaths
branch
from
479b646
to
618042c
Compare
|
Incorporated feedback |
* Implemented a simple path guard to enforce paths to which output can be written
* Implemented a simple path guard to enforce paths to which output can be written
Addresses CodeQL feedback and resolves three CodeQL security alerts
Ticket 5422