bug: reject path traversal in uploaded asset file names and asset writes - #103
Open
devin-ai-integration[bot] wants to merge 1 commit into
Open
devin-ai-integration[bot] wants to merge 1 commit into
devin-ai-integration[bot] wants to merge 1 commit into
Conversation
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
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
Admin asset uploads used the client supplied multipart filename verbatim:
StaticAssetServiceImpl.createStaticAssetFromFileonly replaced the configured invalid chars (+,*,%), so/,\and..survived intobuildAssetURL→StaticAsset.fullUrl→ the FILESYSTEM branch ofStaticAssetStorageServiceImpl.createStaticAssetStorage, which normalizedworkArea + fullUrland wrote it with a rawFileOutputStreambefore any containment check. An authenticated admin could uploadfilename="../../../../opt/app/webapps/ROOT/x.jsp"and write arbitrary bytes anywhere the server process can reach (RCE via a dropped webshell).The url is now assembled from sanitized segments only, and the filesystem writer independently refuses to write outside the work area.
StaticAssetServiceImplstripPathInformation— strips control characters, reduces toFilenameUtils.getName, rejects bare./..; applied to the upload filename innormalizeFileExtension,createStaticAssetFromFileandcreateStaticAsset.sanitizePathSegmentonentityType/entityId,sanitizeAssetPathon the optional multi-segmentfileNameproperty (drops empty/.segments, rejects..).validateAssetURL— final assertion that the assembled url is an already-normalized absolute path.StaticAssetStorageServiceImplvalidateFileSystemAssetUrl(fullUrl)— rejects any url that is not already normalized (covers..\too, viaseparatorsToUnix).getDestinationFile(workArea, fullUrl)— replaces the ad-hocnew File(FilenameUtils.normalize(...)); rejects..segments and requiresdestFile.getCanonicalPath()to start with the work-area canonical path + separator, so symlinked directories inside the work area cannot be used to escape either. Legit urls keep their directory structure (the read path hashes the same url).common.propertiesdisabled.file.extensionsnow also denies active-content/executable extensions (jsp,jspx,html,svg,php,asp,js,jar,class,swf, …) instead of onlypdf.allowed.file.extensionsremains available as an allow-list override.Verification
Maven Central and the Broadleaf nexus are blocked in this environment, so the module cannot be built here. The new methods were extracted verbatim into standalone harnesses and executed against the real commons-io / commons-lang3 jars:
fileName, entity segment traversal)./product/../../../../opt/tomcat/webapps/ROOT/shell.jsp,\..\..\shell.jsp,/link/shell.jsp(symlink), empty and/urls are all rejected.JUnit tests added in
StaticAssetServiceImplTestand the newStaticAssetStorageServiceImplTestcover the same cases.Session: https://app.devin.ai/sessions/dac94c434ef9456fa6da0de2e359f39a
Devin-Org: engineering
Devin Review