1374281 - (CVE-2017-7794) linux sandbox broker permits truncating files using read-only access

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

[Jann Horn]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/59.0.3071.104 Safari/537.36

Steps to reproduce:

[I'm filing this as a restricted bug because this is a lack of validation in security-sensitive code; however, I don't see any real security impact here. Feel free to derestrict the bug if you agree.]

On Linux:

1. Launch Firefox.
2. Ensure that a content process is used.
3. Create a non-empty file in the home directory:

    $ echo foobar > ~/testfile

4. Attach to the content process with gdb and verify that ~/testfile can be opened in readonly mode (O_RDONLY=0), but not as writable (O_WRONLY=1, O_RDWR=2):

    $ gdb attach 4436
    [...]
    0x00007f4805fa384d in poll () from /lib/x86_64-linux-gnu/libc.so.6
    (gdb) handle SIGSYS nostop noprint pass 
    Signal        Stop	Print	Pass to program	Description
    SIGSYS        No	No	Yes		Bad system call
    (gdb) print open("/home/user/testfile", 0)
    $2 = 40
    (gdb) print open("/home/user/testfile", 1)
    $3 = -1
    (gdb) print open("/home/user/testfile", 2)
    $4 = -1

5. Use gdb to inject open("/home/user/testfile", O_RDONLY|O_TRUNC):

    (gdb) print open("/home/user/testfile", 0x200)
    $5 = 42



Actual results:

~/testfile was truncated:

$ ls -l ~/testfile
-rw-r----- 1 user user 0 Jun 19 15:07 /home/user/testfile



Expected results:

The sandbox should prevent any modifications to ~/testfile. O_TRUNC should always require write access.

Comment 1

[:Gijs (he/him)]

Bob, can you look at this and/or forward this to the right people? :-)

Comment 2

[Gian-Carlo Pascutto [:gcp]]

Confirmed and taking.

Comment 3

[Gian-Carlo Pascutto [:gcp]]

The fact that this is possible is a bit surprising IMHO. The design of O_ACCMODE flags makes it seem like they definitely are intended to be the thing that delimits what access is possible. 

I checked:
http://pubs.opengroup.org/onlinepubs/7908799/xsh/open.html

O_TRUNC
    If the file exists and is a regular file, and the file is successfully opened O_RDWR or O_WRONLY, its length is truncated to 0 and the mode and owner are unchanged. It will have no effect on FIFO special files or terminal device files. Its effect on other file types is implementation-dependent. **The result of using O_TRUNC with O_RDONLY is undefined.**

It seems Linux does the most unhelpful (and IMHO illogical) thing possible here by allowing the truncate to go through. Is this common on Unices (thereby necessitating it for backwards compat) or is it a Linux misfeature (that might be reported upstream?).

There's no real security impact from this bug, from the security person perspective, but I suspect most users won't find it helpful when you tell them their data being wiped is "not a security bug", so I think we can treat is as such.

Comment 4

[Gian-Carlo Pascutto [:gcp]]

Attachment: Require MAY_WRITE for O_TRUNC

MozReview-Commit-ID: Ko5m5i4Wkd6

Comment 5

[Gian-Carlo Pascutto [:gcp]]

Linux made O_TRUNC work with O_RDONLY in this (unrelated) commit from v2.6.14-rc4:

commit 834f2a4a1554dc5b2598038b3fe8703defcbe467
Author: Trond Myklebust <[e-mail]>
Date:   Tue Oct 18 14:20:16 2005 -0700

    VFS: Allow the filesystem to return a full file pointer on open intent
    
     This is needed by NFSv4 for atomicity reasons: our open command is in
     fact a lookup+open, so we need to be able to propagate open context
     information from lookup() into the resulting struct file's
     private_data field.
    
     Signed-off-by: Trond Myklebust <[e-mail]>

Comment 6

[Gian-Carlo Pascutto [:gcp]]

For the record, current FreeBSD also allows O_TRUNC | O_RDONLY ("If O_TRUNC is specified and the file exists, the file is truncated to zero length."), but Windows has the sane behavior ("Opens a file and truncates it to zero length; the file must have write permission. Cannot be specified with _O_RDONLY."). Didn't test macOS.

Comment 7

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Comment on attachment 8879264 [details] [diff] [review]
Require MAY_WRITE for O_TRUNC

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

Looks good to me.  The comment does make the nature of the bug a little more obvious than otherwise, but I'd expect that anyone who's already gotten code execution in a content process will be able to figure that out no matter what we do to the patch, so we might as well document it.

Comment 8

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8879264 [details] [diff] [review]
Require MAY_WRITE for O_TRUNC

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1289718
[User impact if declined]: Data wiped when content process is compromised (sec-moderate)
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: No, but possible. See original report.
[List of other uplifts needed for the feature/fix]: No
[Is the change risky?]: No
[Why is the change risky/not risky?]: Changes behavior that is considered undefined by POSIX
[String changes made/needed]: N/A

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Since this is rated sec-moderate you can go ahead and land it on trunk. Once it sticks on m-c we can try beta uplift (later this week)

Comment 10

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c963d52551abd451b110a3a5b4974cee1f96f4f2
Bug 1374281. r=jld

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c963d52551ab

Comment 12

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 8879264 [details] [diff] [review]
Require MAY_WRITE for O_TRUNC

linux sandbox fix, beta55+

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b32263b43666

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Should we consider this for ESR52 also?

Comment 15

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

ESR52 doesn't enable content sandboxing on Linux.