748948 - Possible race condition when launching updates from service after security checks

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1.

James Forshaw wrote:

> If FF is installed to a non-standard directory does the updater service get 
> installed to a secure directory elsewhere, or does it not get installed at 
> all, or does it get installed along side?


And relating to the patch attached to this post:

> That fix for the race is not quite correct, for at least two reasons. 

> 1) The attacker need not provide a path to a file system which honors the
>  write locking. For example if he pointed it to a UNC path then he could
>  change it on the remote server and Windows *could* return the new data at 
> LoadLibrary time.
> 
> 2) The semantics of LoadLibrary differ from CreateFile when it comes to 
> paths. For example there is no checking done to make the path absolute so 
> if you provide a relative path then LoadLibrary "could" search the path 
> first (an unlikely condition). But probably more importantly, if you supply 
> a filename with no extension then LoadLibrary always appends .DLL to the 
> filename before loading, CreateFile will open the name as is.

Comment 1

[Brian R. Bondy [:bbondy]]

Attachment: Patch without NTFS check

Submitted wrong patch

Comment 2

[Brian R. Bondy [:bbondy]]

Re 1: I'm pretty sure that file locking is honored even with UNC paths. I'll verify this once I get back from the Firefox work week though.

Re 2: What would you recommend here? Since we're loading as a resource dll only now I'm not sure which attack you are worried about here.

Comment 3

[James Forshaw]

For 1) It is honored in the sense that the local file system/OS will honor it if you open through the API, but there is no guarantee that the system you connected to will do the same. For example if the share in on a Linux box there is no reason the attacker couldn't change the file data. And while the cache manager should have some of the file data in memory it gives you no assurances on what it will return you next time from a remote service.

For 2) the attack would be that you could place the magic string resource in the .DLL form of the file, but you actually use the non DLL version as the canonical updater. This effectively makes what ever check you are supposed to be doing moot. I assume the check is there to tie say only special updaters as opposed to any binaries signed my the mozilla private key. Whether than ultimately leads to any EoP condition depends on what happens next. Of course if it doesn't lead to EoP then why do it in the first place?

Comment 4

[Brian R. Bondy [:bbondy]]

Re 1: I think we could add a check to ensure the path is not a mapped drive nor UNC path if that is necessary. 
Re 2: We don't actually execute the file unless the file is signed by Mozilla.

Comment 5

[James Forshaw]

Re 1: but there are ways and means, for example everything is a mapped drive in WinNT effectively, and you couldn't exclude a USB drive for example just because it might be mapped per-user rather than per-machine. It is possible to confuse the Win32 API that a drive letter is not really really a network share. 

Re 2: Sure, but if that is the case what is the issue the initial check protects against? If it is to stop you using any mozilla signed binaries which are "not" the updater then this fix does not prevent that. I can assume that the fix is because you do not necessarily trust just anything moz signed as a valid updater?

Comment 6

[Brian R. Bondy [:bbondy]]

James I spoke with Rob Strong about this and we think we have a viable solution in addition to the attached patch.

If the path passed is on an NTFS drive we will use the service for the update.
If the path passed is not on an NTFS drive, we will not even attempt to use the service for the update.

The service will also do this check and fail to apply the update if it finds the path is not on an NTFS drive.  If it is an NTFS drive then it supports file locking.

Please let us know what you think.

Comment 7

[Brian R. Bondy [:bbondy]]

Attachment: Patch with NTFS check.

As discussed, if we have NTFS then we will use the service, otherwise we will not use the service.  

I did verify and disallowing sharing on a file in FAT32 works in the same way as NTFS in XP.

So we could:
- Leave it as is and only use in NTFS
- Change it to accept NTFS and FAT32
- Change it to a different check to check if it is a fixed drive (DRIVE_FIXED) from a call to GetDriveType
http://msdn.microsoft.com/en-us/library/windows/desktop/aa364939%28v=vs.85%29.aspx
- Maybe someone has another idea?

