Bug 1447080 (CVE-2018-5174)

Security: SEE_MASK_FLAG_NO_UI behavior changes in Windows 10, allowing SmartScreen bypass

VERIFIED FIXED in Firefox -esr52

Status

()

defect
P1
normal
VERIFIED FIXED
Last year
5 months ago

People

(Reporter: jafeher, Assigned: jimm)

Tracking

({csectype-other, sec-moderate})

59 Branch
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr5260+ fixed, firefox59 wontfix, firefox60+ fixed, firefox61+ fixed)

Details

(Whiteboard: [adv-main60+][adv-esr52.8+])

User Story

Steps to reproduce:

Download and run these files:
Known malware: https://demo.smartscreen.msft.net/download/known/knownmalicious.exe
Unknown program: https://demo.smartscreen.msft.net/download/unknown/freevideo.exe
Offline: Any of the above with internet on the machine off

More details about the issue:
In the next Windows Release (Redstone 4), Windows Defender SmartScreen will honor the SEE_MASK_FLAG_NO_UI flag. This flag is passed through shellexecute when a user runs an executable, or other supported file, through the firefox download manager. Edge/Windows Explorer do not pass this flag and are unaffected.

There are three problematic scenarios with this flag set:

1. Known malware will be blocked silently: Windows will not show any SmartScreen UI for these types of files, and the user will have no opportunity to override the block through Firefox. The user will not have any indication that executing the file did anything. (Current behavipo
2. Unknown file downloads will be allowed - This is a large number of files and all will be allowed. There is a chance for polymorphic malware to be bucketed within these types of files.
3. User is offline - User is unable to contact SmartScreen. All executions and file opens allowed.

SEE_MASK_FLAG_NO_UI *not* set: all 3 cases will result in a block experience for the user.

SEE_MASK_FLAG_NO_UI *is* set (current Firefox behavior): files in scenario #2 and #3 will be allowed to run even if SmartScreen would otherwise have blocked them.

Attachments

(4 attachments)

Comment hidden (obsolete)

Comment 1

Last year
As best I can tell this was added in bug 394974 for Firefox 2.0 (hi, 2006 called, it wants its flags back, etc.).

Jim, can we just drop this flag? Is there an alternative we should be using? It's not clear from bug 394974 why this flag was added (though some of the others are more obvious).

I expect we'll want to have a fix in place for 60 given the release timelines for the spring updates in the past (april last year for that one). I don't know how we feel about 59 for this.
Group: firefox-core-security → core-security
Component: Untriaged → Widget: Win32
Flags: needinfo?(jmathies)
Product: Firefox → Core
Assignee

Comment 2

Last year
(In reply to :Gijs from comment #1)
> As best I can tell this was added in bug 394974 for Firefox 2.0 (hi, 2006
> called, it wants its flags back, etc.).
> 
> Jim, can we just drop this flag? Is there an alternative we should be using?
> It's not clear from bug 394974 why this flag was added (though some of the
> others are more obvious).
> 
> I expect we'll want to have a fix in place for 60 given the release
> timelines for the spring updates in the past (april last year for that one).
> I don't know how we feel about 59 for this.

I think I just added that because it seemed like a good flag to pass. I don't remember any other specifics.
Flags: needinfo?(jmathies)
Assignee

Comment 3

Last year
Who is jkf49@cornell.edu ? This report sounds like it was filed by a Microsoft employee, but I don't recognize the account.
Flags: needinfo?(dveditz)
I have no idea, but removing the flag and showing errors seems like a good thing regardless.

I don't know why this is a hidden security bug or described as a "bypass". From the description it sounds like things that SmartScreen would block are blocked, just silently, and additional things SmartScreen wouldn't block (offline) are also blocked -- perhaps a bug but not going to cause any security issues.
Flags: needinfo?(dveditz) → needinfo?(jkf49)
Reporter

Comment 5

Last year
Hey, I'm a dev on the smartscreen team. Sorry, it looks like I didn't update the "actual results" properly.

The security issue is the bucket of unknowns and offline.

The unknown bucket has the potential to contain things like polymorphic malware and will be allowed

Offline: running files offline will also be allowed. These could be files that would normally be blocked by smartscreen.
Flags: needinfo?(jkf49)
Dan, should we open this?
Flags: needinfo?(dveditz)
jafeher: I wrote a "User Story" to replace your original description that hopefully removes the confusion. Did I get it right?

(meanwhile I'll set a security rating assuming it is)
User Story: (updated)
Flags: needinfo?(dveditz) → needinfo?(jafeher)
Jim: we'd like this fixed in 60 so it's not too behind the Windows release. Who owns this code now?
Flags: needinfo?(jmathies)
Assignee

Updated

Last year
Assignee: nobody → jmathies
Flags: needinfo?(jmathies)
Assignee

Updated

Last year
Priority: -- → P1
Assignee

Comment 10

Last year
Posted patch patch v.1Splinter Review
Assignee

Comment 11

Last year
Comment on attachment 8962342 [details] [diff] [review]
patch v.1

David mind looking this one line change over? I did a little research on this flag, didn't find anything out on the net indicating this change might introduce some sort of issue. Generally looks like a safe change although I do wonder if users will start seeing error messages when executing attachments due to this. If so, hopefully those messages are useful. Not sure we want to push this out fast, I think lots of bake time is warranted here.
Attachment #8962342 - Flags: review?(davidp99)
Group: core-security → layout-core-security
Assignee

Comment 12

Last year
Comment on attachment 8962342 [details] [diff] [review]
patch v.1

Putting this on hold after discussing with David. We're not sure we want to make these changes. Blocking #2 in particular sounds like it would produce really awful user experiences. #3 is questionable as well.
Attachment #8962342 - Flags: review?(davidp99)
Reporter

Comment 13

Last year
User story looks correct.

@Jim This is how SmartScreen has functioned previously. Chrome has agreed to making this same change. Any commonly downloaded file quickly get reputation in smartscreen and will be allowed. Clicking through SmartScreen once will clear mark of the web and that file will not be blocked again.

Let me know if you require any more information.
Flags: needinfo?(jafeher)
Assignee

Comment 14

Last year
SmartScreen description off wikipedia - 

"It is designed to help protect users against attacks that utilize social engineering and drive-by downloads to infect a system by scanning URLs accessed by a user against a blacklist of websites containing known threats."

https://en.wikipedia.org/wiki/Microsoft_SmartScreen
Assignee

Comment 15

Last year
Hey Romain, can Product provide some input here? Sounds like we probably want to ship this flag but there's an open question as to how annoying it might be for the user. We could tie this to a hidden pref if we think users might want a way to disable. Although I'd prefer not adding that unless we absolutely need it.
Flags: needinfo?(rtestard)
Assignee

Comment 16

Last year
Assignee

Updated

Last year
User Story: (updated)
(In reply to Jim Mathies [:jimm] from comment #15)
> Hey Romain, can Product provide some input here? Sounds like we probably
> want to ship this flag but there's an open question as to how annoying it
> might be for the user. We could tie this to a hidden pref if we think users
> might want a way to disable. Although I'd prefer not adding that unless we
> absolutely need it.

My understanding is that this will only impact Win10 users updating to Redstone 4 (Released Apr 10th it seems).
I tested the files provided in the user story (I run Redstone 3 with Firefox 59.0.2) and both get me a warning screen (red for the known malware and blue for the unknown program) when I attempt to run them (see attached). Can you clarify what is the actual Win10 user experience if SEE_MASK_FLAG_NO_UI is *not* set with Redstone 4 ?

Also do we have telemetry for how often we set that flag on Win10?
Flags: needinfo?(rtestard)
knownmalicious.exe on win10
FWIW, I'd add that the "unknown program" case Romain speaks of (the freevideo.exe one) requires the user to click "More Information" to get to a prompt that allows them to run the program anyway.  Since Firefox is my browser, I very rarely see this prompt so it trips me up every time.  It takes me a moment to figure out what to do and I already know what to do.
Assignee

Comment 21

Last year
(In reply to Romain Testard [:RT] from comment #17)
> (In reply to Jim Mathies [:jimm] from comment #15)
> > Hey Romain, can Product provide some input here? Sounds like we probably
> > want to ship this flag but there's an open question as to how annoying it
> > might be for the user. We could tie this to a hidden pref if we think users
> > might want a way to disable. Although I'd prefer not adding that unless we
> > absolutely need it.
> 
> My understanding is that this will only impact Win10 users updating to
> Redstone 4 (Released Apr 10th it seems).

Correct.

> I tested the files provided in the user story (I run Redstone 3 with Firefox
> 59.0.2) and both get me a warning screen (red for the known malware and blue
> for the unknown program) when I attempt to run them (see attached). Can you
> clarify what is the actual Win10 user experience if SEE_MASK_FLAG_NO_UI is
> *not* set with Redstone 4 ?

According to Microsoft the second two test cases would result in execution without a prompt starting in redstone 4. We still get a block in #1, but apparently we won't get the Windows warning dialog, it'll just fail silently.

> Also do we have telemetry for how often we set that flag on Win10?

We pass this to the API call that launches 3rd party apps for us, so we always pass it. The question is do we want to stop doing that.

The more I dig into this, the more I'm convinced we should take this change.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(rtestard)
Thanks. Talking about experience change between current and Redstone 4 with flag removed my understanding is it will be:
1 Known malware: currently the user sees the red screen, in Redstone 4 with flag removed the user would just see nothing (silent fail. This sounds fine to me if the security risk warrants preventing the user from running the file
2 Unknown program: currently the user sees the blue screen, in Redstone 4 with flag removed the user would still see the blue screen (no change). (I understand from the user story that there is a block in this case and I assume the blue screen as UI)

It sounds to me like we should implement the change since the user experience is only impacted in (1) in a case where known malware would now be prevented from running.

My question above on telemetry was to understand how often the average user sees the screen in scenario (2) although if my interpretation is right this will not change assuming we remove the flag so no need to look into this.
Flags: needinfo?(rtestard)
Assignee

Comment 23

Last year
(In reply to Romain Testard [:RT] from comment #22)
> Thanks. Talking about experience change between current and Redstone 4 with
> flag removed my understanding is it will be:
> 1 Known malware: currently the user sees the red screen, in Redstone 4 with
> flag removed the user would just see nothing (silent fail. This sounds fine
> to me if the security risk warrants preventing the user from running the file

This is incorrect. Currently Firefox will trigger a blocked execution prompt in pre-redstone4 versions of Windows. In redstone4, (according to our ms friend here) current Firefox will fail silently. Landing the patch here (removing the "don't show ui" flag) will not impact Firefox behavior on pre-redtone4, with redstone4 landing the patch will trigger a blocked execution prompt.

> 2 Unknown program: currently the user sees the blue screen, in Redstone 4
> with flag removed the user would still see the blue screen (no change). (I
> understand from the user story that there is a block in this case and I
> assume the blue screen as UI)

Reversed again, but generally yes we want to fix the issue where the blue screen might not display. Landing this patch will accomplish this.

> It sounds to me like we should implement the change since the user
> experience is only impacted in (1) in a case where known malware would now
> be prevented from running.

Ok, we're going land this such that on redstone 4 and beyond, our users get prompted with warnings in all three cases here. Without this patch, those warnings would not be displayed (as they currently are in pre-redstone4 releases of Windows.)
Assignee

Comment 24

Last year
Comment on attachment 8962342 [details] [diff] [review]
patch v.1

We've decided to take this.
Attachment #8962342 - Flags: review?(davidp99)
Comment on attachment 8962342 [details] [diff] [review]
patch v.1

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

Agreed.  Its too valuable to go without.
Attachment #8962342 - Flags: review?(davidp99) → review+
Assignee

Updated

Last year
Assignee

Comment 26

Last year
Comment on attachment 8962342 [details] [diff] [review]
patch v.1

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
This is not a vulnerability bug. Flag change that impacts operating system security response to 3rd party content handling via Firefox. 
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?
all
If not all supported branches, which bug introduced the flaw?
bug 394974
Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
no, and this is not needed. we're going to fix this in current release, and uplift to 60 ESR.
How likely is this patch to cause regressions; how much testing does it need?
medium?
Attachment #8962342 - Flags: sec-approval?
Comment on attachment 8962342 [details] [diff] [review]
patch v.1

As a sec-moderate, this doesn't need sec-approval to land. Let's get it onto trunk and then see about uplifting it to Beta. That will need to happen within the next two weeks so we have a couple of beta left before we ship 60.
Attachment #8962342 - Flags: sec-approval?
Assignee

Updated

Last year
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/56bfd2178dca
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Seems worth a Beta uplift request at least. What are your thoughts on ESR52 (EOL in late August)?
Flags: needinfo?(jmathies)
Assignee

Comment 31

Last year
(In reply to Ryan VanderMeulen [:RyanVM] from comment #30)
> Seems worth a Beta uplift request at least. What are your thoughts on ESR52
> (EOL in late August)?

Once we've verified this is 'ok', this patch can ship to pretty much any release we have right now.
Flags: needinfo?(jmathies)
Assignee

Comment 32

Last year
Comment on attachment 8962342 [details] [diff] [review]
patch v.1

Approval Request Comment
[Feature/Bug causing the regression]:
A newer version of Windows will ship feature changes that are impacted by this flag.
[User impact if declined]:
No warning UI when users interact with untrusted files. 
[Is this code covered by automated tests?]:
yes.
[Has the fix been verified in Nightly?]:
No, we probably should ask for pi on the affected windows version. I will file a pi request.
[Needs manual test from QE? If yes, steps to reproduce]:
see user story 
[List of other uplifts needed for the feature/fix]:
none
[Is the change risky?]:
no
[Why is the change risky/not risky?]:
well understood side effects.
[String changes made/needed]:
none.
Attachment #8962342 - Flags: approval-mozilla-beta?
Group: layout-core-security → core-security-release
Flags: qe-verify+
Comment on attachment 8962342 [details] [diff] [review]
patch v.1

Approved for 60.0b13. Per Jim's PI request, let's get some extra QA attention on it as well.
Attachment #8962342 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Assignee

Comment 35

Last year
Hey jafeher, We're not seeing the prompting in Windows version 1803 (OS Build 17133.73) with our updated version of Firefox. Just want to confirm that the new behavior you describe is in the Windows build above.
Flags: needinfo?(jafeher)
Reporter

Comment 36

Last year
Yes this behavior should exist in that build.

Is this change available in any public beta version of firefox that I can test?
Flags: needinfo?(jafeher) → needinfo?(jmathies)
Yes, 60.0b13 was released today with this fix included.
https://www.mozilla.org/en-US/firefox/channel/desktop/
Assignee

Updated

Last year
Flags: needinfo?(jmathies)
Assignee

Updated

Last year
Flags: needinfo?(jafeher)
Reporter

Comment 38

Last year
Thanks for the information.

We did not realize that Firefox/Chrome call ShellexecuteExW in different ways. There is also a Windows bug in the path that firefox calls. We are working on a fix and will get this out in an early servicing patch.

Let me know if you require any other information.
Flags: needinfo?(jafeher)
Whiteboard: [adv-main60+]
This seems to have missed the ESR52 train as well despite being tracking for 60+ and affected.
Flags: needinfo?(lhenry)
Can you create a patch for esr52 and request uplift? Thanks.
Flags: needinfo?(lhenry) → needinfo?(jmathies)
Assignee

Comment 41

Last year
(In reply to Jimmy from comment #38)
> Thanks for the information.
> 
> We did not realize that Firefox/Chrome call ShellexecuteExW in different
> ways. There is also a Windows bug in the path that firefox calls. We are
> working on a fix and will get this out in an early servicing patch.
> 
> Let me know if you require any other information.

Thanks for the follow up. Is there something we can change on our side? The code in question is here [1]. (In reply to Liz Henry (:lizzard) (needinfo? me) from comment #40)
> Can you create a patch for esr52 and request uplift? Thanks.

The existing patch should apply cleanly there, it's a one line change. I'll flag it.
Flags: needinfo?(jmathies)
Assignee

Comment 42

Last year
Comment on attachment 8962342 [details] [diff] [review]
patch v.1

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
New security related behavioral change in Windows 10 needs to be enabled on our end through a minor code change.
User impact if declined: 
Issues with silent failures when handling file downloads under the newest version of windows 10. 
Fix Landed on Version:
 60, 61.
Risk to taking this patch (and alternatives if risky): 
Low risk, well understood change requested by Microsoft.
String or UUID changes made by this patch: 
none.
Attachment #8962342 - Flags: approval-mozilla-esr52?
Comment on attachment 8962342 [details] [diff] [review]
patch v.1

While this is a fix for a sec-moderate issue I think it warrants uplift to esr52 because it may be a widespread issue with the upcoming Windows update.
Attachment #8962342 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Whiteboard: [adv-main60+] → [adv-main60+][adv-esr52.8+]
Alias: CVE-2018-5174
Hi Jim,

I have retested this issue (followed the steps from comment 0) on Windows 10 x64 Version 1803 (OS Build 17134.5), and the prompts are still not displayed, on 52.8.1 Esr, 60.0.2 Release and 61.0b14 Beta. 

It seems that Edge/Chrome blocks properly both files mentioned in comment 0, when running them.

Any idea what might be wrong here, can you please take a look?
Flags: needinfo?(jmathies)
Reporter

Comment 46

Last year
This fix required a change on the Windows side as well. It is going through servicing now and will be released at the end of this month. I'm not sure of the exact date.

I have validated that firefox has the same behavior as Edge/Chrome with this update.
Assignee

Comment 47

Last year
(In reply to Jimmy from comment #46)
> This fix required a change on the Windows side as well. It is going through
> servicing now and will be released at the end of this month. I'm not sure of
> the exact date.
> 
> I have validated that firefox has the same behavior as Edge/Chrome with this
> update.

Thanks!
Flags: needinfo?(jmathies)
Group: core-security-release

Closing this bug as verified fixed per comment 46.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.