Closed Bug 1639030 Opened 5 years ago Closed 4 years ago

Update //security/sandbox/chromium/ to Chromium's latest stable version

Categories

(Core :: Security: Process Sandboxing, enhancement, P3)

Product:
Core
Component:
Security: Process Sandboxing
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla80
Tracking Flags:
Tracking Status
firefox80 --- fixed

People

(Reporter: toshi, Assigned: toshi)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 16 obsolete files)

Bug 1639030 - Part 1: Update with_update and after_update patches. r=bobowen
47 bytes, text/x-phabricator-request
Details | Review
Bug 1639030 - Part 2: Roll-up of chromium sandbox update and patches to get a running browser. r=bobowen
47 bytes, text/x-phabricator-request
Details | Review
Bug 1639030 - Part 3: Roll-up patch to apply remaining mozilla changes to chromium sandbox. r=bobowen
47 bytes, text/x-phabricator-request
Details | Review

The current target is 81.0.4044.138.

Blocks: 1620114
Priority: -- → P3

This patch updates exising files under //security/sandbox/chromium/base and
//security/sandbox/chromium/build to Chromium 81.0.4044.138:
https://chromium.googlesource.com/chromium/src.git/+/0085b3faa4477bd52f03aeb1ee1097fa54a1bd55

Newly-introduced files will be brought by subsequent patches.

This patch updates exising files under //security/sandbox/chromium/sandbox/win
to Chromium 81.0.4044.138:
https://chromium.googlesource.com/chromium/src.git/+/0085b3faa4477bd52f03aeb1ee1097fa54a1bd55

Newly-introduced files will be brought by subsequent patches.

Depends on D76220

This patch updates exising files under //security/sandbox/chromium/sandbox/linux
to Chromium 81.0.4044.138:
https://chromium.googlesource.com/chromium/src.git/+/0085b3faa4477bd52f03aeb1ee1097fa54a1bd55

Depends on D76222

Why do we need //base/hash/hash.h&cc and //base/third_party/cityhash?

The hash code was moved from //base to //base/hash by
https://source.chromium.org/chromium/chromium/src/+/c13e94667e4ad876254356a57093afdee8a69ea7
We also need CityHash as it was introduced by
https://source.chromium.org/chromium/chromium/src/+/a34848741044c02d8730f9948ef82d4fb11c8a6c
and it is used from //base/win/scoped_handle_verifier.h, added by
https://source.chromium.org/chromium/chromium/src/+/028b47fba8bf0ec17f8dbb234ebf3a9c4d352071

Why do we need //base/third_party/double_conversion?

It replaced //base/third_party/dmg_fp as
https://source.chromium.org/chromium/chromium/src/+/75b27377203ce5aff439ef3387ae82d4054611c3

Why do we need //sandbox/win/src/signed_* files?

It's an essential part to achieve CIG whitelist, delegating NtCreateSection
to the browser process.

Why do we need //base/memory/* files?

It's a new Shared Memory API.
base::SharedMemory and base::SharedMemoryHandle were removed by
https://source.chromium.org/chromium/chromium/src/+/cf96655e1678b212cb696b5e8ed4095cc2fd6052
instead, a new Shared Memory API was introduced by
https://source.chromium.org/chromium/chromium/src/+/cdd8661f3f35d7075020ead0492cd83e18f795e9
We need this because ProcessMitigationsWin32KDispatcher consumes it by
https://source.chromium.org/chromium/chromium/src/+/68e88163e04dc2158df5f76ea4a7fa8293109437

Depends on D76223

Why adding include base/macros.h in //base/feature_list.h?
Now feature_list.h is compiled and DISALLOW_COPY_AND_ASSIGN needs to be available.

Why is //base/observer_list.h empty?
It was added by the following commit, but it's actually unnecessary. Just need a file to compile.
https://source.chromium.org/chromium/chromium/src/+/2538661e0790480b58b14d9c7ac29eb7741ab8d8

What is //base/logging_buildflags.h?
It is a new generated header introduced by the following commit.
https://source.chromium.org/chromium/chromium/src/+/3553b590b10065867cc4d349e1485e5c8478e68a

