Closed Bug 1815062 (CVE-2023-29542) Opened 2 years ago Closed 2 years ago

url/local/lnk/scf .download filename manipulation can be bypassed by using final newline or other sanitized characters

Categories

(Firefox :: File Handling, defect, P1)

Product:
Firefox
Component:
File Handling
Platform:
Desktop
Windows
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
112 Branch
Tracking Flags:
Tracking Status
firefox-esr102 112+ verified
firefox110 --- wontfix
firefox111 --- wontfix
firefox112 + verified

People

(Reporter: fazim.pentester, Assigned: enndeakin)

References

Details

(Keywords: reporter-external, sec-moderate, Whiteboard: [reporter-external] [client-bounty-form] [verif?][post-critsmash-triage][adv-main112+][adv-esr102.10+])

Attachments

(7 files, 1 obsolete file)

poc.html
1.66 KB, text/html
Details
solution.txt
1.29 KB, text/plain
Details
Bug 1815062, move filename and extension validity checks into SanitizeFileName so that they get called after sanitization is complete, r=gijs
48 bytes, text/x-phabricator-request
Details | Review
Bug 1815062, modify callers of DownloadPaths.sanitize so that some use the allowInvalidFilenames argument, r=gijs
48 bytes, text/x-phabricator-request
Details | Review
WIP: Bug 1815062, part 1, esr version
48 bytes, text/x-phabricator-request
RyanVM
: approval-mozilla-esr102+
Details | Review
WIP: Bug 1815062, part 2, esr version
48 bytes, text/x-phabricator-request
RyanVM
: approval-mozilla-esr102+
Details | Review
advisory.txt
380 bytes, text/plain
Details
Attached file poc.html —

Firefox has implemented a security measure aimed at protecting its users by replacing the download of malicious shortcut extensions, such as .url, .lnk, .scf, .local, and others, with the ".download" extension. However, it has been discovered that an attackers can use special character to bypass this filter.

A similar issue has been previously reported by me in Mozilla's bug tracking system (https://bugzilla.mozilla.org/show_bug.cgi?id=1810143) and has recently been addressed in the latest beta version of Firefox (110.0).

After conducting thorough testing on the latest version of Firefox, it has been determined that all dangerous extensions can still be downloaded using this bypass, indicating a lack of a filter to prevent this vulnerability.

I have provided a proof of concept (poc.html) file that downloads .url. Using the special character '\n' bypass, you can directly modify the fileName variable to test other dangerous extensions, including .scf, .lnk, and .local.

Note: This vulnerability is of a more severe nature, given the ability of not only .url extensions, but also .scf, .lnk, and .local extensions to be downloaded as well.

Solution: Block special characters.

Flags: sec-bounty?

This is a video demonstration that is testing the previously mentioned dangerous file extensions: https://youtu.be/_GdMxVxcEKE (YouTube Unlisted)

Note: It is also possible to bypass the DLL download prompt using this method, I am uncertain if this constitutes a problem.

This vulnerability affects the latest version of Firefox Stable, and the method can be used to bypass the recently added .url and .scf filters, as demonstrated in testing on the Firefox beta.

This method can be further exploited using the following techniques:
.lnk/.local: https://bugzilla.mozilla.org/show_bug.cgi?id=1773894
.url: https://bugzilla.mozilla.org/show_bug.cgi?id=1810143
.scf: https://bugzilla.mozilla.org/show_bug.cgi?id=1812354

Original code (from https://hg.mozilla.org/mozilla-central/rev/ab7d32a004aa ):
if (StringEndsWith(fileName, u".lnk"_ns, nsCaseInsensitiveStringComparator) || StringEndsWith(fileName, u".local"_ns, nsCaseInsensitiveStringComparator) || StringEndsWith(fileName, u".url"_ns, nsCaseInsensitiveStringComparator) || StringEndsWith(fileName, u".scf"_ns, nsCaseInsensitiveStringComparator)) { fileName.AppendLiteral(".download"); }
Revised code (Solution):
`#include <string>
#include <algorithm>
#include <cctype>

bool isSpecialCharacter(char c) {
return !std::isalnum(c);
}

void replaceSpecialCharacters(std::string& str) {
std::replace_if(str.begin(), str.end(), isSpecialCharacter, '_');
}

if (StringEndsWith(fileName, u".lnk"_ns, nsCaseInsensitiveStringComparator) ||
StringEndsWith(fileName, u".local"_ns, nsCaseInsensitiveStringComparator) ||
StringEndsWith(fileName, u".url"_ns, nsCaseInsensitiveStringComparator) ||
StringEndsWith(fileName, u".scf"_ns, nsCaseInsensitiveStringComparator)) {

replaceSpecialCharacters(fileName);

fileName.AppendLiteral(".download");
}
`

Solution: The revised code checks if the file name contains any special characters, and blocks them by replacing these characters with an underscore.

Attached file solution.txt —

Comment on attachment 9316055 [details]
solution.txt

This attachment isn't patch-formatted. It also wouldn't work because we can't use std, and the solution posted is not unicode-compatible (ie if the C locale was en-US, you couldn't have any non-ascii filenames), and because we already do sanitization and we should probably just make sure that the filename checks get run after the sanitization, rather than before.

