coheigea commented on PR #3572:
URL: https://github.com/apache/cxf/pull/3572#issuecomment-6142238497
Thanks for this. Cutting the filename down to its last segment is the right
idea, but as it stands I don’t think the fix holds up, and the parser is the
wrong place to do it.
1. Dot-only names still get through. stripPath returns whatever comes after
the last / or \, so these values still reach getFilename():
filename="..", filename="a/.." and filename*=UTF-8''foo%2f%2e%2e all come
back as ... So new File(uploadDir, cd.getFilename()) points at the parent
directory.
filename="x/" comes back as "", which resolves to uploadDir itself.
2. The filename* value isn’t cleaned. Only the filename key is cleaned.
getParameter("filename*") and getParameters() still return the decoded
../../etc/passwd, and CXF did that decoding itself.
3. The raw header and the parsed value now disagree. toString() and
Attachment.getHeader("Content-Disposition") still contain the full path. Code
that re-parses the header with another parser is still exposed, so whether an
app is protected depends on which accessor it calls.
4. Behaviour changes for outbound and client code. ContentDisposition is a
general-purpose parser. After this change, new ContentDisposition("attachment;
filename=\"dir/sub/a.txt\"").getFilename() returns a.txt while the header still
says dir/sub/a.txt. On the JAX-RS side, EntityPartProvider and EntityPartUtils
decide a part is a file with !StringUtils.isEmpty(getFilename()), so
filename="reports/" now quietly turns into a plain form field.
5. It duplicates an existing helper. The new private stripPath is a copy of
org.apache.cxf.helpers.FileUtils.stripPath, which
AttachmentUtil.createAttachment already uses. Please reuse that one.
6. Other risky characters. NUL (shell.jsp%00.png) and : (C:evil.exe, which
Windows can treat as a drive-relative path) also pass through.
Suggested approach:
Harden FileUtils.stripPath, or add a sibling helper such as
FileUtils.safeFileName, that also rejects or neutralises ., .., empty strings,
NUL and :.
Apply it where inbound attachments are consumed (AttachmentUtil and the
JAX-RS multipart / EntityPart code) rather than inside ContentDisposition. Then
toString() and outbound behaviour stay as they are.
Add tests for .., a trailing separator, the filename* key, NUL and the
&#NNNN; encoding (for example ../x).
--
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]