Why adding BUILDFLAG_INTERNAL_USE_TCMALLOC in //base/allocator/buildflags.h?
It replaced the global flag defined in //build/build_config.h as
https://source.chromium.org/chromium/chromium/src/+/b80b085bfabdbeccf5688a1c0291812c577b854a
We use this switch in //base/debug/profiler.cc.

Depends on D76224

Removing the following patches from with_update no longer needed.

  1. update_chromium_linux_x86_syscalls.patch is included in
    https://chromium.googlesource.com/chromium/src.git/+/b4f3df4e77ba1f79717d44b5068cb36f7537332f

  2. ifdef_out_ApplyMitigationsToCurrentThread.patch cannot be used because
    we use ApplyMitigationsToCurrentThread since the following commit.
    https://chromium.googlesource.com/chromium/src.git/+/4bed2eb502974fe665ff7750a1ba4e45132524ad

  3. mingw_base_win_get_caller.patch is included in
    https://chromium.googlesource.com/chromium/chromium/src/+/d8b73eb8f0321bf37931b69c66fdad82ff7063c5

  4. fix_incorrect_int_use_in_Kernel32BaseVersion.patch is fixed by
    https://hg.mozilla.org/mozilla-central/rev/dc9d71fb3bac807a37dbfba35d609ac4ffff1980

  5. revert_removal_of_AlterEnvironment_on_Windows.patch is altered by adding
    environment_internal.h/cc as a different commit.

  6. mingw_undefine_MemoryBarrier.patch is no longer needed as
    base::subtle::MemoryBarrier was removed by
    https://chromium.googlesource.com/chromium/chromium/src/+/bdbaaf4e7e0318fca0cadd1407de810a71309625

  7. public_siginfo_fields.patch is included in
    https://chromium.googlesource.com/chromium/chromium/src/+/6bd491daaf28a8281136931133504c23a18f819f

Depends on D76225

This patch removes the use of base::ScopedNativeLibrary from
sandbox::ApplyMitigationsToCurrentThread to avoid new dependencies.

Depends on D76228

Need the following commit to compile with Mingw, which has not reached
the stable channel yet.
https://chromium.googlesource.com/chromium/src.git/+/1620fe70c299f1f18b2f2c652d16739f6e3c5f78

Depends on D76235

The following commit caused an Mingw build failure because delayimp.h is included twice,
from pe_image.h and pe_image.cc. This patch removes the second include.
https://chromium.googlesource.com/chromium/src.git/+/5c23d46846111ea16aaf2a9b45355cca5ddbf6d8

Depends on D76236

We still use 10.0.17134.0 SDK while Chromium requires 10.0.18362.0 or higher.

Depends on D76237

Below is the list of patches needs to be reviewed, and the latest Try job is this. When review is done, I'll merge these into a few patches so that any commit can be compiled.

Outside chromium

  • Bug 1639030 - Don't use clang's new pass manager for filesystem_interception.cc. r=dmajor
  • Bug 1639030 - Make sure setting MITIGATION_WIN32K_DISABLE before adding SUBSYS_WIN32K_LOCKDOWN. r=bobowen

New with_update patches

  • Bug 1639030 - [with_update] Lower SDK version requirement from 19H1 to RS4. r=bobowen
  • Bug 1639030 - [with_update] Don't include delayimp.h twice from //base/win/pe_image.cc to compile with Mingw. r=bobowen
  • Bug 1639030 - [with_update] Remove Extraneous Backslash Introduced by clang-tidy in ScopedHandle. r=bobowen
  • Bug 1639030 - [with_update] Remove unused functions from //base/third_party/double_conversion to compile. r=bobowen
  • Bug 1639030 - [with_update] Revert unused base::Token serialization to minimize dependency. r=bobowen

Updated with_update patches

  • Bug 1639030 - [with_update] Use GetModuleHandle/GetProcAddress in ApplyMitigationsToCurrentThread. r=bobowen
  • Bug 1639030 - [with_update] Reinstate sandbox::TargetServices::BrokerDuplicateHandle. r=bobowen
  • Bug 1639030 - [with_update] Reinstate BrokerServicesBase::IsActiveTarget and AddTargetPeer. r=bobowen