Attachment #9316055 - Attachment is patch: false

Neil, can you take a look?

Flags: needinfo?(enndeakin)
Component: Security → File Handling
Summary: Bypass all Extension filter using special character → url/local/lnk/scf .download filename manipulation can be bypassed by using final newline character

(In reply to :Gijs (he/him) from comment #7)

Comment on attachment 9316055 [details]
solution.txt

This attachment isn't patch-formatted. It also wouldn't work because we can't use std, and the solution posted is not unicode-compatible (ie if the C locale was en-US, you couldn't have any non-ascii filenames), and because we already do sanitization and we should probably just make sure that the filename checks get run after the sanitization, rather than before.

I apologize, I am unfamiliar with how the patch system works (Can you provide a documentation on this so I can learn how things work? Thank you.)

Yes, we could use the sanitation prior to checking the filename.

However, if the user intends to perform unusual actions, such as incorporating \u unicodes in their filename, I have considered incorporating the sanitation within the dangerous file check, while leaving other non-threatening file extensions open for user creativity.

I believe my code also removes unicode characters, as an attacker could use unicode instead of a character, for example, "test.ln\u006B"

So my solution allows for the use of unicode and other unusual characters in non-threatening file extensions, while filtering them out if they are used in dangerous extensions, providing a proper resolution.

This is why I thought my title should be "Special Character" and not just "Newline".

What is your opinion regarding the fact that the DLL is skipping the prompt, I am uncertain if this constitutes a problem.

