Strip path traversal from uploaded asset file names - #93
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
Uploaded asset file names were only run through the configurable static.asset.invalid.chars.in.filename replacement (+,*,%), so directory separators and .. segments survived into the asset fullUrl and, from there, into the filesystem storage write path. Asset urls are now assembled from sanitized segments only, and the filesystem writer rejects any asset url that is not already normalized.
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.
A Brief Overview
An admin asset upload derived the persisted
StaticAsset.fullUrlstraight from the client-supplied multipart filename. The only normalization wasfileName.replaceAll("[" + notAllowedCharsInFileName + "]", replacementString)(default+,*,%), so/,\and..survived.buildAssetURLthen concatenated the also client-derivedentityType/entityIdand that filename, andStaticAssetStorageServiceImpl.createStaticAssetStorageused the result verbatim to build a filesystem path (asset.use.filesystem.storage=trueis the shipped default):StaticAssetServiceImplnow builds asset urls out of sanitized segments instead of relying on the configurable invalid-char list:stripPathInformation(name)= strip control characters, thenFilenameUtils.getName(...)(handles both separator styles), then reject a bare./... Applied to the uploaded filename innormalizeFileExtension/createStaticAssetFromFileand again increateStaticAsset, so the directcreateStaticAsset(InputStream, fileName, ...)API is covered too.entityType/entityIdgo throughsanitizePathSegment(single segment, no separators).fileNameproperty is documented as a url, so it keeps multiple segments viasanitizeAssetPath: empty/.segments dropped,..rejected.validateAssetURLis a final assertion that the assembled url is a normalized absolute path.StaticAssetStorageServiceImpl.createStaticAssetStorageadditionally rejects any asset url that is not already normalized before it is turned into a filesystem path, so the write sink is safe even for asset records created elsewhere.Anything that reduces to nothing (
../..,.,/) is rejected with anIllegalArgumentExceptionrather than silently renamed. Ordinary uploads are unchanged:img.pngwithentityType=product,entityId=100still yields/product/100/img.png.Additional context
Unit tests added to
StaticAssetServiceImplTest(traversal, Windows separators, absolute paths, NUL byte, traversal-only names, attacker-controlled entity segments, thefileNameproperty) and a newStaticAssetStorageServiceImplTestfor the sink guard.Maven Central and the Broadleaf nexus are not reachable from the environment this was written in, so the module could not be built; the changed files were syntax-checked with
javacand the new logic was additionally executed standalone against commons-io 2.22/2.13 + commons-lang3 with the same payloads the unit tests use (all pass).Add Labels to the right panel: Security, Bug, critical, ready-for-code-review
Devin Review