Spoofing via long runs of spaces in filename download (firefox android)
Categories
(Firefox for Android :: Downloads, defect, P2)
Tracking
()
People
(Reporter: sas.kunz, Assigned: rsainani)
References
Details
(Keywords: csectype-spoof, reporter-external, sec-moderate, Whiteboard: [client-bounty-form] [group4] [fxdroid][adv-main131+])
Attachments
(11 files, 3 obsolete files)
|
566 bytes,
text/html
|
Details | |
|
2.77 MB,
video/mp4
|
Details | |
|
21.69 KB,
image/jpeg
|
Details | |
|
29.20 KB,
image/jpeg
|
Details | |
|
21.12 KB,
image/png
|
Details | |
|
48 bytes,
text/x-phabricator-request
|
tjr
:
sec-approval+
|
Details | Review |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
1.07 MB,
video/mp4
|
Details | |
|
48 bytes,
text/x-phabricator-request
|
phab-bot
:
approval-mozilla-beta+
|
Details | Review |
|
48 bytes,
text/x-phabricator-request
|
phab-bot
:
approval-mozilla-beta+
|
Details | Review |
|
305 bytes,
text/plain
|
Details |
after fixed https://bugzilla.mozilla.org/show_bug.cgi?id=1843032 the special characters have been sanitized to be "" , but i try using space on filename, the space is not sanitized to be "" it lead to spoof
step to produce:
1.open http://103.186.0.20/downloadspoofk.html or downloadspoofk.html
2. click "open" button
impact : victim can be spoofed will think that it is a document file and even though it is an apk file
the special characters have been sanitized to be "" , but i try using space on filename, the space is not sanitized to be "" it lead to spoof
Updated•1 year ago
|
Comment 7•1 year ago
|
||
This is a subset of bug 1843032 so not quite as severe, but enough of the same that calling it "sec-low" would be too little.
We could have converted spaces to underscores as well, but normal people like spaces so I imagine it was intentional not to. But we should coalesce runs of spaces into just a single space character.
BEWARE: there are a ton of "space equivalent" characters -- they ALL have to be treated as the same and removed together. Otherwise you can just repeat this example by alternating space and no-break space, for example. Except it's more than just those -- we need to use class equivalent functions. Java might have some if you can't access the internal Gecko ones through the GeckoView interfaces.
Updated•1 year ago
|
Updated•1 year ago
|
Comment 8•1 year ago
|
||
Tagging Cathy as she is part of the team working on downloads.
Updated•1 year ago
|
Updated•1 year ago
|
Comment 9•1 year ago
|
||
Would trimming a sequence of spaces into one space only be a solution?
Comment 10•1 year ago
•
|
||
I just tried on Chrome, they don't change the file's name, but they add ellipses ("...") in the "middle" of the filename, in the download screen and download notification. This way the file extension is always visible. (see attachment)
I think it might be the best way to handle this problem. What do you think?
Comment 11•1 year ago
|
||
(In reply to Titouan Thibaud [:tthibaud] from comment #10)
Created attachment 9416797 [details]
image.pngI just tried on Chrome, they don't change the file's name, but they add ellipses ("...") in the "middle" of the filename, in the download screen and download notification. This way the file extension is always visible. (see attachment)
I think it might be the best way to handle this problem. What do you think?
Desktop Firefox coalesces the spaces, so the file as-is doesn't reproduce the problem for me. It would be useful to check what happens with filenames that are too long there; I think we've already addressed this in the past, and so I expect you could do whatever we do on desktop?
Comment 12•1 year ago
|
||
That sounds good to me.
I'm not familiar at all with the desktop codebase, do you know where I could find the desktop implementation, for reference, or who I should talk to, to get this information?
Comment 13•1 year ago
|
||
(In reply to Titouan Thibaud [:tthibaud] from comment #12)
That sounds good to me.
I'm not familiar at all with the desktop codebase, do you know where I could find the desktop implementation, for reference, or who I should talk to, to get this information?
Neil Deakin knows the gecko filename sanitization bits the best - it's possible you could make use of them, too.
In terms of the display bits, probably Marco Bonardo or me.
Updated•1 year ago
|
| Assignee | ||
Updated•1 year ago
|
| Assignee | ||
Comment 14•1 year ago
|
||
| Assignee | ||
Comment 15•1 year ago
|
||
This patch fixes the file name containing long continuous spaces, we convert that to a single space. The test for this will be landed as a separate task, the current test updates (that don't point to this problem) are merely about the update in the formatting.
However, for a very long file name, the extension still won't be shown on the Downloads Screen which lists all the downloads a user has done. We will follow up with adding the middle ellipsis when Compose 1.7.0 is released as they have the functionality now added to the UI framework. Should that be a sec bug as well?
Hey Rahul,
We actually already have a bug filed for the ellipsis over in Bug 1910185. Would you be interested in pairing once Compose 1.7.0 is released?
| Assignee | ||
Comment 17•1 year ago
|
||
Great, thanks Nick, sounds good!
| Assignee | ||
Comment 18•1 year ago
|
||
Comment on attachment 9421552 [details]
Bug 1906024 - Format download file names better
Security Approval Request
- How easily could an exploit be constructed based on the patch?: It should be fairly difficult as the test case is missing and the commit message looks like we're updating formatting and actually we are.
- Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
- Which branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?: all
- If not all supported branches, which bug introduced the flaw?: None
- Do you have backports for the affected branches?: No
- If not, how different, hard to create, and risky will they be?: They won't be risky. I think we should land it in 131 and so it goes in 131 beta next week and we can uplift to release, so it'll be in the dot release on September 17.
- How likely is this patch to cause regressions; how much testing does it need?: Less likely. We already have a test case which passes locally and we did manual testing too, but we didn't push that test case yet based on the sec bug landing process.
- Is the patch ready to land after security approval is given?: Yes
- Is Android affected?: Yes
Comment on attachment 9421552 [details]
Bug 1906024 - Format download file names better
Approved to land and request uplift
Comment 20•1 year ago
|
||
Please land this ASAP if you want to target the mid-cycle dot release for uplift.
Comment 21•1 year ago
|
||
Updated•1 year ago
|
Comment 22•1 year ago
|
||
Comment 23•1 year ago
|
||
The patch landed in nightly and beta is affected.
:rsainani, is this bug important enough to require an uplift?
- If yes, please nominate the patch for beta approval.
- If no, please set
status-firefox131towontfix.
For more information, please visit BugBot documentation.
| Assignee | ||
Updated•1 year ago
|
Comment 24•1 year ago
•
|
||
What "brand" of RegExp does Kotlin use? I tried to understand the docs on kotlinlang.org but there were multiple definitions which seemed to depend on what language Kotlin was implemented in.
My worry is that this is likely to miss a wide variety of space-like characters. If \s is like JavaScript then we're good, but if it's like Java—which seems more likely!—then \s doesn't even include no-break space \xA0, let alone any of the Unicode ones. What if you do this same attack using non-ASCII space-like characters? Or alternate them so there aren't "long runs" of any given one?
In a Java-based world I think you have to replace \s with [\h\v]
private fun String.replaceContinuousSpaces(): String {
- val escapedCharactersRegex = "\\s+".toRegex()
+ val escapedCharactersRegex = "[\\h\\v]+".toRegex()
return replace(escapedCharactersRegex, SPACE)
}
Updated•1 year ago
|
| Assignee | ||
Comment 25•1 year ago
|
||
| Assignee | ||
Comment 26•1 year ago
•
|
||
Thanks for catching this. It was indeed not catching the unicode space separators. I've added a patch with the test cases locally. I've tested with both \h\v and \s with \p{Z} and There's quite a bit of overlap so either works with \p{Z}. Let me know your thoughts here or on the patch.
Comment 27•1 year ago
|
||
Comment 28•1 year ago
|
||
Cool, thanks. Java's \s definition is so antiquated I didn't even think to check if there was support for \p{Z}. That's definitely better than my suggestion. The ASCII-only \s must come from a strict view of "backwards compatibility", and if that's the case then the same thinking might apply to \h and \v. They include today's known Unicode spaces, but might not ge t new ones in future Unicode updates. \p{Z} is much more future-proof.
Comment 29•1 year ago
|
||
Updated•1 year ago
|
| Assignee | ||
Updated•1 year ago
|
Comment 30•1 year ago
•
|
||
Verified on the Fenix Nightly 132.0a1 from 9/13 with Realme GT Master Edition (Android 13), and Google Pixel 6 (Android 15).
Updated•1 year ago
|
Comment 31•1 year ago
|
||
Please nominate this for Beta approval when you get a chance.
| Assignee | ||
Comment 32•1 year ago
|
||
Original Revision: https://phabricator.services.mozilla.com/D221771
Updated•1 year ago
|
| Assignee | ||
Comment 33•1 year ago
|
||
Original Revision: https://phabricator.services.mozilla.com/D220559
Updated•1 year ago
|
Comment 34•1 year ago
|
||
beta Uplift Approval Request
- User impact if declined: potential sec bug exploit can occur
- Code covered by automated testing: no
- Fix verified in Nightly: yes
- Needs manual QE test: yes
- Steps to reproduce for manual QE testing: In the bug
- Risk associated with taking this patch: Low
- Explanation of risk level: It improves the download file name formatting so the whole file name can be visible. The code is contained.
- String changes made/needed: No
- Is Android affected?: yes
| Assignee | ||
Comment 35•1 year ago
|
||
Original Revision: https://phabricator.services.mozilla.com/D221771
Updated•1 year ago
|
Comment 36•1 year ago
|
||
beta Uplift Approval Request
- User impact if declined: potential sec bug exploit can occur
- Code covered by automated testing: no
- Fix verified in Nightly: yes
- Needs manual QE test: yes
- Steps to reproduce for manual QE testing: In the bug
- Risk associated with taking this patch: Low
- Explanation of risk level: It improves the download file name formatting so the whole file name can be visible. The code is contained.
- String changes made/needed: No
- Is Android affected?: yes
Updated•1 year ago
|
| Assignee | ||
Comment 37•1 year ago
|
||
Thanks [:RyanVM], just added the uplift request. Since there were 2 patches attached to this, there are two uplifts. D222254 should land before D222259.
Comment 38•1 year ago
|
||
:tjr its a sec-moderate but jic does the sec approval apply to both patches?
Yes; (and it's fine to land the test at the same time)
Updated•1 year ago
|
Updated•1 year ago
|
Comment 40•1 year ago
|
||
| uplift | ||
Updated•1 year ago
|
Comment 41•1 year ago
|
||
Verified as fixed on Beta 131.0b8 with Google Pixel 6 (Android 15), and Oppo Find X3 Lite (Android 11).
| Assignee | ||
Comment 42•1 year ago
|
||
Thanks all! [:tjr] In that case, we can land the tests on Monday with Bug 1918048.
Updated•1 year ago
|
Comment 43•1 year ago
|
||
Updated•1 year ago
|
Updated•9 months ago
|
Description
•