1287426 - Update security/sandbox/chromium/ to Chromium stable channel version 49.0.2623.112

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

[Bob Owen (:bobowen)]

Update to commit 4ec79b7f2379a60cdc15599e93255c0fa417f1ed

This is up to the last Chromium stable channel that still supports WinXP and Vista.

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=81971b781d728ef5bf6d958b140e93d70d8aa0f8

Comment 1

[Bob Owen (:bobowen)]

Attachment: Part 1: Ignore clang warnings for implicit conversions in Chromium sandbox code

This to prevent warnings from the update in Part 3.
It is similar to the ignoring of implicit constructors.

MozReview-Commit-ID: Bq0PocdYdm9

Comment 2

[Bob Owen (:bobowen)]

Attachment: Part 2: Remove uneeded base/basictypes.h include from WindowsMessageLoop.h

This is actually picking up the sandbox file, which is going away, but looks like it's not needed.

MozReview-Commit-ID: AZ9cUmxHk4R

Comment 3

[Bob Owen (:bobowen)]

Attachment: Part 3: Update security/sandbox/chromium/ to commit 4ec79b7f2379a60cdc15599e93255c0fa417f1ed

Apart from the straight forward chromium file updates there is the following.

Chromium base/build additions / removals
A security/sandbox/chromium/base/bit_cast.h - new dependency
A security/sandbox/chromium/base/numerics/safe_math.h - new dependency
A security/sandbox/chromium/base/numerics/safe_math_impl.h - new dependency
A security/sandbox/chromium/base/third_party/valgrind/LICENSE - new dependency
A security/sandbox/chromium/base/third_party/valgrind/valgrind.h - new dependency
A security/sandbox/chromium/base/threading/platform_thread_internal_posix.cc - new dependency
A security/sandbox/chromium/base/threading/platform_thread_internal_posix.h - new dependency
A security/sandbox/chromium/build/buildflag.h - new dependency
R security/sandbox/chromium/base/atomicops_internals_arm_gcc.h - file no longer exists
R security/sandbox/chromium/base/atomicops_internals_x86_gcc.h - file no longer exists
R security/sandbox/chromium/base/basictypes.h - file no longer exists
R security/sandbox/chromium/base/float_util.h - file no longer exists
R security/sandbox/chromium/base/port.h - file no longer exists
R security/sandbox/chromium/base/safe_strerror_posix.cc - file moved, but no longer appears to be needed
R security/sandbox/chromium/base/safe_strerror_posix.h - file moved, but no longer appears to be needed

Chromium Windows sandbox additions / removals
A security/sandbox/chromium/sandbox/win/src/sandbox_rand.cc - new linux sandbox dependency
A security/sandbox/chromium/sandbox/win/src/sandbox_rand.h - new linux sandbox dependency
A security/sandbox/chromium/sandbox/win/src/top_level_dispatcher.cc - new linux sandbox dependency
A security/sandbox/chromium/sandbox/win/src/top_level_dispatcher.h - new linux sandbox dependency
R security/sandbox/chromium/sandbox/win/src/handle_table.cc - file no longer exists
R security/sandbox/chromium/sandbox/win/src/handle_table.h - file no longer exists
R security/sandbox/chromium/sandbox/win/src/shared_handles.cc - file no longer exists
R security/sandbox/chromium/sandbox/win/src/shared_handles.h - file no longer exists