(In reply to Shaheen Fazim from comment #9)

I apologize, I am unfamiliar with how the patch system works (Can you provide a documentation on this so I can learn how things work? Thank you.)

https://firefox-source-docs.mozilla.org/contributing/contribution_quickref.html has details on a bunch of this.

However, if the user intends to perform unusual actions, such as incorporating \u unicodes in their filename, I have considered incorporating the sanitation within the dangerous file check, while leaving other non-threatening file extensions open for user creativity.

This doesn't really make sense. The existing code checks if the file name ends with .lnk (or any of the other extensions). If there is random stuff on the end of the filename, that check will fail, because the filename ends in .lnk plus the extra stuff, so we never enter the if condition in the first place.

To fix this, the code would need to do sanitization before checking the end of the filename.

I think this means the if condition just needs to move after the call to SanitizeFileName. Neil, does that sound right?

(In reply to Shaheen Fazim from comment #2)

This is a video demonstration that is testing the previously mentioned dangerous file extensions: https://youtu.be/_GdMxVxcEKE (YouTube Unlisted)

Note: It is also possible to bypass the DLL download prompt using this method, I am uncertain if this constitutes a problem.

The prompt in your video is the "what do you want to do with this file" dialog. I don't think it will generally show up for DLLs in a clean Firefox profile anyway. It's not normally there as a security feature; that happens when you try to open a dangerous file, when Firefox will tell you the file is executable and ask if you're sure you want to continue. (Yes, this is different from Chrome where there is a warning prompt that happens when you download, not when you open.)

Thank you for the information.

Summary: url/local/lnk/scf .download filename manipulation can be bypassed by using final newline character → url/local/lnk/scf .download filename manipulation can be bypassed by using final newline or other sanitized characters
Duplicate of this bug: 1815204

Is Haxatron1 an external reporter or another assignee? I am confused because I have seen Haxatron1 on other bugs as well.

(In reply to :Gijs (he/him) from comment #13)

I think this means the if condition just needs to move after the call to SanitizeFileName. Neil, does that sound right?

Yes, I think it could be moved afterwards. Perhaps though it should be placed inside at the end SanitizeFilename instead. This would handle the case where VALIDATE_SANITIZE_ONLY was passed. A brief inspection suggests that some callers should be checking for these sorts of invalid filenames. However, my understanding is that callers from the extension api should not be modifying the filename.

Flags: needinfo?(enndeakin)

(In reply to Neil Deakin from comment #18)

(In reply to :Gijs (he/him) from comment #13)

I think this means the if condition just needs to move after the call to SanitizeFileName. Neil, does that sound right?

Yes, I think it could be moved afterwards. Perhaps though it should be placed inside at the end SanitizeFilename instead. This would handle the case where VALIDATE_SANITIZE_ONLY was passed. A brief inspection suggests that some callers should be checking for these sorts of invalid filenames.

Makes sense to me. Are you able to pick up this bug? :-)

Flags: needinfo?(enndeakin)
Assignee: nobody → enndeakin
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

Depends on D169124

Blocks: 1810793

This bug and bug 1815204 were filed within the "collision window" specified in our bug bounty program. We are awarding a bounty split between the two reporters.

Flags: sec-bounty? → sec-bounty+

Thank you for the bounty. I appreciate the fact that the Mozilla team is quick to handle reports.

This would be S3 but the fact that we've published an advisory and the fix can be bypassed so easily means I'm marking S2. We should try to get this fix into 111 if possible. Neil, let me know if there's anything I can do to help!

Severity: -- → S2
OS: Unspecified → Windows
Priority: -- → P1
Hardware: Unspecified → Desktop

Callers that use the filename to assign the default filename in a filepicker or save without a filepicker should use the default arguments, callers that validate a filename entered from a filepicker should supply false for compressWhitespaces, and true for allowInvalidFilenames

Depends on D169124

Flags: needinfo?(enndeakin)

https://hg.mozilla.org/mozilla-central/rev/642008166aac
https://hg.mozilla.org/mozilla-central/rev/bce007ba1a3f

Is this something we want/need on ESR also? If so, it'll need rebasing.

Group: firefox-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
status-firefox110: --- → wontfix
status-firefox111: --- → affected
status-firefox112: --- → fixed
status-firefox-esr102: --- → ?
tracking-firefox111: --- → +
tracking-firefox112: --- → +
tracking-firefox-esr102: --- → ?
Flags: needinfo?(enndeakin)
Resolution: --- → FIXED
Target Milestone: --- → 112 Branch

(In reply to Ryan VanderMeulen [:RyanVM] from comment #26)

Is this something we want/need on ESR also? If so, it'll need rebasing.

Given bug 1809923 was uplifted to ESR, I suppose we should uplift this too.

Neil, can we get a beta uplfit request on this? Also ESR - see comment 26 and comment 27

Regressions: 1819760
Flags: qe-verify+
Whiteboard: [reporter-external] [client-bounty-form] [verif?] → [reporter-external] [client-bounty-form] [verif?][post-critsmash-triage]

I was able to reproduce the issue on Firefox 111.0a1 (2023-02-04) on Windows 11 by using the informations provided in Comment 0 and the demonstration on Comment 2 where the ".download" was not used when editing the poc with "\n" (tried with url, lnk, local, scf and dll).

The issue is fixed on Firefox 112.0a1 (2023-03-07) on the same system and all of them have the ".download" added. Dll downloads seems to be behave like before as they are directly downloaded as they are.

Status: RESOLVED → VERIFIED
status-firefox112: fixed → verified
Flags: qe-verify+
Blocks: 1824468

Depends on D174190

A patch has been attached on this bug, which was already closed. Filing a separate bug will ensure better tracking. If this was not by mistake and further action is needed, please alert the appropriate party. (Or: if the patch doesn't change behavior -- e.g. landing a test case, or fixing a typo -- then feel free to disregard this message)

Comment on attachment 9326148 [details]
WIP: Bug 1815062, part 1, esr version

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: Host can use a specially crafted name and file that, if saved, ends up creating a shortcut on the desktop that can execute code.
  • User impact if declined: security issue for user
  • Fix Landed on Version: 112
  • Risk to taking this patch: Medium
  • Why is the change risky/not risky? (and alternatives if risky): Has various tests in bug 1824468.

Note also that regression bug 1819760 is also needed.

Flags: needinfo?(enndeakin)
Attachment #9326148 - Flags: approval-mozilla-esr102?
Attachment #9316428 - Flags: approval-mozilla-esr102?
Attachment #9316646 - Flags: approval-mozilla-esr102?
Attachment #9319866 - Flags: approval-mozilla-esr102?
Attachment #9326149 - Flags: approval-mozilla-esr102?
Attachment #9319866 - Flags: approval-mozilla-esr102?
Attachment #9316646 - Flags: approval-mozilla-esr102?
Attachment #9316428 - Flags: approval-mozilla-esr102?

Comment on attachment 9326148 [details]
WIP: Bug 1815062, part 1, esr version

Approved for 102.10esr.

Attachment #9326148 - Flags: approval-mozilla-esr102? → approval-mozilla-esr102+
Attachment #9326149 - Flags: approval-mozilla-esr102? → approval-mozilla-esr102+

Verified that the issue is fixed as well with Firefox ESR 102.10.0 on Windows 11 and all have the ".download" added.

status-firefox-esr102: fixed → verified
Whiteboard: [reporter-external] [client-bounty-form] [verif?][post-critsmash-triage] → [reporter-external] [client-bounty-form] [verif?][post-critsmash-triage][adv-main112+]
Whiteboard: [reporter-external] [client-bounty-form] [verif?][post-critsmash-triage][adv-main112+] → [reporter-external] [client-bounty-form] [verif?][post-critsmash-triage][adv-main112+][adv-esr102.10+]
Attached file advisory.txt —
Alias: CVE-2023-29542
Attachment #9316646 - Attachment is obsolete: true
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: