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.
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
Product: Firefox → Core
Confirmed and taking.
Assignee: nobody → gpascutto
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.
Attachment #8879264 - Flags: review?(jld)
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>
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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+
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?
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 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+
Should we consider this for ESR52 also?
ESR52 doesn't enable content sandboxing on Linux.
Whiteboard: [sb+][adv-main55+] → [sb+][adv-main55+][post-critsmash-triage]
You need to log in before you can comment on or make changes to this bug.