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

RESOLVED FIXED in Firefox 51

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bobowen, Assigned: bobowen)

Tracking

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

(Whiteboard: sbwc2, sblc2)

Attachments

(8 attachments, 1 obsolete attachment)

1.37 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
879 bytes, patch
jimm
: review+
Details | Diff | Splinter Review
2.32 MB, patch
aklotz
: review+
jld
: review+
Details | Diff | Splinter Review
2.84 KB, patch
bobowen
: review+
Details | Diff | Splinter Review
28.29 KB, patch
bobowen
: review+
Details | Diff | Splinter Review
7.10 KB, patch
bobowen
: review+
Details | Diff | Splinter Review
2.99 KB, patch
bobowen
: review+
Details | Diff | Splinter Review
3.47 KB, patch
aklotz
: review+
Details | Diff | Splinter Review
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
Created attachment 8772450 [details] [diff] [review]
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
Attachment #8772450 - Flags: review?(ehsan)
Created attachment 8772452 [details] [diff] [review]
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
Attachment #8772452 - Flags: review?(jmathies)
Created attachment 8772454 [details] [diff] [review]
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
Attachment #8772454 - Flags: review?(jld)
Attachment #8772454 - Flags: review?(aklotz)
Created attachment 8772458 [details] [diff] [review]
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
Created attachment 8772459 [details] [diff] [review]
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
Created attachment 8772460 [details] [diff] [review]
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
Created attachment 8772461 [details] [diff] [review]
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

Updated

2 years ago
Attachment #8772450 - Flags: review?(ehsan) → review+
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.
Attachment #8772454 - Flags: review?(aklotz) → review+
(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?
Flags: needinfo?(aklotz)

Updated

2 years ago
Attachment #8772452 - Flags: review?(jmathies) → review+
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.
Attachment #8772454 - Flags: review?(jld) → review+
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.
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.
Created attachment 8781162 [details] [diff] [review]
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
Attachment #8781162 - Flags: review?(aklotz)
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?
Flags: needinfo?(aklotz) → needinfo?(bobowen.code)
(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.
(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.
Flags: needinfo?(bobowen.code) → needinfo?(aklotz)
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?
Attachment #8781162 - Flags: review?(aklotz)
Created attachment 8787663 [details] [diff] [review]
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
Attachment #8787663 - Flags: review?(aklotz)
Attachment #8781162 - Attachment is obsolete: true

Updated

2 years ago
Attachment #8787663 - Flags: review?(aklotz) → review+
OK, let's go with that.
Flags: needinfo?(aklotz)
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
Attachment #8772458 - Flags: review+
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
Attachment #8772459 - Flags: review+
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
Attachment #8772460 - Flags: review+
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
Attachment #8772461 - Flags: review+

Comment 25

2 years ago
Pushed by bobowencode@gmail.com:
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
Depends on: 1303325
Blocks: 1271890
Blocks: 1275813
Blocks: 1337331
You need to log in before you can comment on or make changes to this bug.