Bug 1374281 (CVE-2017-7794)

linux sandbox broker permits truncating files using read-only access

RESOLVED FIXED in Firefox 55

Status

()

defect
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: jannh, Assigned: gcp)

Tracking

({sec-moderate})

52 Branch
mozilla56
Unspecified
Linux
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox54 wontfix, firefox55 fixed, firefox56 fixed)

Details

(Whiteboard: [sb+][adv-main55+][post-critsmash-triage])

Attachments

(1 attachment)

Reporter

Description

2 years ago
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

2 years ago
Bob, can you look at this and/or forward this to the right people? :-)
Group: firefox-core-security → core-security
Component: Untriaged → Security: Process Sandboxing
Flags: needinfo?(bobowencode)
Product: Firefox → Core

Updated

2 years ago
Flags: needinfo?(bobowencode) → needinfo?(gpascutto)
Group: core-security → dom-core-security
Assignee

Comment 2

2 years ago
Confirmed and taking.
Assignee: nobody → gpascutto
Flags: needinfo?(gpascutto)
Assignee

Comment 3

2 years ago
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.
Assignee

Comment 4

2 years ago
MozReview-Commit-ID: Ko5m5i4Wkd6
Attachment #8879264 - Flags: review?(jld)
Assignee

Comment 5

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

commit 834f2a4a1554dc5b2598038b3fe8703defcbe467
Author: Trond Myklebust <Trond.Myklebust@netapp.com>
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 <Trond.Myklebust@netapp.com>
Assignee

Comment 6

2 years ago
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.
Assignee

Updated

2 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: sec-moderate
OS: Unspecified → Linux
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.
Attachment #8879264 - Flags: review?(jld) → review+
Assignee

Comment 8

2 years ago
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
Attachment #8879264 - Flags: approval-mozilla-beta?

Updated

2 years ago
Whiteboard: [sb+]
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)
Flags: needinfo?(gpascutto)
Assignee

Updated

2 years ago
Flags: needinfo?(gpascutto)
https://hg.mozilla.org/mozilla-central/rev/c963d52551ab
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment on attachment 8879264 [details] [diff] [review]
Require MAY_WRITE for O_TRUNC

linux sandbox fix, beta55+
Attachment #8879264 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Group: dom-core-security → core-security-release
Whiteboard: [sb+] → [sb+][adv-main55+]
Should we consider this for ESR52 also?
Blocks: 1289718
Flags: needinfo?(gpascutto)
Version: 54 Branch → 52 Branch
ESR52 doesn't enable content sandboxing on Linux.
Flags: needinfo?(gpascutto)
Alias: CVE-2017-7794
Flags: qe-verify-
Whiteboard: [sb+][adv-main55+] → [sb+][adv-main55+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.