mac: Pad the unsigned official branded x86_64 main executable to ~257kB

Seriously.

When the main executable is updated on disk while the application is
running, and the offset of the Mach-O image at the main executable’s
path changes from the offset that was determined when the executable was
loaded, SecCode ceases to be able to work with the executable. This may
be triggered when the product is updated on disk but the application has
not yet relaunched. This affects SecCodeCopySelf and
SecCodeCopyGuestWithAttributes. Bugs are evident even when validation
(SecCodeCheckValidity) is not attempted.

Practically, this is only a concern for fat (universal) files, because
the offset of a Mach-O image in a thin (single-architecture) file is
always 0. The branded product always ships a fat executable, and because
some uses of SecCode are in OS code beyond Chrome’s control, an effort
is made to freeze the geometry of the branded (is_chrome_branded)
for-public-release (is_official_build) main executable.

The fat file is produced by installer/mac/universalizer.py. The x86_64
slice always precedes the arm64 slice: lipo, as used by
universalizer.py, always places the arm64 slice last. See Xcode 12.0
https://github.com/apple-oss-distributions/cctools/blob/cctools-973.0.1/misc/lipo.c#L2672
cmp_qsort, used by create_fat at #L962. universalizer.py ensures that
the first slice in the file is located at a constant offset (16kB since
98.0.4758.80), but if the first slice’s size changes, it can affect the
offset of the second slice, the arm64 one, triggering SecCode-related
bugs for arm64 users across updates.

As quite a hack of a workaround, the offset of the arm64 slice within
the fat main executable is fixed at a constant value by introducing
padding to the x86_64 slice that precedes it. The arm64 slice needs to
remain at offset 304kB (since 98.0.4758.80), so enough padding is added
to the x86_64 slice to ensure that the arm64 slice lands where it needs
to be when universalized. This padding needs to be added to the thin
form of the x86_64 image before being fed to universalizer.py.

To make things extra tricky, the final size of the x86_64 image is not
known when it is linked, because code signing will contribute more data
to the file. Typically, official code signing adds a non-constant
22-23kB. The non-constancy makes a precise computation of the required
padding at this stage of the build impossible. Luckily, the size of the
x86_64 image doesn’t need to be so precise. The arm64 slice that follows
it in the fat file will be 16kB-aligned, so it’s enough to ensure that
the x86_64 slice ends at an offset in the range (288kB, 304kB], or,
since the x86_64 slice itself begins at offset 16kB, its total size once
signed must be in the range (272kB, 288kB]. Targeting size 280kB, right
in the middle of that range, and assuming an expected 23200-byte
contribution from code signing, the unsigned x86_64 image should be
padded to 263520 bytes. Code signing may then add any amount in the
range (15008 bytes, 31392 bytes] and the result, when universalized,
will have its arm64 slice at the required 304kB offset.

This introduces //build/toolchain/apple/pad_linkedit.py to insert the
padding, which can be used by passing -Wcrl,pad_linkedit to
linker_driver.py, and uses it when linking the x86_64 main executable in
branded official builds. pad_linkedit.py can only increase the size of
the image. It will raise an error on an attempt to decrease the size.
Fortunately, the x86_64 main executable in this build configuration is
currently smaller than this size, and is expected to shrink even further
in the future.