Add/Remove files

  • Bug 1639030 - Part 6: Update with_update and after_update patches. r=bobowen
  • Bug 1639030 - Part 5: Update chromium-shim files. r=bobowen
  • Bug 1639030 - Part 4: Add new files and remove unused files from Chromium. r=bobowen
  • Bug 1639030 - Part 3: Roll-up of chromium sandbox files under //sandbox/linux. r=jld
  • Bug 1639030 - Part 2: Roll-up of chromium sandbox files under //sandbox/win. r=bobowen
  • Bug 1639030 - Part 1: Roll-up of chromium sandbox files under //base/ and //build/. r=bobowen
See Also: → 1645184
Attachment #9150610 - Attachment description: Bug 1639030 - Don't use clang's new pass manager for filesystem_interception.cc. r=dmajor → Bug 1639030 - Disable tail merging for filesystem_interception.cc to work around Clang's bug. r=dmajor
Attachment #9150589 - Attachment is obsolete: true
Attachment #9150592 - Attachment is obsolete: true
Attachment #9150593 - Attachment is obsolete: true
Attachment #9150595 - Attachment is obsolete: true
Attachment #9150597 - Attachment is obsolete: true
Attachment #9150594 - Attachment is obsolete: true
Attachment #9150596 - Attachment is obsolete: true
Attachment #9150598 - Attachment is obsolete: true
Attachment #9150599 - Attachment is obsolete: true
Attachment #9150603 - Attachment is obsolete: true
Attachment #9150605 - Attachment is obsolete: true
Attachment #9150606 - Attachment is obsolete: true
Attachment #9150607 - Attachment is obsolete: true
Attachment #9150608 - Attachment is obsolete: true
Attachment #9150609 - Attachment is obsolete: true
Attachment #9150610 - Attachment is obsolete: true

This commit updates files under security/sandbox/chromium-shim/patches/
to prepare our codebase for Chromium sandbox update. See patch files for
the details of each patch.

This also removes the following patches from with_update no longer needed.

  1. update_chromium_linux_x86_syscalls.patch is included in
    https://chromium.googlesource.com/chromium/src.git/+/b4f3df4e77ba1f79717d44b5068cb36f7537332f
  2. ifdef_out_ApplyMitigationsToCurrentThread.patch cannot be used because
    we use ApplyMitigationsToCurrentThread since the following commit.
    https://chromium.googlesource.com/chromium/src.git/+/4bed2eb502974fe665ff7750a1ba4e45132524ad
  3. mingw_base_win_get_caller.patch is included in
    https://chromium.googlesource.com/chromium/chromium/src/+/d8b73eb8f0321bf37931b69c66fdad82ff7063c5
  4. fix_incorrect_int_use_in_Kernel32BaseVersion.patch is fixed by
    https://hg.mozilla.org/mozilla-central/rev/dc9d71fb3bac807a37dbfba35d609ac4ffff1980
  5. revert_removal_of_AlterEnvironment_on_Windows.patch is altered by adding
    environment_internal.h/cc as a different commit.
  6. mingw_undefine_MemoryBarrier.patch is no longer needed as
    base::subtle::MemoryBarrier was removed by
    https://chromium.googlesource.com/chromium/chromium/src/+/bdbaaf4e7e0318fca0cadd1407de810a71309625
  7. public_siginfo_fields.patch is included in
    https://chromium.googlesource.com/chromium/chromium/src/+/6bd491daaf28a8281136931133504c23a18f819f

This commit does:

  • Sync files under security/sandbox/chromium/ with Chromium 81.0.4044.138
  • Update files under security/sandbox/chromium-shim/
  • Apply patches under security/sandbox/chromium-shim/patches/with_update/
  • Apply a workaround for Clang's bug to compile filesystem_interception.cc
  • Add mozilla::AddWin32kLockdownPolicy to apply MITIGATION_WIN32K_DISABLE before SUBSYS_WIN32K_LOCKDOWN

Depends on D79558

This commit applies patches under security/sandbox/chromium-shim/patches/after_update/.

Depends on D79560