Comment 8

[Brian R. Bondy [:bbondy]]

By the way the user profile dir is usually on the primary volume which is always NTFS on Vista and up.  So that's why I am OK with excluding non NTFS volumes.  In those cases the user would get a UAC prompt instead.

Comment 9

[James Forshaw]

Sounds a good idea. 

The only thing I could suggest would be to ensure you copy the update contents to a known good location somewhere which cannot be changed by the user before doing any checks. That way it doesn't much matter if they can break the lock through some remote share trick. However even that has risks, such as if you fail in deleting the content then you have copied a potentially dangerous executable to a "secure" location, some times the OS will trust it more if as a user you could not have written the file. Plus I can imagine it would be a fair amount of extra work for a fairly difficult edge case.

Comment 10

[Brian R. Bondy [:bbondy]]

Thanks for the feedback. 

I think making it more complex at this point would do more harm than good.  Locking the file on a file system we know supports it is good and cannot be exploited to my knowledge.

Comment 11

[James Forshaw]

Looking at the patch, only thing I could suggest would be to ensure you are checking the return from PathStripToRoot (just because I am paranoid, might as well always check a return code, it should return an empty string if no root info, but well...). And I guess there is the even more slight edge case of symlinks.
 
Thankfully MS in their wisdom made symlinks require admin rights (well you could enable them for normal users). But they _could_ in theory point it to a network share. If you strip to root then it will think it is ntfs.

It seems that GetVolumeInformation will actually take a full path and return the correct response even in the symlink case, but I don't know if I would want to rely on that behaviour :)

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 618433 [details] [diff] [review]
Patch without NTFS check

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

Just driving by, but you probably want to make sure that the Create call succeeds, right?

Comment 13

[Robert Strong (they/them - no direct email)]

Comment on attachment 619192 [details] [diff] [review]
Patch with NTFS check.

I'm ok with some or all of the following being done in a followup bug:
1. flag the update to not use the service upon download. 
2. the service shouldn't be installed or updated for this install. That would mean not showing the option in the installer as well.
3. the option to use the service should not be displayed in the preferences ui.
4. might be other cases.

Make sense?

Comment 14

[Brian R. Bondy [:bbondy]]

Good ideas.  After thinking about them more though I think only #3 should be done (and #4 if we think of new things).  

> 1. flag the update to not use the service upon download. 

This one does not need to be done because even know the update is marked as pending-service, the service will simply not be used when we do the check in the unelevated updater before any update is attempted. We do the same type of thing if we happen to notice that we already have write access in the unelevated updater. Instead of launching the service command we just relaunch ourselves as an elevated process.

> 2. the service shouldn't be installed or updated for this install. 
> That would mean not showing the option in the installer as well.

I think we should not do this one because we will use the service for prefetch clearing once that lands, and there may be future use cases for the service. 

> 3. the option to use the service should not be displayed in the preferences ui.

Yup, I'll post a follow up for it as you suggested. 

> 4. might be other cases

Can't think of any off hand but I CC'ed Simona who may think of extra things.

Comment 15

[Robert Strong (they/them - no direct email)]

Makes sense and thanks

Comment 16

[Brian R. Bondy [:bbondy]]

> Looking at the patch, only thing I could suggest would be to ensure you 
> are checking the return from PathStripToRoot

GetVolumeInformationW will fail if that call fails so it is already covered.

 
> Just driving by, but you probably want to make sure that the Create 
> call succeeds, right?

The other security checks will fail in that case so I don't think it needs to be checked.

Comment 17

[Brian R. Bondy [:bbondy]]

I tested this on a Vista machine where I set the OS profile dir to be on a FAT32 volume but the installation dir on NTFS volume and it works.  I also tested this on the same machine with the OS profile dir on an NTFS volume.