Bug: 1300598, 1295098
Change-Id: Ib1b92c45ecae3d38752eb56103e436032e818c03
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3488454
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Commit-Queue: Mark Mentovai <mark@chromium.org>
Cr-Commit-Position: refs/heads/main@{#974951}

build/toolchain/apple/linker_driver.py

@@ -51,9 +51,17 @@
 # -Wcrl,strippath,<strip_path>
 #    Sets the path to the strip to run with -Wcrl,strip, in which case
 #    `xcrun` is not used to invoke it.
+#
+# -Wcrl,pad_linkedit,<size>
+#    Pads the total size of the linker's output to the specified size in bytes,
+#    after running the linker and `strip`. Padding is added to the __LINKEDIT
+#    segment by the pad_linkedit.py script located in the same directory as this
+#    script. It is an error to attempt to reduce the size of the linked image
+#    using this option.
 
 
 class LinkerDriver(object):
+
     def __init__(self, args):
         """Creates a new linker driver.
 
@@ -74,6 +82,7 @@ def __init__(self, args):
             ('unstripped,', self.run_save_unstripped),
             ('strippath,', self.set_strip_path),
             ('strip,', self.run_strip),
+            ('pad_linkedit,', self.run_pad_linkedit),
         ]
 
         # Linker driver actions can modify the these values.
@@ -278,6 +287,21 @@ def set_strip_path(self, strip_path):
         self._strip_cmd = [strip_path]
         return []
 
+    def run_pad_linkedit(self, size_string):
+        """Linker driver action for -Wcrl,pad_linkedit,<size>.
+
+        Args:
+            size_string: string, the size to pass to pad_linkedit.py.
+
+        Returns:
+            list of string, Build step outputs.
+        """
+        pad_linkedit_command = (os.path.join(os.path.dirname(__file__),
+                                             'pad_linkedit.py'),
+                                self._get_linker_output(), size_string)
+        subprocess.check_call(pad_linkedit_command)
+        return []
+
 
 # Regular expressions matching log spam messages from dsymutil.
 DSYM_SPURIOUS_PATTERNS = [

build/toolchain/apple/pad_linkedit.py

@@ -0,0 +1,175 @@
+#!/usr/bin/env python3
+# coding: utf-8
+
+# Copyright 2022 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+# Increases the size of a Mach-O image by adding padding to its __LINKEDIT
+# segment.
+
+import argparse
+import os
+import struct
+import sys
+
+# Constants from <mach-o/loader.h>.
+_MH_MAGIC = 0xfeedface
+_MH_MAGIC_64 = 0xfeedfacf
+_LC_SEGMENT = 0x1
+_LC_SEGMENT_64 = 0x19
+_LC_CODE_SIGNATURE = 0x1d
+_SEG_LINKEDIT = b'__LINKEDIT'.ljust(16, b'\x00')
+
+
+class PadLinkeditError(Exception):
+    pass
+
+
+def _struct_read_unpack(file, format_or_struct):
+    """Reads bytes from |file|, unpacking them via the struct module. This
+    function is provided for convenience: the number of bytes to read from
+    |file| is determined based on the size of data that the struct unpack
+    operation will consume.
+
+    Args:
+        file: The file object to read from.
+
+        format_or_struct: A string suitable for struct.unpack’s |format|
+            argument, or a struct.Struct object. This will be used to determine
+            the number of bytes to read and to perform the unpack.
+
+    Returns:
+        A tuple of unpacked items.
+    """
+
+    if isinstance(format_or_struct, struct.Struct):
+        struc = format_or_struct
+        return struc.unpack(file.read(struc.size))
+
+    format = format_or_struct
+    return struct.unpack(format, file.read(struct.calcsize(format)))
+
+
+def PadLinkedit(file, size):
+    """Takes |file|, a single-architecture (thin) Mach-O image, and increases
+    its size to |size| by adding padding (NUL bytes) to its __LINKEDIT segment.
+    If |file| is not a thin Mach-O image, if its structure is unexpected, if it
+    is already larger than |size|, or if it is already code-signed, raises
+    PadLinkeditError.
+
+    The image must already have a __LINKEDIT segment, the load command for the
+    __LINKEDIT segment must be the last segment load command in the image, and
+    the __LINKEDIT segment contents must be at the end of the file.
+
+    Args:
+        file: The file object to read from and modify.
+
+        size: The desired size of the file.
+
+    Returns:
+        None
+
+    Raises:
+        PadLinkeditError if |file| is not suitable for the operation.
+    """
+
+    file.seek(0, os.SEEK_END)
+    current_size = file.tell()
+    file.seek(0, os.SEEK_SET)
+
+    magic, = _struct_read_unpack(file, '<I')
+    if magic == _MH_MAGIC_64:
+        bits, endian = 64, '<'
+    elif magic == _MH_MAGIC:
+        bits, endian = 32, '<'
+    elif magic == struct.unpack('>I', struct.pack('<I', _MH_MAGIC_64)):
+        bits, endian = 64, '>'
+    elif magic == struct.unpack('>I', struct.pack('<I', _MH_MAGIC)):
+        bits, endian = 32, '>'
+    else:
+        raise PadLinkeditError('unrecognized magic', magic)
+
+    if bits == 64:
+        lc_segment = _LC_SEGMENT_64
+        segment_command_struct = struct.Struct(endian + '16s4Q4I')
+    else:
+        lc_segment = _LC_SEGMENT
+        segment_command_struct = struct.Struct(endian + '16s8I')
+
+    grow = size - current_size
+    if grow < 0:
+        raise PadLinkeditError('file would need to shrink', grow)
+
+    (cputype, cpusubtype, filetype, ncmds, sizeofcmds,
+     flags) = _struct_read_unpack(file,
+                                  endian + '6I' + ('4x' if bits == 64 else ''))
+
+    load_command_struct = struct.Struct(endian + '2I')
+    found_linkedit = False
+    segment_max_offset = 0
+
+    # Iterate through the load commands. It’s possible to consider |sizeofcmds|,
+    # but since the file is being edited in-place, that would just be a sanity
+    # check.
+    for load_command_index in range(ncmds):
+        cmd, cmdsize = _struct_read_unpack(file, load_command_struct)
+        consumed = load_command_struct.size
+        if cmd == lc_segment:
+            if found_linkedit:
+                raise PadLinkeditError('__LINKEDIT segment not last')
+
+            (segname, vmaddr, vmsize, fileoff, filesize, maxprot, initprot,
+             nsects, flags) = _struct_read_unpack(file, segment_command_struct)
+            consumed += segment_command_struct.size
+
+            if segname == _SEG_LINKEDIT:
+                found_linkedit = True
+
+                if fileoff < segment_max_offset:
+                    raise PadLinkeditError('__LINKEDIT data not last')
+                if fileoff + filesize != current_size:
+                    raise PadLinkeditError('__LINKEDIT data not at EOF')
+
+                vmsize += grow
+                filesize += grow
+                file.seek(-segment_command_struct.size, os.SEEK_CUR)
+                file.write(
+                    segment_command_struct.pack(segname, vmaddr, vmsize,
+                                                fileoff, filesize, maxprot,
+                                                initprot, nsects, flags))
+
+            segment_max_offset = max(segment_max_offset, fileoff + filesize)
+        elif cmd == _LC_CODE_SIGNATURE:
+            raise PadLinkeditError(
+                'modifying an already-signed image would render it unusable')
+
+        # Aside from the above, load commands aren’t being interpreted, or even
+        # read, so skip ahead to the next one.
+        file.seek(cmdsize - consumed, os.SEEK_CUR)
+
+    if not found_linkedit:
+        raise PadLinkeditError('no __LINKEDIT')
+
+    # Add the padding to the __LINKEDIT segment data.
+    file.seek(grow, os.SEEK_END)
+    file.truncate()
+
+
+def _main(args):
+    parser = argparse.ArgumentParser(
+        description=
+        'Increase the size of a Mach-O image by adding padding to its ' +
+        '__LINKEDIT segment.')
+    parser.add_argument('file', help='The Mach-O file to modify')
+    parser.add_argument('size',
+                        type=int,
+                        help='The desired final size of the file, in bytes')
+    parsed = parser.parse_args()
+
+    with open(parsed.file, 'r+b') as file:
+        PadLinkedit(file, parsed.size)
+
+
+if __name__ == '__main__':
+    sys.exit(_main(sys.argv[1:]))

chrome/BUILD.gn

@@ -485,21 +485,23 @@ if (is_win) {
       "//chrome/common:version_header",
     ]
 
+    ldflags = []
+
     if (enable_stripping) {
       # At link time, preserve the global symbols specified in the .exports
       # file. All other global symbols will be marked as private. The default
       # //build/config/mac:strip_all config will then remove the remaining
       # local and debug symbols.
-      ldflags = [ "-Wl,-exported_symbols_list," +
-                  rebase_path("app/app.exports", root_build_dir) ]
+      ldflags += [ "-Wl,-exported_symbols_list," +
+                   rebase_path("app/app.exports", root_build_dir) ]
     }
 
     if (is_component_build) {
       # In a component build, the framework is directly linked to the
       # executable because dlopen() and loading all the dependent dylibs
       # is time-consuming, see https://crbug.com/1197495.
       deps += [ ":chrome_framework+link" ]
-      ldflags = [ "-Wl,-rpath,@executable_path/../Frameworks" ]
+      ldflags += [ "-Wl,-rpath,@executable_path/../Frameworks" ]
 
       # The Framework is packaged inside the .app bundle. But when using the
       # component build, all the dependent shared libraries of :chrome_dll are
@@ -513,6 +515,70 @@ if (is_win) {
     if (enable_chromium_updater) {
       deps += [ ":chromium_updater_privileged_helper" ]
     }
+
+    if (is_chrome_branded && is_official_build && current_cpu == "x64") {
+      # This is for https://crbug.com/1300598, and more generally,
+      # https://crbug.com/1297588 (and all of the associated bugs). It's
+      # horrible!
+      #
+      # When the main executable is updated on disk while the application is
+      # running, and the offset of the Mach-O image at the main executable's
+      # path changes from the offset that was determined when the executable was
+      # loaded, SecCode ceases to be able to work with the executable. This may
+      # be triggered when the product is updated on disk but the application has
+      # not yet relaunched. This affects SecCodeCopySelf and
+      # SecCodeCopyGuestWithAttributes. Bugs are evident even when validation
+      # (SecCodeCheckValidity) is not attempted.
+      #
+      # Practically, this is only a concern for fat (universal) files, because
+      # the offset of a Mach-O image in a thin (single-architecture) file is
+      # always 0. The branded product always ships a fat executable, and because
+      # some uses of SecCode are in OS code beyond Chrome's control, an effort
+      # is made to freeze the geometry of the branded (is_chrome_branded)
+      # for-public-release (is_official_build) main executable.
+      #
+      # The fat file is produced by installer/mac/universalizer.py. The x86_64
+      # slice always precedes the arm64 slice: lipo, as used by
+      # universalizer.py, always places the arm64 slice last. See Xcode 12.0
+      # https://github.com/apple-oss-distributions/cctools/blob/cctools-973.0.1/misc/lipo.c#L2672
+      # cmp_qsort, used by create_fat at #L962. universalizer.py ensures that
+      # the first slice in the file is located at a constant offset (16kB since
+      # 98.0.4758.80), but if the first slice's size changes, it can affect the
+      # offset of the second slice, the arm64 one, triggering SecCode-related
+      # bugs for arm64 users across updates.
+      #
+      # As quite a hack of a workaround, the offset of the arm64 slice within
+      # the fat main executable is fixed at a constant value by introducing
+      # padding to the x86_64 slice that precedes it. The arm64 slice needs to
+      # remain at offset 304kB (since 98.0.4758.80), so enough padding is added
+      # to the x86_64 slice to ensure that the arm64 slice lands where it needs
+      # to be when universalized. This padding needs to be added to the thin
+      # form of the x86_64 image before being fed to universalizer.py.
+      #
+      # To make things extra tricky, the final size of the x86_64 image is not
+      # known when it is linked, because code signing will contribute more data
+      # to the file. Typically, official code signing adds a non-constant
+      # 22-23kB. The non-constancy makes a precise computation of the required
+      # padding at this stage of the build impossible. Luckily, the size of the
+      # x86_64 image doesn't need to be so precise. The arm64 slice that follows
+      # it in the fat file will be 16kB-aligned, so it's enough to ensure that
+      # the x86_64 slice ends at an offset in the range (288kB, 304kB], or,
+      # since the x86_64 slice itself begins at offset 16kB, its total size once
+      # signed must be in the range (272kB, 288kB]. Targeting size 280kB, right
+      # in the middle of that range, and assuming an expected 23200-byte
+      # contribution from code signing, the unsigned x86_64 image should be
+      # padded to 263520 bytes. Code signing may then add any amount in the
+      # range (15008 bytes, 31392 bytes] and the result, when universalized,
+      # will have its arm64 slice at the required 304kB offset.
+      #
+      # -Wcrl,pad_linkedit runs //build/toolchain/apple/pad_linkedit.py, and can
+      # only increase the size of the image. It will raise an error on an
+      # attempt to decrease the size. Fortunately, the x86_64 main executable in
+      # this build configuration is currently (at the time of this writing)
+      # smaller than this size, and is expected to shrink even further in the
+      # future.
+      ldflags += [ "-Wcrl,pad_linkedit,263520" ]
+    }
   }
 
   if (verify_dynamic_libraries) {