Chromium Linux sandbox additions / moves / removals
A security/sandbox/chromium/sandbox/linux/bpf_dsl/codegen.cc - moved from seccomp-bpf/codegen.cc
A security/sandbox/chromium/sandbox/linux/bpf_dsl/codegen.h - moved from seccomp-bpf/codegen.h
A security/sandbox/chromium/sandbox/linux/bpf_dsl/errorcode.h - moved from seccomp-bpf/errorcode.h
A security/sandbox/chromium/sandbox/linux/bpf_dsl/linux_syscall_ranges.h - new linux sandbox dependency
A security/sandbox/chromium/sandbox/linux/bpf_dsl/seccomp_macros.h - new linux sandbox dependency
A security/sandbox/chromium/sandbox/linux/bpf_dsl/syscall_set.cc - new linux sandbox dependency
A security/sandbox/chromium/sandbox/linux/bpf_dsl/syscall_set.h - new linux sandbox dependency
A security/sandbox/chromium/sandbox/linux/services/syscall_wrappers.cc - new linux sandbox dependency
A security/sandbox/chromium/sandbox/linux/services/syscall_wrappers.h - new linux sandbox dependency
A security/sandbox/chromium/sandbox/linux/system_headers/arm_linux_syscalls.h - moved from services/arm_linux_syscalls.h
A security/sandbox/chromium/sandbox/linux/system_headers/arm_linux_ucontext.h - moved and renamed from android_arm_ucontext.h
A security/sandbox/chromium/sandbox/linux/system_headers/capability.h - new linux sandbox dependency
A security/sandbox/chromium/sandbox/linux/system_headers/i386_linux_ucontext.h - moved and renamed from android_i386_ucontext.h 
A security/sandbox/chromium/sandbox/linux/system_headers/linux_filter.h - new linux sandbox dependency
A security/sandbox/chromium/sandbox/linux/system_headers/linux_futex.h - moved and renamed from android_futex.h 
A security/sandbox/chromium/sandbox/linux/system_headers/linux_seccomp.h - moved from seccomp-bpf/linux_seccomp.h
A security/sandbox/chromium/sandbox/linux/system_headers/linux_signal.h - new linux sandbox dependency
A security/sandbox/chromium/sandbox/linux/system_headers/linux_syscalls.h - moved from services/linux_syscalls.h
A security/sandbox/chromium/sandbox/linux/system_headers/linux_ucontext.h - moved and renamed from android_ucontext.h 
A security/sandbox/chromium/sandbox/linux/system_headers/x86_32_linux_syscalls.h - moved from services/x86_32_linux_syscalls.h
A security/sandbox/chromium/sandbox/linux/system_headers/x86_64_linux_syscalls.h - moved from services/x86_64_linux_syscalls.h
A security/sandbox/chromium/sandbox/linux/system_headers/x86_64_linux_ucontext.h - moved and renamed from android_x86_64_ucontext.h 
R security/sandbox/chromium/sandbox/linux/seccomp-bpf/basicblock.cc - file no longer exists
R security/sandbox/chromium/sandbox/linux/seccomp-bpf/basicblock.h - file no longer exists
R security/sandbox/chromium/sandbox/linux/seccomp-bpf/instruction.h - file no longer exists
R security/sandbox/chromium/sandbox/linux/seccomp-bpf/syscall_iterator.cc - I think replaced by syscall_set.cc
R security/sandbox/chromium/sandbox/linux/seccomp-bpf/syscall_iterator.h - I think replaced by syscall_set.h
R security/sandbox/chromium/sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc
R security/sandbox/chromium/sandbox/linux/seccomp-bpf/verifier.cc - file moved, but no longer appears to be needed
R security/sandbox/chromium/sandbox/linux/seccomp-bpf/verifier.h - file moved, but no longer appears to be needed

Chromium sandbox shim changes to take account of the above
M security/sandbox/chromium-shim/base/logging.cpp - new dummy logging dependencies added
M security/sandbox/chromium-shim/base/win/sdkdecls.h - Windows function decalrations that are not available on Windows XP added. These are loaded dynamically, but they still rely on the declarations to describe the function pointers.
A security/sandbox/chromium-shim/base/debug/debugging_flags.h - generated by chromium build, required by base/debug/profiler.cc
A security/sandbox/chromium-shim/base/file_version_info_win.h - dummy implementation of FileVersionInfoWin required by unused function GetVersionFromKernel32 in base/win/windows_version.cc
A security/sandbox/chromium-shim/base/files/file_path.cpp - dummy implementation of FilePath functions required by FileVersionInfoWin

Gecko Linux sandbox changes to take account of the above - mainly includes changes, but also some code gen changes
M security/sandbox/linux/Sandbox.cpp
M security/sandbox/linux/SandboxFilter.cpp
M security/sandbox/linux/SandboxFilterUtil.h
M security/sandbox/linux/SandboxLogging.cpp
M security/sandbox/linux/SandboxUtil.cpp
M security/sandbox/linux/common/SandboxInfo.cpp
M security/sandbox/linux/moz.build

Gecko Windows sandbox changes to take account of the above
M security/sandbox/moz.build


MozReview-Commit-ID: 14eHMsYZznA

Comment 4

[Bob Owen (:bobowen)]

Attachment: Part 4: Re-apply pre-vista stdout/err process inheritance change to Chromium code after merge

Originally landed as changset:
https://hg.mozilla.org/mozilla-central/rev/f94a07671389

MozReview-Commit-ID: 2dpjBXkzlze

Comment 5

[Bob Owen (:bobowen)]

Attachment: Part 5: Re-apply - Logging changes to the Chromium interception code

Originally landed as changset:
https://hg.mozilla.org/mozilla-central/rev/0f763c186855

MozReview-Commit-ID: DtuHfDoB1Dx

Comment 6

[Bob Owen (:bobowen)]

Attachment: Part 6: Re-apply - Change Chromium sandbox to allow rules for files on network drives to be added

Originally landed as changset:
https://hg.mozilla.org/mozilla-central/rev/afa4f68de47c