Pushed by cbrindusan@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9d82c8fa3d3b Part 1: Update with_update and after_update patches. r=bobowen https://hg.mozilla.org/integration/autoland/rev/0c2d7e8a4131 Part 2: Roll-up of chromium sandbox update and patches to get a running browser. r=bobowen https://hg.mozilla.org/integration/autoland/rev/55b963f34eb0 Part 3: Roll-up patch to apply remaining mozilla changes to chromium sandbox. r=bobowen

Backed out 3 changesets (bug 1639030) for sandbox related bustages.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=build&fromchange=561694a246a0e12f9c261fca58ce26b8767e6e3b&tochange=50324c6ce855b56eb4bd4712ff7924023f045d49&selectedTaskRun=dAgFr4UgRoWCKvh_I3BeLA.0

Backout link: https://hg.mozilla.org/integration/autoland/rev/50324c6ce855b56eb4bd4712ff7924023f045d49

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=306392581&repo=autoland&lineNumber=18792

[task 2020-06-15T17:36:32.780Z] 17:36:32     INFO -  make[4]: Entering directory '/builds/worker/workspace/obj-build/security/sandbox'
[task 2020-06-15T17:36:32.780Z] 17:36:32     INFO -  /builds/worker/fetches/sccache/sccache /builds/worker/fetches/clang/bin/clang-cl -Xclang -std=c++17 --target=aarch64-windows-msvc -Fowin_utils.obj -c  -guard:cf -DNDEBUG=1 -DTRIMMED=1 -DUNICODE -D_UNICODE -DNS_NO_XPCOM -D_CRT_RAND_S -DCHROMIUM_SANDBOX_BUILD -DSANDBOX_EXPORTS -I/builds/worker/checkouts/gecko/security/sandbox -I/builds/worker/workspace/obj-build/security/sandbox -I/builds/worker/checkouts/gecko/security/sandbox/chromium-shim -I/builds/worker/checkouts/gecko/security/sandbox/chromium -I/builds/worker/checkouts/gecko/nsprpub -I/builds/worker/workspace/obj-build/dist/include -I/builds/worker/workspace/obj-build/dist/include/nspr -I/builds/worker/workspace/obj-build/dist/include/nss -MD -FI /builds/worker/workspace/obj-build/mozilla-config.h -DMOZILLA_CLIENT -Qunused-arguments -Qunused-arguments -fcrash-diagnostics-dir=/builds/worker/artifacts -TP -Zc:sizedDealloc- -D_HAS_EXCEPTIONS=0 -W3 -Gy -Zc:inline -Gw -Wno-inline-new-delete -Wno-invalid-offsetof -Wno-microsoft-enum-value -Wno-microsoft-include -Wno-unknown-pragmas -Wno-ignored-pragmas -Wno-deprecated-declarations -Wno-invalid-noreturn -Wno-inconsistent-missing-override -Wno-implicit-exception-spec-mismatch -Wno-microsoft-exception-spec -Wno-unused-local-typedef -Wno-ignored-attributes -Wno-used-but-marked-unused -D_SILENCE_TR1_NAMESPACE_DEPRECATION_WARNING -GR- -Z7 -Xclang -load -Xclang /builds/worker/workspace/obj-build/build/clang-plugin/libclang-plugin.so -Xclang -add-plugin -Xclang moz-check -O2 -Oy- -Werror -Wno-deprecated-declarations  -Xclang -MP -Xclang -dependency-file -Xclang .deps/win_utils.obj.pp -Xclang -MT -Xclang win_utils.obj   /builds/worker/checkouts/gecko/security/sandbox/chromium/sandbox/win/src/win_utils.cc
[task 2020-06-15T17:36:32.780Z] 17:36:32     INFO -  /builds/worker/checkouts/gecko/security/sandbox/chromium/sandbox/win/src/win_utils.cc(562,32): error: static_cast from 'PRTL_USER_PROCESS_PARAMETERS' (aka '_RTL_USER_PROCESS_PARAMETERS *') to 'uint8_t *' (aka 'unsigned char *') is not allowed
[task 2020-06-15T17:36:32.780Z] 17:36:32     INFO -    uint8_t* processParameters = static_cast<uint8_t*>(peb.ProcessParameters);
[task 2020-06-15T17:36:32.780Z] 17:36:32     INFO -                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[task 2020-06-15T17:36:32.780Z] 17:36:32     INFO -  1 error generated.
[task 2020-06-15T17:36:32.780Z] 17:36:32    ERROR -  make[4]: *** [/builds/worker/checkouts/gecko/config/rules.mk:748: win_utils.obj] Error 1
[task 2020-06-15T17:36:32.780Z] 17:36:32     INFO -  make[4]: Leaving directory '/builds/worker/workspace/obj-build/security/sandbox'
[task 2020-06-15T17:36:32.780Z] 17:36:32    ERROR -  make[3]: *** [/builds/worker/checkouts/gecko/config/recurse.mk:74: security/sandbox/target-objects] Error 2
[task 2020-06-15T17:36:32.780Z] 17:36:32     INFO -  make[3]: *** Waiting for unfinished jobs....
Flags: needinfo?(tkikuchi)

