1906024 - (CVE-2024-9395) Spoofing via long runs of spaces in filename download (firefox android)

Status: VERIFIED FIXED

(Firefox for Android :: Downloads, defect, P2)

Description

[Hafiizh]

Attachment: video6064628473471701113.mp4

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

Comment 1

[Hafiizh]

Attachment: downloadspoofk.html

Comment 2

[Hafiizh]

Attachment: downloadspoofk.html

Comment 3

[Hafiizh]

Attachment: video6064628473471701121.mp4

Comment 4

[Hafiizh]

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

Comment 5

[Hafiizh]

Attachment: msg183539871-56352.jpg

Comment 6

[Hafiizh]

Attachment: msg183539871-56351.jpg

Comment 7

[Daniel Veditz [:dveditz]]

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.

Comment 8

[Arturo Mejia [:amejia]]

Tagging Cathy as she is part of the team working on downloads.

Comment 9

[Titouan Thibaud [:titouan]]

Would trimming a sequence of spaces into one space only be a solution?

Comment 10

[Titouan Thibaud [:titouan]]

Attachment: image.png

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

[:Gijs (he/him)]

(In reply to Titouan Thibaud [:tthibaud] from comment #10)

Created attachment 9416797 [details]
image.png

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?

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

[Titouan Thibaud [:titouan]]

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

[:Gijs (he/him)]

(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.

Comment 14

[Rahul Sainani [:rsainani]]

Attachment: Bug 1906024 - Format download file names better

Comment 15

[Rahul Sainani [:rsainani]]

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?

Comment 16

[Nicholas Poon [:Nick]]

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?

Comment 17

[Rahul Sainani [:rsainani]]

Great, thanks Nick, sounds good!

Comment 18

[Rahul Sainani [:rsainani]]

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 19

[Tom Ritter [:tjr]]

Comment on attachment 9421552 [details]
Bug 1906024 - Format download file names better

Approved to land and request uplift

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Please land this ASAP if you want to target the mid-cycle dot release for uplift.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/115ddede5cd0
Format download file names better r=android-reviewers,tthibaud

Comment 22

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/115ddede5cd0

Comment 23

[BugBot [:suhaib / :marco/ :calixte]]

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-firefox131 to wontfix.

For more information, please visit BugBot documentation.

Comment 24

[Daniel Veditz [:dveditz]]

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)
  }

Comment 25

[Rahul Sainani [:rsainani]]

Attachment: Bug 1906024 - Format download file names

Comment 26

[Rahul Sainani [:rsainani]]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b51d18d1b989
Format download file names r=android-reviewers,tthibaud

Comment 28

[Daniel Veditz [:dveditz]]

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

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/b51d18d1b989

Comment 30

[Mira Lobontiu (Android QA)]

Attachment: Bug 1906024.mp4

Verified on the Fenix Nightly 132.0a1 from 9/13 with Realme GT Master Edition (Android 13), and Google Pixel 6 (Android 15).

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval when you get a chance.

Comment 32

[Rahul Sainani [:rsainani]]

Attachment: Bug 1906024 - Format download file names

Original Revision: https://phabricator.services.mozilla.com/D221771

Comment 33

[Rahul Sainani [:rsainani]]

Attachment: Bug 1906024 - Format download file names better

Original Revision: https://phabricator.services.mozilla.com/D220559

Comment 34

[Phabricator Automation]

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

Comment 35

[Rahul Sainani [:rsainani]]

Attachment: Bug 1906024 - Format download file names

Original Revision: https://phabricator.services.mozilla.com/D221771

Comment 36

[Phabricator Automation]

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

Comment 37

[Rahul Sainani [:rsainani]]

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

[Dianna Smith [:diannaS]]

:tjr its a sec-moderate but jic does the sec approval apply to both patches?

Comment 39

[Tom Ritter [:tjr]]

Yes; (and it's fine to land the test at the same time)

Comment 40

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/6defd36ecb43
https://hg.mozilla.org/releases/mozilla-beta/rev/724dca00a9dd

Comment 41

[Mira Lobontiu (Android QA)]

Verified as fixed on Beta 131.0b8 with Google Pixel 6 (Android 15), and Oppo Find X3 Lite (Android 11).

Comment 42

[Rahul Sainani [:rsainani]]

Thanks all! [:tjr] In that case, we can land the tests on Monday with Bug 1918048.

Comment 43

[Jason Kratzer [:jkratzer]]

Attachment: advisory.txt