I do think ACL's are part of the filesystem but I think file locking is not directly part of the filesystem.  That being said I am not confident that this is the best fix.  I kind of prefer my original fix that doesn't even consider NTFS.  It just locks the file for write access.

Comment 18

[Brian R. Bondy [:bbondy]]

In addition to Comment 17...

I just tested with a samba share to a Ubuntu ext4 drive, and file locking works there too.
I recommend we go with the original patch and I'm re-requesting review for that one. 

Sorry for the time waste on reviewing the second patch but I feel more comfortable catching it late then just going with the flow when I think it is wrong.

The only possible attack metric here is if the user was in control of both computers, the Ubuntu computer can modify the file while it is locked.
I don't think the extra code complexity here is worth it though.  
It also requires the OS' user profile dir to be on that same share.

Comment 19

[Robert Strong (they/them - no direct email)]

Comment on attachment 618433 [details] [diff] [review]
Patch without NTFS check

I think this is sufficient for this attack vector but it would be a good thing to bounce this off of someone in security as well. Perhaps Ian?

Comment 20

[Ian Melven :imelven]

(In reply to Robert Strong [:rstrong] (do not email) from comment #19)
> Comment on attachment 618433 [details] [diff] [review]
> Patch without NTFS check
> 
> I think this is sufficient for this attack vector but it would be a good
> thing to bounce this off of someone in security as well. Perhaps Ian?

i will ask for dveditz's opinion as well.

(In reply to Brian R. Bondy [:bbondy] from comment #18)
> It also requires the OS' user profile dir to be on that same share.

what's the basis of this restriction ? above you had said that the profile dir could be on a FAT32 volume and the installation dir on another volume, why do they need to be colocated in the remote case ? the attack here is the attacker can specify a server they control and then choose to not honor the file locking and exploit the race - if the profile also has to be on that attacker controller server, there are many other security issues, i would think. 

what about the 'copy the update to a local location that can definitely be locked' approach suggested in comment 9 ?

Comment 21

[Brian R. Bondy [:bbondy]]

> > It also requires the OS' user profile dir to be on that same share.
> what's the basis of this restriction ?

I just meant that by default the updates get applied to the user profile directory, but I guess someone could pass commands to the service arbitrarily. 

> the attack here is the attacker can specify a server they control and then 
> choose to not honor the file locking and exploit the race

The attack I mentioned cannot be exploited from a user on the local windows computer.  They have to somehow signal to the other computer to start a process to modify the file at the exact instant.  If the user from the local computer tried to modify the file, even if they had write access they would get a sharing violation error. 

If we can't trust file sharing in Windows then I think that we have issues elsewhere as well. 

> what about the 'copy the update to a local location that can definitely 
> be locked' approach suggested in comment 9 ?

If the above isn't acceptable I'd rather go back to the NTFS approach which will cover the case of remote shares.   Copying the file to another location and requiring it to be deleted opens up a lot of other failure conditions that we'd have to test for and I'd rather avoid that.

Comment 22

[Brian R. Bondy [:bbondy]]

However extremely unlikely Comment 21's attack is, I realize that I'm asking you to give the OK on a known attack. 

If it's OK with you Ian, I'd like to just land the already tested NTFS patch.

Comment 23

[Brian R. Bondy [:bbondy]]

Another thing we could do is instead of checking the filesystem type which doesn't matter and using that to mean local or remote, we could just use GetDriveType checking for DRIVE_FIXED.

http://msdn.microsoft.com/en-us/library/windows/desktop/aa364939%28v=vs.85%29.aspx

Please advise.

Recall: It doesn't matter if your local drive is FAT32 or NTFS file locking works.

Comment 24

[Ian Melven :imelven]

(In reply to Brian R. Bondy [:bbondy] from comment #21)
> The attack I mentioned cannot be exploited from a user on the local windows
> computer.  They have to somehow signal to the other computer to start a
> process to modify the file at the exact instant.  

this is true, but they just need to win the race they want once.. 

> If the user from the local
> computer tried to modify the file, even if they had write access they would
> get a sharing violation error. 
> 
> If we can't trust file sharing in Windows then I think that we have issues
> elsewhere as well. 

indeed.
 
> > what about the 'copy the update to a local location that can definitely 
> > be locked' approach suggested in comment 9 ?
> 
> If the above isn't acceptable I'd rather go back to the NTFS approach which
> will cover the case of remote shares.   Copying the file to another location
> and requiring it to be deleted opens up a lot of other failure conditions
> that we'd have to test for and I'd rather avoid that.

yes, that makes a lot of sense !

Comment 25

[Ian Melven :imelven]

(In reply to Brian R. Bondy [:bbondy] from comment #22)
> However extremely unlikely Comment 21's attack is, I realize that I'm asking
> you to give the OK on a known attack. 
> 
> If it's OK with you Ian, I'd like to just land the already tested NTFS patch.

yes, and an externally reported attack, at that.

i'm fine with landing the NTFS patch. DRIVE_FIXED also looks like that will mitigate this also if you prefer that approach - it perhaps is slightly better in terms of not having to be so strict about filesystem type.

Comment 26

[Brian R. Bondy [:bbondy]]

Attachment: Patch with local drive check

Carrying forward r+. 

The fix is the same as the NTFS fix but it checks specifically for a fixed drive rather than checking for NTFS.  

Thanks for for your input Robert and Ian.

Comment 27

[Robert Strong (they/them - no direct email)]

Considering the number of users using shares is extremely likely to be relatively small not using the service for them seems like the best path forward especially if DRIVE_FIXED is used.

Comment 28

[Brian R. Bondy [:bbondy]]

Attachment: Patch with local drive check. v2.

:%s/GetDriveType/GetDriveTypeW

Comment 29

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

I was asked to chime in...

tl;dr: It seems OK/good to do the DRIVE_FIXED check but it isn't clear that it completely solves the problem, though it probably does. We should follow up with implementing the suggestion in bug 748948 comment 9 unless we can find that DRIVE_FIXED is specified to guarantee that locking will be honored.

I think that the DRIVE_FIXED check is good because then there is less of a chance of multiple clients trying to update the same network-share-installed copy of Firefox. The network share system itself should be running the update service, or else the network admin probably doesn't want anybody to try to update the shared copy.

However, I am not so confident that the DRIVE_FIXED check is sufficient. Is there some documentation that says that file access modes are always honored for DRIVE_FIXED? I get nervous about relying on things that are only determined to be true experimentally, because we may be relying on assumptions that are not necessarily true now or which could become wrong in the future.

I agree with the people who said that that writing the updater to the install folder is the most obviously-correct fix. If an attacker can write to the install folder then it is already game-over. If we do anything less then that, then how will we be confident that the fix works in all situations? IMO, we should at least file a follow-up bug for doing that. 

It is good news that the install drive will usually be NTFS on Vista+, but it would be good to have the service working for as many people as possible, including on Windows XP, which means supporting . So, I agree with the decision to remove the NTFS check.

Comment 30

[Brian R. Bondy [:bbondy]]

Thanks for chiming in.

I don't think that there is a case where file locking is not respected when the drive is DRIVE_FIXED but I would welcome a bug to work on that presented a counter example. 

>  I agree with the people who said that that writing the updater to the install folder is the most 
> obviously-correct fix.

I don't agree.  In addition to my previous comments, here is my more sound reasoning:

The installation directory is not guaranteed to be in a secure location.  We often install into the user's app data which is not secure.  If we allow the exe to run in the installation directory that means we allow it to run an exe that could be in an insecure location.

You could say that it's the user's fault for installing into an insecure location, but the harmful effects are greater than them messing up firefox.  They can escalate their limited user account to the system account and indirectly to executing code in any logged in session. 

Even if we forced users to install into program files, I know that some users have already set write access into our installation directory. Forcing users into program files would surely have some consequences and we'd leave a long tail of users in an insecure state if we did it. We could try to find some other directory that we don't own or create such a secure directory to run the executable from but this seems like overkill and leaves room for more holes and problems.

Comment 31

[Brian R. Bondy [:bbondy]]

Attachment: Patch with local drive check. v3.

Carrying forward r+.

I added in a strict check to make sure restricting file sharing succeeds as per bent's suggestion.  This is important since the security depends on us obtaining a file handle that specifies not to share write access.  For example the call could have failed if someone tried an attack that opens the file with write access already.

Comment 32

[Brian R. Bondy [:bbondy]]

In addition FAT32 lacks ACLs, whereas the fs type doesn't seem to matter with file open sharing restricted.

Comment 33

[James Forshaw]

> I don't agree.  In addition to my previous comments...

I would argue that it isn't the firefox installation directory to which you should be writing, it is the maintenance service's directory. After all if you can already write to that directory then you can replace the service binary and gain system anyway.

That said see bug https://bugzilla.mozilla.org/show_bug.cgi?id=750850 which I filed yesterday to understand why even that isn't going to be a good idea necessarily :)

It does worry me somewhat that the level of complexity required to do the "right thing" seems to be spiraling out of control somewhat.

Comment 34

[Robert Strong (they/them - no direct email)]

It has been my experience that doing the "right thing" typically does spiral out of control when trying to prevent attack vectors that require local system access though we do try to protect against them at least for the normal use case and often for edge cases as well.

Comment 35

[Brian R. Bondy [:bbondy]]

I would add to Comment 34 that this is equivalent to running an elevated process in general. Since you can simply call CreateService followed by StartService to run any code as the local system account.  

Even more horrible in Windows is that if you are either an elevated administrator or the system account (equivalent because of the above), you can run code from any user account that is logged in by simply walking the process list and using any token, then getting the linked elevated token to that token, then calling CreateProcessAsUser with that token.

Comment 36

[Brian R. Bondy [:bbondy]]

Tested this GetDriveType / share lock fix by forcing the profile dir to be on a remote drive. It falls back to UAC without trying the service.  Also tested normal updates via elm and they are working.  Will land shortly.

Comment 37

[Brian R. Bondy [:bbondy]]

http://hg.mozilla.org/mozilla-central/rev/dc1d88aff8e5

Comment 38

[Brian R. Bondy [:bbondy]]

Comment on attachment 620164 [details] [diff] [review]
Patch with local drive check. v3.

[Approval Request Comment]
Regression caused by (bug #): 481815
User impact if declined: unelevated process can run code elevated
Testing completed (on m-c, etc.): I tested updates on elm, I will wait for 2 days on m-c to ensure updates are working there before pushing to aurora and beta.
Risk to taking this patch (and alternatives if risky): low
String changes made by this patch: none

Comment 39

[Alex Keybl [:akeybl]]

Comment on attachment 620164 [details] [diff] [review]
Patch with local drive check. v3.

[Triage Comment]
Looks good on m-c, approving this sg:crit for Aurora 14 and Beta 13.

If the ESR10 branch is affected, please prepare the applicable patch.

Comment 40

[Brian R. Bondy [:bbondy]]

http://hg.mozilla.org/releases/mozilla-beta/rev/023b3a0e655b
http://hg.mozilla.org/releases/mozilla-aurora/rev/01f32caf02bf

Comment 41

[u279076]

Is there something QA can do to verify this fix?

Comment 42

[Brian R. Bondy [:bbondy]]

I don't think so, the original report was pretty hard to exploit.  I can't think of anything but maybe someone else will chime in.

Comment 43

[u279076]

Okay, thanks Brian. If someone else is able to reproduce this exploit, please go ahead and verify the fix on Firefox 13.0b3, latest-mozilla-aurora, and latest-mozilla-central.