Thank you for letting me know. Both issues, build error on Arm64 and crash on ASAN, have been resolved.

Let us hold off landing for a while and target version 80.

We're hitting a new compiler issue on the x64 shippable build, that has nothing to do with the new pass manager.

See Also: → 1650419
Flags: needinfo?(tkikuchi)
Pushed by dluca@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2c6cfddbc1af Part 1: Update with_update and after_update patches. r=bobowen https://hg.mozilla.org/integration/autoland/rev/dc961d2004fd Part 2: Roll-up of chromium sandbox update and patches to get a running browser. r=bobowen https://hg.mozilla.org/integration/autoland/rev/d7bd92ae8de6 Part 3: Roll-up patch to apply remaining mozilla changes to chromium sandbox. r=bobowen

Needed to update security/sandbox/linux/moz.build.

Flags: needinfo?(tkikuchi)
Pushed by btara@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c40a82e96834 Part 1: Update with_update and after_update patches. r=bobowen https://hg.mozilla.org/integration/autoland/rev/e93c2e3b1e62 Part 2: Roll-up of chromium sandbox update and patches to get a running browser. r=bobowen https://hg.mozilla.org/integration/autoland/rev/e614d160ab92 Part 3: Roll-up patch to apply remaining mozilla changes to chromium sandbox. r=bobowen
Depends on: 1650419
See Also: 1650419 →
Flags: needinfo?(tkikuchi)
Pushed by csabou@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8d50ff6a9f05 Part 1: Update with_update and after_update patches. r=bobowen https://hg.mozilla.org/integration/autoland/rev/0b1d280dee6f Part 2: Roll-up of chromium sandbox update and patches to get a running browser. r=bobowen https://hg.mozilla.org/integration/autoland/rev/4f038b3c182d Part 3: Roll-up patch to apply remaining mozilla changes to chromium sandbox. r=bobowen

https://hg.mozilla.org/mozilla-central/rev/8d50ff6a9f05
https://hg.mozilla.org/mozilla-central/rev/0b1d280dee6f
https://hg.mozilla.org/mozilla-central/rev/4f038b3c182d

Status: NEW → RESOLVED
Closed: 4 years ago
status-firefox80: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla80

It looks like this requires a clobber; at least on my Windows system, I'm seeing a build failure in security/sandbox/chromium/base, apparently because hash.cc moved into a subdirectory. (Maybe other reasons too, but that's where it dies for me.)

It would have been nice to touch the CLOBBER file as part of the landing here.

Regressions: 1651756

(In reply to Jonathan Kew (:jfkthame) from comment #33)

It looks like this requires a clobber; at least on my Windows system, I'm seeing a build failure in security/sandbox/chromium/base, apparently because hash.cc moved into a subdirectory. (Maybe other reasons too, but that's where it dies for me.)

It would have been nice to touch the CLOBBER file as part of the landing here.

Right, thank you heads-up. Another solution is to remove all files under $MOZ_OBJDIR/security/sandbox manually. I think that's faster than CLOBBER :).

I'm making this block mach-busted since that's the first place to look for known build failures.

Blocks: mach-busted
Regressions: 1658429
No longer regressions: 1658429
Regressions: 1668876
Blocks: 1918069
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: