reject mixed-separator path traversal in imap attachment names - #70665
Conversation
potiuk
left a comment
There was a problem hiding this comment.
Good fix. Tying the check to os.sep meant that on Windows, where / is equally valid as a separator, ../../evil walked straight past the guard — and attachment names come from email, so that input is attacker-controlled whenever the mailbox receives external mail.
The revision addresses the earlier review point properly: using os.sep plus os.altsep keeps backslashes as ordinary characters on POSIX while catching both separators on Windows, and the two parametrized tests lock that platform difference in. ("..\\..\\test1.csv", False) on POSIX is exactly the case worth pinning.
I checked the obvious follow-on question — absolute names like /etc/passwd — and they are covered, though somewhat by accident: _correct_path concatenates strings rather than using os.path.join, so an absolute name becomes /out//etc/passwd and stays inside the directory. Worth knowing that this is load-bearing, since switching that concatenation to os.path.join later would reintroduce an escape that this check does not cover.
Splitting on components also catches a bare .. that the old substring test missed, which is a nice bonus even though it isn't exploitable on its own.
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
Signed-off-by: bibi samina <sam@bugqore.com>
4180097 to
dab673d
Compare
…e#70665) * reject mixed-separator path traversal in imap attachment names * Use os.altsep for separator-agnostic imap traversal check Signed-off-by: bibi samina <sam@bugqore.com> --------- Signed-off-by: bibi samina <sam@bugqore.com>
_is_escaping_current_directoryin the imap hook rejects a traversing attachment name withf"..{os.sep}" in name, which is tied to the host separator. On Windowsos.sepis a backslash but a forward slash is also a valid path separator, so an attachment named../../evilslips past the check and, once joined onto the output directory in_correct_path, resolves outside it, letting whoever sent the mail write an arbitrary file. This normalises both separators and rejects any..path component instead.Was generative AI tooling used to co-author this PR?