Closed Bug 936252 Opened 6 years ago Closed 6 years ago

Augment seccomp whitelist for b2g mochitests

Categories

(Core :: Security, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: jld, Assigned: jld)

References

Details

(Whiteboard: [c= p= s=2013.11.22 u= ])

Attachments

(1 file, 3 obsolete files)

I've gotten mochitests to pass on the b2g emulator; the necessary whitelist changes are in this patch.  (There are also some issues with SpecialPowers calling FileUtils calling mkdir; that will be a separate bug.)  Things:

1. The crash reporter uses socketpair and sendmsg to send a pipe to the parent.

2. One of the form tests calls nsIFile's remove method, which uses lstat.  Obviously we should avoid touching files in the content process, but for now we already allow stat, so lstat is presumably not worse.

3. FormHistory is using sqlite3 in the child process.  I'm not sure if this should be happening in the child process at all, but as long as it is: sqlite3 wants geteuid (which should be harmless?) and fsync, which in principle falls under operations on fds the process already has, but in practice makes me a little nervous in terms of what it means to filesystem code.

Also, because this should be mentioned somewhere: lstat64 and geteuid32 are, like sigaction vs. rt_sigaction, syscalls with other versions that Desktop or later versions of B2G might need to care about.
Attachment #828947 - Flags: review?(gdestuynder)
Rebase to not depend on bug 936145.
Attachment #828947 - Attachment is obsolete: true
Attachment #828947 - Flags: review?(gdestuynder)
Attachment #828961 - Flags: review?(gdestuynder)
Reverse previous rebase, now that I've read my feedback in bug 936145 a little more closely.  Sorry for the noise.
Attachment #828961 - Attachment is obsolete: true
Attachment #828961 - Flags: review?(gdestuynder)
Attachment #829404 - Flags: review?(gdestuynder)
Comment on attachment 829404 [details] [diff] [review]
bug936252-seccomp-mochitest-augment-hg2.diff

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

See the comment about socketpair. Otherwise ok.

::: security/sandbox/linux/seccomp_filter.h
@@ +147,5 @@
>    SECCOMP_WHITELIST_ARCH_LAST \
>    /* restart_syscall is called internally, generally when debugging */ \
>    ALLOW_SYSCALL(restart_syscall), \
>    ALLOW_SYSCALL(uname), \
> +  ALLOW_SYSCALL(socketpair), \

we should put this under "remove if possible" due to the possibility of creating a new communication channel with socketpair. Actually, I wonder where it's used, since generally you would:
socketpair()
fork()
and use one for the parent, one for the child. in our case I guess it's passed to the parent over the existing IPC (?) (in this case, it sounds like the parent should call socketpair since it's a "resource creation")
Attachment #829404 - Flags: review?(gdestuynder) → review+
(In reply to Guillaume Destuynder [:kang] (use NEEDINFO!) from comment #3)
> Actually, I wonder where it's used, since generally you would:
> socketpair()
> fork()
> and use one for the parent, one for the child. in our case I guess it's
> passed to the parent over the existing IPC (?) (in this case, it sounds like
> the parent should call socketpair since it's a "resource creation")

I tracked this down to bug 516759; there might be some hints as to rationale there.
Carrying over r=kang.
Attachment #829404 - Attachment is obsolete: true
Attachment #829685 - Flags: review+
(In reply to Guillaume Destuynder [:kang] (use NEEDINFO!) from comment #3)
> we should put this under "remove if possible" due to the possibility of
> creating a new communication channel with socketpair. Actually, I wonder
> where it's used

I just realized I didn't (directly) answer this question earlier.  It's in google_breakpad::CrashGenerationClient::RequestDump, in toolkit/crashreporter/google-breakpad/src/client/linux/crash_generation/crash_generation_client.cc:

    sys_socketpair(AF_UNIX, SOCK_STREAM, 0, fds);
    ...
    ssize_t ret = HANDLE_EINTR(sys_sendmsg(server_fd_, &msg, 0));
Whiteboard: [c= p= s= u= ]
Duplicate of this bug: 925119
https://hg.mozilla.org/mozilla-central/rev/77aa3cf5873a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Whiteboard: [c= p= s= u= ] → [c= p= s=2013.11.22 u= ]
You need to log in before you can comment on or make changes to this bug.