allthingssecurity commented on code in PR #27119:
URL: https://github.com/apache/camel/pull/27119#discussion_r4165000428
##########
components/camel-google/camel-google-storage/src/main/java/org/apache/camel/component/google/storage/GoogleCloudStorageConsumer.java:
##########
@@ -360,13 +360,30 @@ protected String evaluateFileExpression(Exchange
exchange, String downloadFileNa
// use blob as file name
exchange.getMessage().setHeader(GoogleCloudStorageConstants.FILE_NAME,
blogName);
+ // the local path is resolved from
GoogleCloudStorageConstants.FILE_NAME set above, which carries the remote
+ // object name and is therefore untrusted input, no matter whether the
token is appended here or already part
+ // of the configured downloadFileName. An absolute object name, or one
with a .. segment, is rejected whatever
+ // the configuration looks like, and the resolved path is then
confined:
+ // - plain directory (no expression): the object name is appended to
it and the configured value itself is the
+ // directory the download must stay within
+ // - directory followed by an expression (for example
/tmp/downloads/${file:name}): the static directory
+ // prefix before the first expression token is the directory the
download must stay within
+ // - fully dynamic value with no static directory prefix (for example
${file:name}): the route author did not
+ // configure any directory, so a relative result must stay within
the working directory
+ if (blogName != null) {
+ GoogleCloudStorageFileNameHelper.assertSafeObjectName(blogName);
+ }
Review Comment:
The containment itself looks right to me, and the module tests pass. One
behavior question about running `assertSafeObjectName` before the branching,
even when `downloadFileName` does not use the object name at all.
With `downloadFileName=target/dl/${exchangeId}.bin`, `deleteAfterRead=true`,
and objects `a.txt`, `b/../c.txt`, `d.txt`:
- On main all 3 objects are routed (3 files on disk).
- On this PR none are. Every poll fails with `Cannot download to file
'b/../c.txt' as the object name has a '..' path segment`, and since
`createExchanges(List<Blob>)` rethrows the first exception, the other objects
in the bucket are not consumed either, until someone removes that object.
Two possible ways:
- Run the name check only where the name actually ends up in the path. In
the two confined branches `assertWithinDirectory` already covers it, so the
check is really needed for the fully dynamic branch. Strictly rejecting
`a/../b.txt` in the plain-directory branch is also a fair choice, as the
upgrade guide says.
- Or keep the strict check, but handle a rejected object on its own
(`getExceptionHandler().handleException(...)` and skip it) instead of failing
the poll. The upgrade guide could then say what happens to such an object.
Small note: `hasDriveLetter` is applied on every OS, so on Linux an object
called `C:report.txt` is rejected too. The upgrade guide mentions drive
letters, so this is fine if intended.
_Claude Code on behalf of allthingssecurity_
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