MozReview-Commit-ID: A18C0KcEqvP

Comment 7

[Bob Owen (:bobowen)]

Attachment: Part 7: Re-apply - Update chromium's list of linux-x86-32 syscalls

Originally landed as changset:
https://hg.mozilla.org/mozilla-central/rev/adb1d2a92e0d

MozReview-Commit-ID: KpjitH5GQEq

Comment 8

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8772454 [details] [diff] [review]
Part 3: Update security/sandbox/chromium/ to commit 4ec79b7f2379a60cdc15599e93255c0fa417f1ed

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

As usual, rs=me.

Comment 9

[Bob Owen (:bobowen)]

(In reply to Aaron Klotz [:aklotz] from comment #8)

> As usual, rs=me.

Thanks Aaron, have you cast your eye over the security/sandbox/chromium-shim changes/additions to make sure I've not done anything stupid there?

Comment 10

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

Comment on attachment 8772454 [details] [diff] [review]
Part 3: Update security/sandbox/chromium/ to commit 4ec79b7f2379a60cdc15599e93255c0fa417f1ed

Linux parts look good; thanks for fixing the breakage.

Comment 11

[Bob Owen (:bobowen)]

I re-pushed to try, to make sure I didn't break anything:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=53671b3e0eb082a12f13a68b19c38b39c872149a

I've found an odd problem though, when I try running on Windows with sandbox level 1, the content process crashes here:
	xul.dll!mozilla::dom::ProcessGlobal::SetInitialProcessData Line 109
 	xul.dll!mozilla::dom::ContentChild::InitXPCOM Line 1038
 	xul.dll!mozilla::dom::ContentProcess::Init Line 125
 	xul.dll!XRE_InitChildProcess Line 647
 	firefox.exe!content_process_main Line 224
 	firefox.exe!NS_internal_main Line 357
 	firefox.exe!wmain Line 118
 	firefox.exe!__scrt_common_main_seh Line 255
 	kernel32.dll!@BaseThreadInitThunk@12
 	ntdll.dll!___RtlUserThreadStart@8
 	ntdll.dll!__RtlUserThreadStart@8

With error:
Exception thrown: read access violation.
this was nullptr.

Level 0 (sandbox off) and 2 (current Nightly) work.

Comment 12

[Bob Owen (:bobowen)]

Problem with USER_NON_ADMIN access token level used with level 1 - impersonation token on main thread (initial weaker token before lockdown) gets replaced when ::CoInitializeSecurity is called in MainThreadRuntime::InitializeSecurity with one for NT AUTHORITY\ANONYMOUS LOGON user with untrusted integrity, so other reads after that fail.

Haven't worked out why this happens when USER_NON_ADMIN is used as the lockdown token.  Only changes I can see in that area are around lifetime management of token handles, so wonder if the token is getting deleted somehow.

Comment 13

[Bob Owen (:bobowen)]

Attachment: Part 8: Change the USER_NON_ADMIN token to be a restricted token with the same access

This is to work around an issue where the call to CoInitializeSecurity in MainThreadRuntime::InitializeSecurity causes the impersonation token, used to give the pre-lockdown permissions, to be replaced with one with no rights.
This only seems to happen when the lockdown token is USER_NON_ADMIN, which is not a restricted token.

I can't work out what has changed in the chromium code to cause this problem with USER_NON_ADMIN.
The main (lockdown) and impersonation (initial) tokens look identical with this latest chromium code.
I've debugged the disassembled code, so the change definitely happens on the call to CoInitializeSecurity not in any RAII code that happens on return.


MozReview-Commit-ID: 6HFuDFmWLTf

Comment 14

[(No longer employed by Mozilla) Aaron Klotz]

Have you done any experimentation with the dwImpLevel with CoInitializeSecurity? ie, would a different COM impersonation level prevent it from messing with the impersonation token?

Comment 15

[Bob Owen (:bobowen)]

(In reply to Aaron Klotz [:aklotz] from comment #14)
> Have you done any experimentation with the dwImpLevel with
> CoInitializeSecurity? ie, would a different COM impersonation level prevent
> it from messing with the impersonation token?

I haven't, I'll try that when I get back, thanks.

Comment 16

[Bob Owen (:bobowen)]

(In reply to Aaron Klotz [:aklotz] from comment #14)
> Have you done any experimentation with the dwImpLevel with
> CoInitializeSecurity? ie, would a different COM impersonation level prevent
> it from messing with the impersonation token?

All values of dwImpLevel seem to fail in a similar way, except for RPC_C_IMP_LEVEL_DEFAULT where the CoInitializeSecurity fails completely.

What I don't understand is that the tokens on the process look identical (as far as I can tell) before and after Part 3.
Clearly something must be different, but I can't see what.

Comment 17

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8781162 [details] [diff] [review]
Part 8: Change the USER_NON_ADMIN token to be a restricted token with the same access

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

Please re-submit after answering my question. :-)

::: security/sandbox/chromium/sandbox/win/src/restricted_token_utils.cc
@@ +52,5 @@
>        sid_exceptions.push_back(WinWorldSid);
>        sid_exceptions.push_back(WinInteractiveSid);
>        sid_exceptions.push_back(WinAuthenticatedUserSid);
>        privilege_exceptions.push_back(SE_CHANGE_NOTIFY_NAME);
> +      restricted_token.AddRestrictingSid(WinBuiltinUsersSid);

How did you arrive at this list of restricting sids to add?

Comment 18

[Bob Owen (:bobowen)]

Attachment: Part 8: Change the USER_NON_ADMIN token to be a restricted token with the same access

(In reply to Aaron Klotz [:aklotz] from comment #17)
> Comment on attachment 8781162 [details] [diff] [review]

> How did you arrive at this list of restricting sids to add?

Fair question. :-)
Hopefully the comment I've added explains and my reasoning is sound.

MozReview-Commit-ID: 6HFuDFmWLTf

Comment 19

[(No longer employed by Mozilla) Aaron Klotz]

OK, let's go with that.

Comment 20

[Bob Owen (:bobowen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=027dc35c0cc6

Comment 21

[Bob Owen (:bobowen)]

Comment on attachment 8772458 [details] [diff] [review]
Part 4: Re-apply pre-vista stdout/err process inheritance change to Chromium code after merge

Carrying r=tabraldes from original changeset:
https://hg.mozilla.org/mozilla-central/rev/f94a07671389

Comment 22

[Bob Owen (:bobowen)]

Comment on attachment 8772459 [details] [diff] [review]
Part 5: Re-apply - Logging changes to the Chromium interception code

Carrying r=tabraldes from original changeset:
https://hg.mozilla.org/mozilla-central/rev/0f763c186855

Comment 23

[Bob Owen (:bobowen)]

Comment on attachment 8772460 [details] [diff] [review]
Part 6: Re-apply - Change Chromium sandbox to allow rules for files on network drives to be added

Carrying r=aklotz from original changeset:
https://hg.mozilla.org/mozilla-central/rev/afa4f68de47c

Comment 24

[Bob Owen (:bobowen)]

Comment on attachment 8772461 [details] [diff] [review]
Part 7: Re-apply - Update chromium's list of linux-x86-32 syscalls

Carrying r=jld from original changeset:
https://hg.mozilla.org/mozilla-central/rev/adb1d2a92e0d

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7d0c8ff20f1
Part 1: Ignore clang warnings for implicit conversions in Chromium sandbox code. r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/23c95e36d1be
Part 2: Remove uneeded base/basictypes.h include from WindowsMessageLoop.h r=jimm
https://hg.mozilla.org/integration/mozilla-inbound/rev/a46f0e32289b
Part 3: Update security/sandbox/chromium/ to commit 4ec79b7f2379a60cdc15599e93255c0fa417f1ed. r=aklotz, r=jld
https://hg.mozilla.org/integration/mozilla-inbound/rev/00d1d90c348b
Part 4: Re-apply pre-vista stdout/err process inheritance change to Chromium code after merge. r=tabraldes
https://hg.mozilla.org/integration/mozilla-inbound/rev/a05726163a79
Part 5: Re-apply - Logging changes to the Chromium interception code. r=tabraldes
https://hg.mozilla.org/integration/mozilla-inbound/rev/9069b89a690b
Part 6: Re-apply - Change Chromium sandbox to allow rules for files on network drives to be added. r=aklotz
https://hg.mozilla.org/integration/mozilla-inbound/rev/e834e810a3fa
Part 7: Re-apply - Update chromium's list of linux-x86-32 syscalls. r=jld
https://hg.mozilla.org/integration/mozilla-inbound/rev/32f4d07e9d99
Part 8: Change the USER_NON_ADMIN token to be a restricted token with the same access. r=aklotz

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d7d0c8ff20f1
https://hg.mozilla.org/mozilla-central/rev/23c95e36d1be
https://hg.mozilla.org/mozilla-central/rev/a46f0e32289b
https://hg.mozilla.org/mozilla-central/rev/00d1d90c348b
https://hg.mozilla.org/mozilla-central/rev/a05726163a79
https://hg.mozilla.org/mozilla-central/rev/9069b89a690b
https://hg.mozilla.org/mozilla-central/rev/e834e810a3fa
https://hg.mozilla.org/mozilla-central/rev/32f4d07e9d99