890853 - (CVE-2013-1726) MAR signature bypass in Updater could lead to downgrade

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Seb Patane (he/him)]

Attachment: Proof of concept source + binary

The updater doesn't write-lock the user-supplied MAR file while it is using it. This leads to a race condition between the MAR file's signatures being validated and the actual access of data within the file.

If an attacker can change the file after the MAR signatures are validated, but before data is read, the attacker supplied MAR data will be what the updater uses. The proof-of-concept uses this to replace the version string which allows us to downgrade Firefox to a vulnerable version (at which point an attacker could use a previous vulnerability such as #888314 to execute arbitrary code).

I'd imagine  with further investigation into the MAR file format an attacker could feasibly have the updater extract and run their own files as part of the update process as well. This all runs as the SYSTEM user.

Proof-of-concept is attached. It expects the current directory to hold an update.original.mar file (tested with http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/20.0/update/win32/en-GB/firefox-20.0.complete.mar), which it will use to downgrade the Firefox installation in c:\Program Files (x86)\Mozilla Firefox\.

As with race conditions, timing can be a little finicky. On my PC (Win7 x64, updater.exe 22.0.0.4917) I find there's a 200ms window between ~250-450ms. Given no arguments, the PoC will start at 200ms and work its way up to 1000ms in 10ms increments until it succeeds.

Comment 1

[Brian R. Bondy [:bbondy]]

We did take special care to make sure the MAR file was opened before verifying its signature and using it, without closing it in between.

Unfortunately it looks like mar_open maps to _wfopen which allows file sharing rights by default. see: 
http://mxr.mozilla.org/mozilla-central/source/modules/libmar/src/mar_read.c#181

We can simply change this to use _wfopen_s and the problem should be fixed.
_wfopen_s doesn't allow any file sharing rights.

i.e. _wfopen uses CreateFile's FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE
_wfopen_s uses none of those flags.

VS2005's SDK's CRT and above supports it so I think we can use it.

Note: the new code that was added which protects replacing updater.exe uses CreateFile directly so doesn't specify the sharing flags.  The mar_open code predates the maintenance service, but we haven't verified signatures until the same time.

Also note that this problem is equally a problem without the maintenance service since we verify signatures on the MAR file without the maintenance service as well. It just landed close to the maintenance service.

Comment 2

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1.

See last comment.

Comment 3

[Brian R. Bondy [:bbondy]]

Comment on attachment 772027 [details] [diff] [review]
Patch v1.

There is no security rating yet so I thought I'd request sec-approval just in case.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
There is one attache to this bug, somewhat easily but you need physical access to the machine.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
I don't think it's very well known that _wfopen_s doesn't allow sharing but _wfopen does, so I don't think it points a bulls-eye on the problem. 

Which older supported branches are affected by this flaw?
All, it's worth noting that old versions of Firefox before the maintenance service didn't even try to protect against spoofed MAR files. So we lived with this in the past. 

If not all supported branches, which bug introduced the flaw?
Bug 699700 but we didn't have MAR verification at all before that.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
The patches should be the same.

How likely is this patch to cause regressions; how much testing does it need?
Low, I'll be pushing to oak first to manually test.

Comment 4

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

Adding release management before sec-approval as we'll need to take this on ESR, where it is more of a concern.

Dan pointed out that the only diff in the patch is a change to a _s version of an api that was created as a _s variant specifically for security reasons but the patch itself is trivial.

Comment 5

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

Comment on attachment 772027 [details] [diff] [review]
Patch v1.

sec-approval+ since release management plussed it for branches.

Comment 7

[Lukas Blakk [:lsblakk] use ?needinfo]

Let's get an uplift nomination here so we can land this before tomorrow's beta and get qa around the updater code.

Comment 10

[Brian R. Bondy [:bbondy]]

This hasn't landed on m-c yet, I'll request uplift once it does, and once it is on there for a couple days. This will not make tomorrow's beta.

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

This should land asap - we have to give QA a chance to test it - so please ensure this is landed to central and nominated for uplift in the next day or so in order to make our second-to-last beta which goes to build Thursday morning.

Comment 12

[Brian R. Bondy [:bbondy]]

I understand the importance and urgency. This will land as soon as it is safe to do so, and as soon as I verify it and its duped PoCs.

Comment 13

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/896cdd92608d

I can't successfully reproduce with the PoC but the reported problem makes sense and the fix is what needs to be done. I'd appreciate help verifying this on mozilla-central after it lands there please Seb Patane.

I couldn't reproduce with the duped bug PoCs either. It's just a hard timing thing to get right I think.

Comment 14

[Brian R. Bondy [:bbondy]]

Comment on attachment 772027 [details] [diff] [review]
Patch v1.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 699700 added mar signing, but before that we didn't even do mar signing.  We never locked the Mar during updates either.
User impact if declined: User that has physical access to a machine, if unelevated can extract files to an elevated location.  They can downgrade firefox.
Testing completed (on m-c, etc.): I tested on oak to make sure updates didn't break.  I couldn't reproduce the PoC problem though so I hope the original reporter can help with that.
Risk to taking this patch (and alternatives if risky): Low
String or IDL/UUID changes made by this patch: None

Comment 15

[Seb Patane (he/him)]

Sure thing Brian, let me know when it makes it into a nightly and I'll check.

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/896cdd92608d

Comment 17

[Alex Keybl [:akeybl]]

Let's wait to take this on any branches until it's confirmed to fix the problem on Nightly, and doesn't cause regressions. This means we may not take a fix until FF24, given this is an update change.

When approved last week we expected a quick landing.

Comment 18

[Brian R. Bondy [:bbondy]]

(In reply to Alex Keybl [:akeybl] from comment #17)
> When approved last week we expected a quick landing.

A patch was provided and r+ed 07-08, but there was no approval given until a week after that.  Had the sec-approval (which was waiting on release management) been given earlier, then this would have landed much earlier. There is a real cost of context switching when waiting a week after a patch is provided and r+ed. Please set expectations to match turnarounds that you can also match. Thanks.

Comment 19

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

Brian, we've been working out some of the kinks between branch approvals and sec-approvals. Things should go more quickly in the future. 

That said, there *was* as much time waiting for checkin as you were waiting for approval and the branch patch did only show up yesterday. We can't take certain things too late in the cycle and the cycle is only six weeks long. We'll be faster in the future but things need to go in fairly quickly when they do get approval.

Comment 20

[Brian R. Bondy [:bbondy]]

(In reply to Al Billings [:abillings] from comment #19)
> Brian, we've been working out some of the kinks between branch approvals and
> sec-approvals. Things should go more quickly in the future. 
> That said, there *was* as much time waiting for checkin as you were waiting
> for approval and the branch patch did only show up yesterday. We can't take
> certain things too late in the cycle and the cycle is only six weeks long.
> We'll be faster in the future 

My complaint was not about the length of time taken, it was about uneven expectations and blame mentioned in Comment 17.

> but things need to go in fairly quickly when they do get approval.

I test updater related changes on oak and run through several manual tests first. This takes extra time compared to normal approvals which can go in with no extra work after approval. I also have a large queue of other things I'm working on. My average check-in time after approval I'm sure is a day.  That being said, the statement is understood and already followed.

Comment 21

[Brian R. Bondy [:bbondy]]

Hi Seb Patane, could you give thi build a try?
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-07-24-03-02-04-mozilla-central/firefox-25.0a1.en-US.win32.installer.exe

Comment 22

[Seb Patane (he/him)]

Hi Brian, the proof of concept fails consistently with the nightly build - the updater returns error code 6 instead of successfully exploiting.

Comment 23

[Brian R. Bondy [:bbondy]]

Thanks Seb!

Comment 24

[bhavana bajaj [:bajaj]]

Approving on aurora given the nightly verification.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5d99a7c9a26a

Comment 26

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

This has been waiting for ESR17 patch approval for a month. Release management, can you give it please?

Comment 27

[bhavana bajaj [:bajaj]]

(In reply to Al Billings [:abillings] from comment #26)
> This has been waiting for ESR17 patch approval for a month. Release
> management, can you give it please?

Approved ! Clearing the needinfo.

Also adding checkin-needed and cc'ing Ryan so this can get landed on the esr17 branch.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr17/rev/be505e812ba0

Comment 29

[Matt Wobensmith [:mwobensmith][:matt:]]

Hi Seb, we'd like to know if two other branches also pass your verification. They should.

ftp://ftp.mozilla.org/pub/mozilla.org/firefox/candidates/17.0.9esr-candidates/build1/win32/
ftp://ftp.mozilla.org/pub/mozilla.org/firefox/candidates/24.0-candidates/build1/win32/

If you have time to check out one or both of the above, we'd really appreciate it.

Thanks.