certificate_tag: flesh out.

With this change, the code should function. Only the superfluous
certificate mode of operation is supported.

Change-Id: I5e280e19c70169a0934f0815874cca68d2e61979
BUG: 1031734
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2013626Reviewed-by: Sorin Jianu <sorin@chromium.org>
Commit-Queue: Sorin Jianu <sorin@chromium.org>
Auto-Submit: Adam Langley <agl@chromium.org>
Cr-Commit-Position: refs/heads/master@{#738803}

chrome/updater/DEPS

@@ -11,4 +11,5 @@ include_rules = [
   "+courgette",
   "+third_party/crashpad",
   "+third_party/zlib/google",
+  "+third_party/boringssl/src/include",
 ]

chrome/updater/tools/BUILD.gn

@@ -40,5 +40,8 @@ source_set("unittest") {
     "//base",
     "//base/test:test_support",
     "//testing/gtest",
+    "//third_party/zlib/google:compression_utils",
   ]
+
+  data = [ "../test/data/signed.exe.gz" ]
 }

chrome/updater/tools/certificate_tag.cc

@@ -4,176 +4,492 @@
 
 #include "chrome/updater/tools/certificate_tag.h"
 
-#include <algorithm>
-#include <iostream>
-#include <iterator>
-#include <utility>
-
-#include "base/logging.h"
-#include "base/time/time.h"
+#include "base/containers/span.h"
+#include "base/optional.h"
+#include "third_party/boringssl/src/include/openssl/bytestring.h"
+#include "third_party/boringssl/src/include/openssl/crypto.h"
 
 namespace updater {
 namespace tools {
-namespace {
-
-// The number of bits in the RSA modulus of the key.
-constexpr int kRsaKeyBits = 2048;
-
-// kNotBeforeTime and kNotAfterTime are the validity period of the
-// certificate. They are deliberately set so that they are already expired.
-constexpr char kNotBeforeTime[] = "Mon Jan 1 10:00:00 UTC 2013";
-constexpr char kNotAfterTime[] = "Mon Apr 1 10:00:00 UTC 2013";
-
-// The structures here were taken from "Microsoft Portable Executable and
-// Common Object File Format Specification".
-constexpr int fileHeaderSize = 20;
-
-// FileHeader represents the IMAGE_FILE_HEADER structure (the COFF header
-// format) from
-// https://docs.microsoft.com/en-us/windows/win32/api/winnt/ns-winnt-image_file_header.
-struct FileHeader {
-  uint16_t machine;
-  uint16_t number_of_sections;
-  uint32_t time_date_stamp;
-  uint32_t pointer_for_symbol_table;
-  uint32_t number_of_symbols;
-  uint16_t size_of_optional_header;
-  uint16_t characteristics;
-};
-
-// OptionalHeader represents the IMAGE_OPTIONAL_HEADER structure from
-// https://docs.microsoft.com/en-us/windows/win32/api/winnt/ns-winnt-image_optional_header32.
-struct OptionalHeader {
-  uint16_t magic;
-  uint8_t major_linker_version;
-  uint8_t minor_linker_version;
-  uint32_t size_of_code;
-  uint32_t size_of_initialized_data;
-  uint32_t size_of_uninitialized_data;
-  uint32_t address_of_entry_point;
-  uint32_t base_of_code;
-};
-
-// DataDirectory represents the IMAGE_DATA_DIRECTORY structure from
-// https://docs.microsoft.com/en-us/windows/win32/api/winnt/ns-winnt-image_data_directory.
-struct dataDirectory {
-  uint32_t virtual_address;
-  uint32_t size;
-};
-
-// A subset of the known COFF "characteristic" flags found in
-// fileHeader.Characteristics.
-constexpr int kCoffCharacteristicExecutableImage = 2;
-constexpr int kCoffCharacteristicDLL = 0x2000;
-constexpr int kPE32Magic = 0x10b;
-constexpr int kPE32PlusMagic = 0x20b;
 
-// Certificate constants. See
-// https://docs.microsoft.com/en-us/windows/win32/api/wintrust/ns-wintrust-win_certificate.
-// Despite MSDN claiming that 0x100 is the only, current revision - in practice
-// it is 0x200.
-constexpr int kAttributeCertificateRevision = 0x200;
-constexpr int kAttributeCertificateTypePKCS7SignedData = 2;
-constexpr int kCertificateTableIndex = 4;
+// CBS is a structure from BoringSSL used for parsing binary and ASN.1-based
+// formats. This implementation detail is not exposed in the interface of this
+// code so these utility functions convert to/from base::span.
 
-}  // namespace
+static CBS CBSFromSpan(const base::span<const uint8_t>& span) {
+  CBS cbs;
+  CBS_init(&cbs, span.data(), span.size());
+  return cbs;
+}
 
-Binary::Binary() = default;
+static base::span<const uint8_t> SpanFromCBS(const CBS* cbs) {
+  return base::span<const uint8_t>(CBS_data(cbs), CBS_len(cbs));
+}
+
+Binary::Binary(const Binary&) = default;
 Binary::~Binary() = default;
 
-bool ParseUnixTime(const char* time_string, base::Time* parsed_time) {
-  return base::Time::FromUTCString(time_string, parsed_time);
-}
+// kTagOID contains the DER-serialised form of the extension OID that we stuff
+// the tag into: 1.3.6.1.4.1.11129.2.1.9999.
+static constexpr uint8_t kTagOID[] = {0x2b, 0x06, 0x01, 0x04, 0x01, 0xd6,
+                                      0x79, 0x02, 0x01, 0xce, 0x0f};
 
-std::vector<uint8_t> AppendedTag(const Binary& bin) {
-  const bool is_all_zero =
-      std::all_of(std::begin(bin.appended_tag), std::end(bin.appended_tag),
-                  [](uint8_t i) { return i == 0; });
-  if (is_all_zero && bin.appended_tag.size() < 8)
-    return {};
-  return bin.appended_tag;
-}
+// Certificate constants. See
+// http://msdn.microsoft.com/en-us/library/ms920091.aspx.
+//
+// Despite MSDN claiming that 0x100 is the only, current revision - in
+// practice it's 0x200.
+static constexpr uint16_t kAttributeCertificateRevision = 0x200;
+static constexpr uint16_t kAttributeCertificateTypePKCS7SignedData = 2;
 
-std::vector<uint8_t> RemoveAppendedTag(const Binary& bin) {
-  const auto appended_tag = AppendedTag(bin);
-  if (appended_tag.empty())
-    std::cerr << "authenticodetag: no appended tag found";
-  return BuildBinary(bin, bin.asn1_data, {});
-}
+// static
+base::Optional<Binary> Binary::Parse(base::span<const uint8_t> binary) {
+  // Parse establishes some offsets into |binary| for structures that |GetTag|
+  // and |SetTag| will both need.
 
-std::vector<uint8_t> SetAppendedTag(const Binary& bin,
-                                    const std::vector<uint8_t>& tag_contents) {
-  return BuildBinary(bin, bin.asn1_data, tag_contents);
-}
+  // kPEHeaderOffsetOffset is the offset into the binary where the offset of the
+  // PE header is found.
+  static constexpr size_t kPEHeaderOffsetOffset = 0x3c;
+  static constexpr uint32_t kPEMagic = 0x4550;  // "PE\x00\x00"
+
+  // These are a subset of the known COFF "characteristic" flags.
+  static constexpr uint16_t kCOFFCharacteristicExecutableImage = 2;
+  static constexpr uint16_t kCOFFCharacteristicDLL = 0x2000;
+
+  static constexpr int kPE32Magic = 0x10b;
+  static constexpr int kPE32PlusMagic = 0x20b;
+  static constexpr size_t kCertificateTableIndex = 4;
+  static constexpr size_t kFileHeaderSize = 20;
+
+  CBS bin = CBSFromSpan(binary);
+  CBS bin_for_offset = bin;
+  CBS bin_for_header = bin;
+  uint32_t pe_offset, pe_magic;
+  uint16_t size_of_optional_header, characteristics, optional_header_magic;
+  CBS optional_header;
+  // See the IMAGE_FILE_HEADER structure from
+  // http://msdn.microsoft.com/en-us/library/windows/desktop/ms680313(v=vs.85).aspx.
+  if (!CBS_skip(&bin_for_offset, kPEHeaderOffsetOffset) ||
+      !CBS_get_u32le(&bin_for_offset, &pe_offset) ||
+      !CBS_skip(&bin_for_header, pe_offset) ||
+      !CBS_get_u32le(&bin_for_header, &pe_magic) || pe_magic != kPEMagic ||
+      // http://msdn.microsoft.com/en-us/library/windows/desktop/ms680313(v=vs.85).aspx
+      !CBS_skip(&bin_for_header, 16) ||
+      !CBS_get_u16le(&bin_for_header, &size_of_optional_header) ||
+      !CBS_get_u16le(&bin_for_header, &characteristics) ||
+      (characteristics & kCOFFCharacteristicExecutableImage) == 0 ||
+      (characteristics & kCOFFCharacteristicDLL) != 0 ||
+      // See the IMAGE_OPTIONAL_HEADER structure from
+      // http://msdn.microsoft.com/en-us/library/windows/desktop/ms680339(v=vs.85).aspx.
+      !CBS_get_bytes(&bin_for_header, &optional_header,
+                     size_of_optional_header) ||
+      !CBS_get_u16le(&optional_header, &optional_header_magic)) {
+    return base::nullopt;
+  }
+
+  size_t address_size, extra_header_bytes = 0;
+  switch (optional_header_magic) {
+    case kPE32PlusMagic:
+      address_size = 8;
+      break;
+    case kPE32Magic:
+      address_size = 4;
+      // PE32 contains an additional field in the optional header.
+      extra_header_bytes = 4;
+      break;
+    default:
+      return base::nullopt;
+  }
+
+  // Skip the Windows-specific header section up until the number of data
+  // directory entries.
+  const size_t to_skip =
+      22 + extra_header_bytes + address_size + 40 + address_size * 4 + 4;
+  uint32_t num_directory_entries, cert_entry_virtual_addr, cert_entry_size;
+  if (!CBS_skip(&optional_header, to_skip) ||
+      // Read the number of directory entries, which is also the last value
+      // in the Windows-specific header.
+      !CBS_get_u32le(&optional_header, &num_directory_entries) ||
+      num_directory_entries > 4096 ||
+      num_directory_entries <= kCertificateTableIndex ||
+      !CBS_skip(&optional_header, kCertificateTableIndex * 8) ||
+      // See the IMAGE_DATA_DIRECTORY structure from
+      // http://msdn.microsoft.com/en-us/library/windows/desktop/ms680305(v=vs.85).aspx.
+      !CBS_get_u32le(&optional_header, &cert_entry_virtual_addr) ||
+      !CBS_get_u32le(&optional_header, &cert_entry_size) ||
+      static_cast<size_t>(cert_entry_virtual_addr) + cert_entry_size <
+          cert_entry_size ||
+      static_cast<size_t>(cert_entry_virtual_addr) + cert_entry_size !=
+          CBS_len(&bin)) {
+    return base::nullopt;
+  }
 
-int GetAttributeCertificates(const std::vector<uint8_t>& contents,
-                             size_t* offset,
-                             size_t* size,
-                             size_t* cert_size_offset) {
-  // TODO(agl): implement this. crbug.com/1031734.
+  CBS bin_for_certs = bin;
+  CBS certs;
+  if (!CBS_skip(&bin_for_certs, cert_entry_virtual_addr) ||
+      !CBS_get_bytes(&bin_for_certs, &certs, cert_entry_size)) {
+    return base::nullopt;
+  }
+
+  // See the WIN_CERTIFICATE structure from
+  // http://msdn.microsoft.com/en-us/library/ms920091.aspx.
+  uint32_t certs_len;
+  uint16_t revision, certs_type;
+  CBS signed_data;
+  const size_t expected_certs_len = CBS_len(&certs);
+  if (!CBS_get_u32le(&certs, &certs_len) || certs_len != expected_certs_len ||
+      !CBS_get_u16le(&certs, &revision) ||
+      revision != kAttributeCertificateRevision ||
+      !CBS_get_u16le(&certs, &certs_type) ||
+      certs_type != kAttributeCertificateTypePKCS7SignedData ||
+      !CBS_get_asn1_element(&certs, &signed_data, CBS_ASN1_SEQUENCE)) {
+    return base::nullopt;
+  }
+
+  Binary ret;
+  ret.certs_size_offset_ =
+      pe_offset + 4 + kFileHeaderSize + size_of_optional_header -
+      8 * (num_directory_entries - kCertificateTableIndex) + 4;
+
+  // Double-check that the calculated |certs_size_offset_| is correct by reading
+  // from that location and checking that the value is as expected.
+  uint32_t cert_entry_size_duplicate;
+  CBS bin_for_check = bin;
+  if (!CBS_skip(&bin_for_check, ret.certs_size_offset_) ||
+      !CBS_get_u32le(&bin_for_check, &cert_entry_size_duplicate) ||
+      cert_entry_size_duplicate != cert_entry_size) {
     NOTREACHED();
-  return -1;
+    return base::nullopt;
+  }
+
+  ret.binary_ = binary;
+  ret.content_info_ = SpanFromCBS(&signed_data);
+  ret.attr_cert_offset_ = cert_entry_virtual_addr;
+
+  if (!ret.ParseTag()) {
+    return base::nullopt;
+  }
+
+  return ret;
 }
 
-int ProcessAttributeCertificates(
-    const std::vector<uint8_t>& attribute_certificates,
-    std::vector<uint8_t>* asn1_data,
-    std::vector<uint8_t>* appended_tag) {
-  // TODO(agl): implement this. crbug.com/1031734.
-  NOTREACHED();
-  return -1;
+const base::Optional<base::span<const uint8_t>>& Binary::tag() const {
+  return tag_;
 }
 
-std::vector<uint8_t> SetSuperfluousCertTag(const Binary& bin,
-                                           const std::vector<uint8_t>& tag) {
-  // TODO(agl): implement this. crbug.com/1031734.
-  NOTREACHED();
-  return {};
+// AddName appends an X.500 Name structure to |cbb| containing a single
+// commonName with the given value.
+static bool AddName(CBB* cbb, const char* common_name) {
+  // kCommonName is the DER-enabled OID for common names.
+  static constexpr uint8_t kCommonName[] = {
+      0x55,
+      0x04,
+      0x03,
+  };
+
+  CBB name, rdns, rdn, oid, str;
+  if (!CBB_add_asn1(cbb, &name, CBS_ASN1_SEQUENCE) ||
+      !CBB_add_asn1(&name, &rdns, CBS_ASN1_SET) ||
+      !CBB_add_asn1(&rdns, &rdn, CBS_ASN1_SEQUENCE) ||
+      !CBB_add_asn1(&rdn, &oid, CBS_ASN1_OBJECT) ||
+      !CBB_add_bytes(&oid, kCommonName, sizeof(kCommonName)) ||
+      !CBB_add_asn1(&rdn, &str, CBS_ASN1_UTF8STRING) ||
+      !CBB_add_bytes(&str, reinterpret_cast<const uint8_t*>(common_name),
+                     strlen(common_name)) ||
+      !CBB_flush(cbb)) {
+    return false;
+  }
+
+  return true;
 }
 
-std::vector<uint8_t> BuildBinary(const Binary& bin,
-                                 const std::vector<uint8_t>& asn1_data,
-                                 const std::vector<uint8_t>& tag) {
-  // TODO(agl): implement this. crbug.com/1031734.
-  NOTREACHED();
-  return {};
+// CopyASN1 copies a single ASN.1 element from |in| to |out|.
+static bool CopyASN1(CBB* out, CBS* in) {
+  CBS element;
+  return CBS_get_any_asn1_element(in, &element, nullptr, nullptr) == 1 &&
+         CBB_add_bytes(out, CBS_data(&element), CBS_len(&element)) == 1;
+}
+
+base::Optional<std::vector<uint8_t>> Binary::SetTag(
+    base::span<const uint8_t> tag) const {
+  bssl::ScopedCBB cbb;
+  if (!CBB_init(cbb.get(), binary_.size() + 1024) ||
+      // Copy the binary up to the |WIN_CERTIFICATE| structure directly to the
+      // output.
+      !CBB_add_bytes(cbb.get(), binary_.data(), attr_cert_offset_) ||
+      // See the WIN_CERTIFICATE structure from
+      // http://msdn.microsoft.com/en-us/library/ms920091.aspx.
+      !CBB_add_u32(cbb.get(), 0 /* Length. Filled in later. */) ||
+      !CBB_add_u16le(cbb.get(), kAttributeCertificateRevision) ||
+      !CBB_add_u16le(cbb.get(), kAttributeCertificateTypePKCS7SignedData)) {
+    return base::nullopt;
+  }
+
+  // Walk the PKCS SignedData structure from the input and copy elements to the
+  // output until the list of certificates is reached.
+  CBS content_info = CBSFromSpan(content_info_);
+  CBS pkcs7, certs;
+  CBB content_info_cbb, outer_pkcs7_cbb, pkcs7_cbb, certs_cbb;
+  if (!CBS_get_asn1(&content_info, &content_info, CBS_ASN1_SEQUENCE) ||
+      !CBB_add_asn1(cbb.get(), &content_info_cbb, CBS_ASN1_SEQUENCE) ||
+      // See https://tools.ietf.org/html/rfc2315#section-7
+      // type
+      !CopyASN1(&content_info_cbb, &content_info) ||
+      !CBS_get_asn1(&content_info, &pkcs7,
+                    0 | CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC) ||
+      !CBB_add_asn1(&content_info_cbb, &outer_pkcs7_cbb,
+                    0 | CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC) ||
+      // See https://tools.ietf.org/html/rfc2315#section-9.1
+      !CBS_get_asn1(&pkcs7, &pkcs7, CBS_ASN1_SEQUENCE) ||
+      !CBB_add_asn1(&outer_pkcs7_cbb, &pkcs7_cbb, CBS_ASN1_SEQUENCE) ||
+      // version
+      !CopyASN1(&pkcs7_cbb, &pkcs7) ||
+      // digests
+      !CopyASN1(&pkcs7_cbb, &pkcs7) ||
+      // contentInfo
+      !CopyASN1(&pkcs7_cbb, &pkcs7) ||
+      !CBS_get_asn1(&pkcs7, &certs,
+                    0 | CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC) ||
+      !CBB_add_asn1(&pkcs7_cbb, &certs_cbb,
+                    0 | CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC)) {
+    return base::nullopt;
+  }
+
+  // Copy the certificates from the input to the output, potentially omitting
+  // the last one if it's a superfluous cert.
+  bool have_last_cert = false;
+  CBS last_cert;
+
+  while (CBS_len(&certs) > 0) {
+    if ((have_last_cert && !CBB_add_bytes(&certs_cbb, CBS_data(&last_cert),
+                                          CBS_len(&last_cert))) ||
+        !CBS_get_asn1_element(&certs, &last_cert, CBS_ASN1_SEQUENCE)) {
+      return base::nullopt;
+    }
+    have_last_cert = true;
+  }
+
+  // If there's not already a tag then we need to keep the last certificate.
+  // Otherwise it's the certificate with the tag in and we're going to replace
+  // it.
+  if (!tag_.has_value() &&
+      !CBB_add_bytes(&certs_cbb, CBS_data(&last_cert), CBS_len(&last_cert))) {
+    return base::nullopt;
+  }
+
+  // These values are DER-encoded OIDs needed in the X.509 certificate that's
+  // constructed below.
+  static constexpr uint8_t kSHA256WithRSA[] = {0x2a, 0x86, 0x48, 0x86, 0xf7,
+                                               0x0d, 0x01, 0x01, 0x0b};
+  static constexpr uint8_t kECPublicKey[] = {0x2a, 0x86, 0x48, 0xce,
+                                             0x3d, 0x02, 0x01};
+  static constexpr uint8_t kP256[] = {
+      0x2a, 0x86, 0x48, 0xce, 0x3d, 0x03, 0x01, 0x07,
+  };
+  static constexpr uint8_t kSHA256RSAEncryption[] = {
+      0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x01, 0x0b,
+  };
+  // kPublicKeyPoint is a X9.62, uncompressed P-256 point where x = 0.
+  static constexpr uint8_t kPublicKeyPoint[] = {
+      0x04, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+      0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+      0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+      0x66, 0x48, 0x5c, 0x78, 0x0e, 0x2f, 0x83, 0xd7, 0x24, 0x33, 0xbd,
+      0x5d, 0x84, 0xa0, 0x6b, 0xb6, 0x54, 0x1c, 0x2a, 0xf3, 0x1d, 0xae,
+      0x87, 0x17, 0x28, 0xbf, 0x85, 0x6a, 0x17, 0x4f, 0x93, 0xf4,
+  };
+
+  // Create a dummy certificate with an extension containing the tag. See
+  // https://tools.ietf.org/html/rfc5280#section-4.1 for the structure that's
+  // being created here.
+  CBB cert, tbs_cert, version, spki, sigalg, sigalg_oid, validity, not_before,
+      not_after, key_params, public_key, extensions_tag, extensions, extension,
+      critical_flag, tag_cbb, oid, null, signature;
+  uint8_t* cbb_data;
+  size_t cbb_len;
+  if (!CBB_add_asn1(&certs_cbb, &cert, CBS_ASN1_SEQUENCE) ||
+      !CBB_add_asn1(&cert, &tbs_cert, CBS_ASN1_SEQUENCE) ||
+      // version
+      !CBB_add_asn1(&tbs_cert, &version,
+                    CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 0) ||
+      !CBB_add_asn1_uint64(&version,
+                           2 /* version 3, because X.509 is off by one. */) ||
+      // serialNumber
+      !CBB_add_asn1_uint64(&tbs_cert, 1) ||
+      // signature algorithm
+      !CBB_add_asn1(&tbs_cert, &sigalg, CBS_ASN1_SEQUENCE) ||
+      !CBB_add_asn1(&sigalg, &sigalg_oid, CBS_ASN1_OBJECT) ||
+      !CBB_add_bytes(&sigalg_oid, kSHA256WithRSA, sizeof(kSHA256WithRSA)) ||
+      !CBB_add_asn1(&sigalg, &null, CBS_ASN1_NULL) ||
+      // issuer
+      !AddName(&tbs_cert, "Dummy issuer") ||
+      !CBB_add_asn1(&tbs_cert, &validity, CBS_ASN1_SEQUENCE) ||
+      !CBB_add_asn1(&validity, &not_before, CBS_ASN1_UTCTIME) ||
+      !CBB_add_bytes(&not_before,
+                     reinterpret_cast<const uint8_t*>("130101100000Z"), 13) ||
+      !CBB_add_asn1(&validity, &not_after, CBS_ASN1_UTCTIME) ||
+      !CBB_add_bytes(&not_after,
+                     reinterpret_cast<const uint8_t*>("130401100000Z"), 13) ||
+      // subject
+      !AddName(&tbs_cert, "Dummy certificate") ||
+      // subjectPublicKeyInfo
+      !CBB_add_asn1(&tbs_cert, &spki, CBS_ASN1_SEQUENCE) ||
+      !CBB_add_asn1(&spki, &key_params, CBS_ASN1_SEQUENCE) ||
+      !CBB_add_asn1(&key_params, &oid, CBS_ASN1_OBJECT) ||
+      !CBB_add_bytes(&oid, kECPublicKey, sizeof(kECPublicKey)) ||
+      !CBB_add_asn1(&key_params, &oid, CBS_ASN1_OBJECT) ||
+      !CBB_add_bytes(&oid, kP256, sizeof(kP256)) ||
+      !CBB_add_asn1(&spki, &public_key, CBS_ASN1_BITSTRING) ||
+      // Zero unused bits in BITSTRING.
+      !CBB_add_bytes(&public_key, reinterpret_cast<const uint8_t*>(""), 1) ||
+      !CBB_add_bytes(&public_key, kPublicKeyPoint, sizeof(kPublicKeyPoint)) ||
+      !CBB_add_asn1(&tbs_cert, &extensions_tag,
+                    3 | CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC) ||
+      !CBB_add_asn1(&extensions_tag, &extensions, CBS_ASN1_SEQUENCE) ||
+      !CBB_add_asn1(&extensions, &extension, CBS_ASN1_SEQUENCE) ||
+      !CBB_add_asn1(&extension, &oid, CBS_ASN1_OBJECT) ||
+      !CBB_add_bytes(&oid, kTagOID, sizeof(kTagOID)) ||
+      !CBB_add_asn1(&extension, &critical_flag, CBS_ASN1_BOOLEAN) ||
+      // Not critical.
+      !CBB_add_bytes(&critical_flag, reinterpret_cast<const uint8_t*>(""), 1) ||
+      !CBB_add_asn1(&extension, &tag_cbb, CBS_ASN1_OCTETSTRING) ||
+      !CBB_add_bytes(&tag_cbb, tag.data(), tag.size()) ||
+      !CBB_add_asn1(&cert, &sigalg, CBS_ASN1_SEQUENCE) ||
+      !CBB_add_asn1(&sigalg, &sigalg_oid, CBS_ASN1_OBJECT) ||
+      !CBB_add_bytes(&sigalg_oid, kSHA256RSAEncryption,
+                     sizeof(kSHA256RSAEncryption)) ||
+      !CBB_add_asn1(&sigalg, &null, CBS_ASN1_NULL) ||
+      !CBB_add_asn1(&cert, &signature, CBS_ASN1_BITSTRING) ||
+      // Dummy, 1-byte signature.
+      !CBB_add_bytes(&signature, reinterpret_cast<const uint8_t*>("\x00"), 2) ||
+      // Copy signerInfos from the input PKCS#7 structure.
+      !CopyASN1(&pkcs7_cbb, &pkcs7) || CBS_len(&pkcs7) != 0 ||
+      !CBB_finish(cbb.get(), &cbb_data, &cbb_len)) {
+    return base::nullopt;
+  }
+
+  // Copy the CBB result into a std::vector.
+  std::vector<uint8_t> ret;
+  ret.reserve(cbb_len);
+  ret.insert(ret.begin(), cbb_data, cbb_data + cbb_len);
+  OPENSSL_free(cbb_data);
+
+  // Inject the updated length in a couple of places:
+  const uint32_t certs_size = cbb_len - attr_cert_offset_;
+  //   1) The |IMAGE_DATA_DIRECTORY| structure that delineates the
+  //      |WIN_CERTIFICATE| structure.
+  memcpy(&ret[certs_size_offset_], &certs_size, sizeof(certs_size));
+  //   2) The first word of the |WIN_CERTIFICATE| structure itself.
+  memcpy(&ret[attr_cert_offset_], &certs_size, sizeof(certs_size));
+
+  return ret;
 }
 
-std::tuple<std::unique_ptr<Binary>, int /*err*/> NewBinary(
-    const std::vector<uint8_t>& contents) {
-  size_t offset = 0;
-  size_t size = 0;
-  size_t cert_size_offset;
-  int err =
-      GetAttributeCertificates(contents, &offset, &size, &cert_size_offset);
-  if (err) {
-    std::cerr << "authenticodetag: error parsing headers: " << err;
-    std::make_tuple(nullptr, err);
-  }
-
-  const std::vector<uint8_t> attribute_certificates{&contents[offset],
-                                                    &contents[offset + size]};
-  std::vector<uint8_t> asn1_data;
-  std::vector<uint8_t> appended_tag;
-  err = ProcessAttributeCertificates(attribute_certificates, &asn1_data,
-                                     &appended_tag);
-  if (err) {
-    std::cerr
-        << "authenticodetag: error parsing attribute certificate section: "
-        << err;
-    std::make_tuple(nullptr, err);
-  }
-
-  // TODO(agl): ASN1 unmarshaling magic here. crbug.com/1031734.
-
-  auto binary = std::make_unique<Binary>();
-  binary->contents = contents;
-  binary->attr_cert_offset = offset;
-  binary->cert_size_offset = cert_size_offset;
-  binary->asn1_data = std::move(asn1_data);
-  binary->appended_tag = std::move(appended_tag);
-
-  return std::make_tuple(std::move(binary), 0);
+Binary::Binary() = default;
+
+bool Binary::ParseTag() {
+  // Parse the |WIN_CERTIFICATE| structure to find the final certificate in the
+  // list and see whether it has an extension with |kTagOID|. If so, that's the
+  // tag for this binary.
+
+  CBS content_info = CBSFromSpan(content_info_);
+  CBS pkcs7, certs;
+  // See https://tools.ietf.org/html/rfc2315#section-7
+  if (!CBS_get_asn1(&content_info, &content_info, CBS_ASN1_SEQUENCE) ||
+      // type
+      !CBS_get_asn1(&content_info, nullptr, CBS_ASN1_OBJECT) ||
+      !CBS_get_asn1(&content_info, &pkcs7,
+                    0 | CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC) ||
+      // See https://tools.ietf.org/html/rfc2315#section-9.1
+      !CBS_get_asn1(&pkcs7, &pkcs7, CBS_ASN1_SEQUENCE) ||
+      // version
+      !CBS_get_asn1(&pkcs7, nullptr, CBS_ASN1_INTEGER) ||
+      // digests
+      !CBS_get_asn1(&pkcs7, nullptr, CBS_ASN1_SET) ||
+      // contentInfo
+      !CBS_get_asn1(&pkcs7, nullptr, CBS_ASN1_SEQUENCE) ||
+      !CBS_get_asn1(&pkcs7, &certs,
+                    0 | CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC)) {
+    return false;
+  }
+
+  bool have_last_cast = false;
+  CBS last_cert;
+
+  while (CBS_len(&certs) > 0) {
+    if (!CBS_get_asn1(&certs, &last_cert, CBS_ASN1_SEQUENCE)) {
+      return false;
+    }
+    have_last_cast = true;
+  }
+
+  if (!have_last_cast) {
+    return false;
+  }
+
+  // See https://tools.ietf.org/html/rfc5280#section-4.1 for the X.509 structure
+  // being parsed here.
+  CBS tbs_cert, outer_extensions;
+  int has_extensions;
+  if (!CBS_get_asn1(&last_cert, &tbs_cert, CBS_ASN1_SEQUENCE) ||
+      // version
+      !CBS_get_optional_asn1(
+          &tbs_cert, nullptr, nullptr,
+          CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 0) ||
+      // serialNumber
+      !CBS_get_asn1(&tbs_cert, nullptr, CBS_ASN1_INTEGER) ||
+      // signature algorithm
+      !CBS_get_asn1(&tbs_cert, nullptr, CBS_ASN1_SEQUENCE) ||
+      // issuer
+      !CBS_get_asn1(&tbs_cert, nullptr, CBS_ASN1_SEQUENCE) ||
+      // validity
+      !CBS_get_asn1(&tbs_cert, nullptr, CBS_ASN1_SEQUENCE) ||
+      // subject
+      !CBS_get_asn1(&tbs_cert, nullptr, CBS_ASN1_SEQUENCE) ||
+      // subjectPublicKeyInfo
+      !CBS_get_asn1(&tbs_cert, nullptr, CBS_ASN1_SEQUENCE) ||
+      // issuerUniqueID
+      !CBS_get_optional_asn1(
+          &tbs_cert, nullptr, nullptr,
+          CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 1) ||
+      // subjectUniqueID
+      !CBS_get_optional_asn1(
+          &tbs_cert, nullptr, nullptr,
+          CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 2) ||
+      !CBS_get_optional_asn1(
+          &tbs_cert, &outer_extensions, &has_extensions,
+          CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 3)) {
+    return false;
+  }
+
+  if (!has_extensions) {
+    return true;
+  }
+
+  CBS extensions;
+  if (!CBS_get_asn1(&outer_extensions, &extensions, CBS_ASN1_SEQUENCE)) {
+    return false;
+  }
+
+  while (CBS_len(&extensions) > 0) {
+    CBS extension, oid, contents;
+    if (!CBS_get_asn1(&extensions, &extension, CBS_ASN1_SEQUENCE) ||
+        !CBS_get_asn1(&extension, &oid, CBS_ASN1_OBJECT) ||
+        (CBS_peek_asn1_tag(&extension, CBS_ASN1_BOOLEAN) &&
+         !CBS_get_asn1(&extension, nullptr, CBS_ASN1_BOOLEAN)) ||
+        !CBS_get_asn1(&extension, &contents, CBS_ASN1_OCTETSTRING) ||
+        CBS_len(&extension) != 0) {
+      return false;
+    }
+
+    if (CBS_len(&oid) == sizeof(kTagOID) &&
+        memcmp(CBS_data(&oid), kTagOID, sizeof(kTagOID)) == 0) {
+      tag_ = SpanFromCBS(&contents);
+      break;
+    }
+  }
+
+  return true;
 }
 
 }  // namespace tools

chrome/updater/tools/certificate_tag.h

@@ -5,68 +5,54 @@
 #ifndef CHROME_UPDATER_TOOLS_CERTIFICATE_TAG_H_
 #define CHROME_UPDATER_TOOLS_CERTIFICATE_TAG_H_
 
-#include <cstdint>
-#include <memory>
-#include <tuple>
-#include <vector>
-
-namespace base {
-class Time;
-}
+#include "base/containers/span.h"
+#include "base/optional.h"
 
 namespace updater {
 namespace tools {
 
-struct SignedData;
-
-// Binary represents a PE binary.
-struct Binary {
-  Binary();
+// Binary represents a Windows PE binary and provides functions to extract and
+// set data outside of the signed area (called a "tag"). This allows a binary to
+// contain arbitrary data without invalidating any Authenticode signature.
+class Binary {
+ public:
+  Binary(const Binary&);
   ~Binary();
-  Binary(const Binary&) = delete;
-  Binary& operator=(const Binary&) = delete;
 
-  // The full file.
-  std::vector<uint8_t> contents;
+  // Parse a signed, Windows PE binary. Note that the returned structure
+  // contains pointers into the given data.
+  static base::Optional<Binary> Parse(base::span<const uint8_t> binary);
 
-  // The offset to the attribute certificates table.
-  size_t attr_cert_offset = 0;
+  // tag returns the embedded tag, if any.
+  const base::Optional<base::span<const uint8_t>>& tag() const;
 
-  // The offset to the size of the attribute certificates table.
-  size_t cert_size_offset = 0;
+  // SetTag returns an updated version of the binary that contains the given
+  // tag, or |nullopt| on error. If the binary already contains a tag then it
+  // will be replaced.
+  base::Optional<std::vector<uint8_t>> SetTag(
+      base::span<const uint8_t> tag) const;
 
-  // The PKCS#7, SignedData in DER form.
-  std::vector<uint8_t> asn1_data;
-
-  // The appended tag, if any.
-  std::vector<uint8_t> appended_tag;
+ private:
+  Binary();
 
-  // The parsed, SignedData structure.
-  SignedData* signed_data = nullptr;
+  // ParseTag attempts to parse out the tag. It returns false on parse error or
+  // true on success. If successful, it sets |tag_|.
+  bool ParseTag();
+
+  // binary_ contains the whole input binary.
+  base::span<const uint8_t> binary_;
+  // content_info_ contains the |WIN_CERTIFICATE| structure.
+  base::span<const uint8_t> content_info_;
+  // tag_ contains the embedded tag, or |nullopt| if there isn't one.
+  base::Optional<base::span<const uint8_t>> tag_;
+  // attr_cert_offset_ is the offset in the file where the |WIN_CERTIFICATE|
+  // structure appears. (This is the last structure in the file.)
+  size_t attr_cert_offset_;
+  // certs_size_offset_ is the offset in the file where the u32le size of the
+  // |WIN_CERTIFICATE| structure is embedded in an |IMAGE_DATA_DIRECTORY|.
+  size_t certs_size_offset_;
 };
 
-bool ParseUnixTime(const char* time_string, base::Time* parsed_time);
-
-std::tuple<std::unique_ptr<Binary>, int /*err*/> NewBinary(
-    std::vector<uint8_t> contents);
-std::vector<uint8_t> BuildBinary(const Binary& bin,
-                                 const std::vector<uint8_t>& asn1_data,
-                                 const std::vector<uint8_t>& tag);
-
-int ProcessAttributeCertificates(
-    const std::vector<uint8_t>& attribute_certificates,
-    std::vector<uint8_t>* asn1_data,
-    std::vector<uint8_t>* appended_tag);
-
-std::vector<uint8_t> AppendedTag(const Binary& bin);
-std::vector<uint8_t> RemoveAppendedTag(const Binary& bin);
-std::vector<uint8_t> SetAppendedTag(const Binary& bin,
-                                    const std::vector<uint8_t>& tag_contents);
-
-// SetSuperfluousCertTag returns a PE binary based on bin, but where the
-// superfluous certificate contains the given tag data.
-std::vector<uint8_t> SetSuperfluousCertTag(const Binary& bin,
-                                           const std::vector<uint8_t>& tag);
 }  // namespace tools
 }  // namespace updater
 

chrome/updater/tools/certificate_tag_unittest.cc

@@ -3,27 +3,65 @@
 // found in the LICENSE file.
 
 #include "chrome/updater/tools/certificate_tag.h"
-#include "base/time/time.h"
+#include "base/base_paths.h"
+#include "base/files/file.h"
+#include "base/files/file_path.h"
+#include "base/files/file_util.h"
+#include "base/path_service.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/zlib/google/compression_utils.h"
 
 namespace updater {
 namespace tools {
 
-TEST(UpdaterCertificateTagTests, ParseUnixTime) {
-  base::Time time;
-  EXPECT_TRUE(ParseUnixTime("Mon Jan 1 10:00:00 UTC 2013", &time));
-  base::Time::Exploded exploded;
-  time.UTCExplode(&exploded);
-  time.UTCExplode(&exploded);
-  EXPECT_EQ(2013, exploded.year);
-  EXPECT_EQ(1, exploded.month);
-  EXPECT_EQ(2, exploded.day_of_week);
-  EXPECT_EQ(10, exploded.hour);
-  EXPECT_EQ(0, exploded.minute);
-  EXPECT_EQ(0, exploded.second);
-
-  // Tests parsing an invalid date string.
-  EXPECT_FALSE(ParseUnixTime("Mon Jan 101 10:00:00 UTC 2013", &time));
+TEST(CertificateTag, RoundTrip) {
+  base::FilePath source_path;
+  ASSERT_TRUE(base::PathService::Get(base::DIR_SOURCE_ROOT, &source_path));
+  const base::FilePath exe_path = source_path.AppendASCII("chrome")
+                                      .AppendASCII("updater")
+                                      .AppendASCII("test")
+                                      .AppendASCII("data")
+                                      .AppendASCII("signed.exe.gz");
+  std::string exe;
+  ASSERT_TRUE(base::ReadFileToString(exe_path, &exe));
+  ASSERT_TRUE(compression::GzipUncompress(exe, &exe));
+  const base::span<const uint8_t> exe_span(
+      reinterpret_cast<const uint8_t*>(exe.data()), exe.size());
+
+  base::Optional<Binary> bin(Binary::Parse(exe_span));
+  ASSERT_TRUE(bin);
+
+  // Binary should be untagged on disk.
+  base::Optional<base::span<const uint8_t>> orig_tag(bin->tag());
+  EXPECT_FALSE(orig_tag);
+
+  static const uint8_t kTag[] = {1, 2, 3, 4, 5};
+  base::Optional<std::vector<uint8_t>> updated_exe(bin->SetTag(kTag));
+  ASSERT_TRUE(updated_exe);
+
+  base::Optional<Binary> bin2(Binary::Parse(*updated_exe));
+  ASSERT_TRUE(bin2);
+  base::Optional<base::span<const uint8_t>> parsed_tag(bin2->tag());
+  ASSERT_TRUE(parsed_tag);
+  EXPECT_TRUE(parsed_tag->size() == sizeof(kTag) &&
+              memcmp(kTag, parsed_tag->data(), sizeof(kTag)) == 0);
+
+  // Update an existing tag.
+  static const uint8_t kTag2[] = {1, 2, 3, 4, 6};
+  base::Optional<std::vector<uint8_t>> updated_again_exe(bin2->SetTag(kTag2));
+  ASSERT_TRUE(updated_again_exe);
+
+  base::Optional<Binary> bin3(Binary::Parse(*updated_again_exe));
+  ASSERT_TRUE(bin3);
+  base::Optional<base::span<const uint8_t>> parsed_tag2(bin3->tag());
+  ASSERT_TRUE(parsed_tag2);
+  EXPECT_TRUE(parsed_tag2->size() == sizeof(kTag2) &&
+              memcmp(kTag2, parsed_tag2->data(), sizeof(kTag2)) == 0);
+
+  // Updating an existing tag with a tag of the same size should not have grown
+  // the binary, i.e. the old tag should have been erased first.
+  EXPECT_EQ(sizeof(kTag), sizeof(kTag2));
+  EXPECT_EQ(updated_exe->size(), updated_again_exe->size());
 }
 
 }  // namespace tools

chrome/updater/tools/main.cc

@@ -22,58 +22,26 @@
 
 namespace updater {
 namespace tools {
-namespace {
-
-// Command line switches.
-// If set, any appended tag is dumped to stdout.
-const char kDumpAppendedTagSwitch[] = "dump-appended-tag";
-
-// If set, any appended tag is removed and the binary rewritten.
-const char kRemoveAppendedTagSwitch[] = "remove-appended-tag";
-
-// If set, this flag contains a filename from which the contents of the appended
-// tag will be saved.
-const char kLoadAppendedTagSwitch[] = "load-appended-tag";
 
 // If set, this flag contains a string and a superfluous certificate tag with
 // that value will be set and the binary rewritten. If the string begins
 // with '0x' then it will be interpreted as hex.
 const char kSetSuperfluousCertTagSwitch[] = "set-superfluous-cert-tag";
 
-// A superfluous cert tag will be padded with zeros to at least this number of
-// bytes.
-const char kPaddedLengthSwitch[] = "padded-length";
-
-// If set to a filename, the PKCS7 data from the original binary will be written
-// to that file.
-const char kSavePKCS7Switch[] = "save-pkcs7";
+// If set, this flag causes the current tag, if any, to be written to stdout.
+const char kGetSuperfluousCertTagSwitch[] = "get-superfluous-cert-tag";
 
 // If set, the updated binary is written to this file. Otherwise the binary is
 // updated in place.
 const char kOutFilenameSwitch[] = "out";
 
 struct CommandLineArguments {
-  CommandLineArguments();
-  ~CommandLineArguments();
-
-  // Dumps tag to stdout.
-  bool dump_appended_tag = false;
-
-  // Removes the tag from the binary.
-  bool remove_appended_tag = false;
-
-  // Reads and outputs the tag from the binary to stdout or the output file.
-  base::FilePath load_appended_tag;
+  // Whether to print the current tag.
+  bool get_superfluous_cert_tag = false;
 
   // Sets the certificate from bytes.
   std::string set_superfluous_cert_tag;
 
-  // Minimum Length of the padded area in the certificate.
-  int padded_length = 0;
-
-  // Specifies the filename to save the PKCS7 data into.
-  base::FilePath save_PKCS7;
-
   // Specifies the input file (which may be the same as the output file).
   base::FilePath in_filename;
 
@@ -81,60 +49,43 @@ struct CommandLineArguments {
   base::FilePath out_filename;
 };
 
-CommandLineArguments::CommandLineArguments() = default;
-CommandLineArguments::~CommandLineArguments() = default;
-
 void PrintUsageAndExit(const base::CommandLine* cmdline) {
   std::cerr << "Usage: " << cmdline->GetProgram().MaybeAsASCII()
-            << " [flags] binary.exe";
+            << " [flags] binary.exe" << std::endl;
   std::exit(255);
 }
 
 void HandleError(int error) {
-  std::cerr << "Error: " << error;
+  std::cerr << "Error: " << error << std::endl;
   std::exit(1);
 }
 
-CommandLineArguments ParseCommandLineArgs() {
+CommandLineArguments ParseCommandLineArgs(int argc, char** argv) {
   CommandLineArguments args;
-  base::CommandLine::Init(0, nullptr);
+  base::CommandLine::Init(argc, argv);
   auto* cmdline = base::CommandLine::ForCurrentProcess();
   if (cmdline->argv().size() == 1 || cmdline->GetArgs().size() != 1)
     PrintUsageAndExit(cmdline);
 
   args.in_filename = base::FilePath{cmdline->GetArgs()[0]};
+  args.out_filename = args.in_filename;
 
-  args.dump_appended_tag = cmdline->HasSwitch(kDumpAppendedTagSwitch);
-  cmdline->RemoveSwitch(kDumpAppendedTagSwitch);
-
-  args.remove_appended_tag = cmdline->HasSwitch(kRemoveAppendedTagSwitch);
-  cmdline->RemoveSwitch(kRemoveAppendedTagSwitch);
-
-  args.load_appended_tag = cmdline->GetSwitchValuePath(kLoadAppendedTagSwitch);
-  cmdline->RemoveSwitch(kLoadAppendedTagSwitch);
+  args.get_superfluous_cert_tag =
+      cmdline->HasSwitch(kGetSuperfluousCertTagSwitch);
+  cmdline->RemoveSwitch(kGetSuperfluousCertTagSwitch);
 
   args.set_superfluous_cert_tag =
       cmdline->GetSwitchValueASCII(kSetSuperfluousCertTagSwitch);
   cmdline->RemoveSwitch(kSetSuperfluousCertTagSwitch);
 
-  args.padded_length = [cmdline]() {
-    int retval = 0;
-    if (base::StringToInt(cmdline->GetSwitchValueASCII(kPaddedLengthSwitch),
-                          &retval)) {
-      std::cerr << "Invalid command line argument: "
-                << cmdline->GetSwitchValueASCII(kPaddedLengthSwitch);
-      std::exit(EXIT_FAILURE);
-    }
-    return retval;
-  }();
-  cmdline->RemoveSwitch(kPaddedLengthSwitch);
-
-  args.save_PKCS7 = cmdline->GetSwitchValuePath(kSavePKCS7Switch);
-  cmdline->RemoveSwitch(kSavePKCS7Switch);
-
-  args.out_filename = cmdline->GetSwitchValuePath(kOutFilenameSwitch);
+  const base::FilePath maybe_out_filename =
+      cmdline->GetSwitchValuePath(kOutFilenameSwitch);
   cmdline->RemoveSwitch(kOutFilenameSwitch);
 
+  if (!maybe_out_filename.empty()) {
+    args.out_filename = maybe_out_filename;
+  }
+
   const auto unknown_switches = cmdline->GetSwitches();
   if (!unknown_switches.empty()) {
     std::cerr << "Unknown command line switch: "
@@ -145,9 +96,8 @@ CommandLineArguments ParseCommandLineArgs() {
   return args;
 }
 
-int CertificateTagMain() {
-  const auto args = ParseCommandLineArgs();
-  return 0;
+int CertificateTagMain(int argc, char** argv) {
+  const auto args = ParseCommandLineArgs(argc, argv);
 
   const base::FilePath in_filename = args.in_filename;
   const base::FilePath out_filename =
@@ -163,74 +113,20 @@ int CertificateTagMain() {
     HandleError(logging::GetLastSystemErrorCode());
   }
 
-  std::unique_ptr<tools::Binary> bin;
-  int err = 0;
-  std::tie(bin, err) = NewBinary(contents);
-  if (err)
-    HandleError(err);
-
-  bool did_something = false;
-
-  if (!args.save_PKCS7.empty()) {
-    if (base::WriteFile(args.save_PKCS7,
-                        reinterpret_cast<char*>(&bin->asn1_data.front()),
-                        bin->asn1_data.size()) == -1) {
-      std::cerr << "Error while writing file: "
-                << logging::GetLastSystemErrorCode();
+  base::Optional<tools::Binary> bin = tools::Binary::Parse(contents);
+  if (!bin) {
+    std::cerr << "Failed to parse tag binary" << std::endl;
     std::exit(1);
   }
-    did_something = true;
-  }
 
-  if (args.dump_appended_tag) {
-    const auto appended_tag = AppendedTag(*bin);
-    if (appended_tag.empty())
-      std::cerr << "No appended string found";
-    else
-      std::cout << base::HexEncode(appended_tag.data(), appended_tag.size());
-    did_something = true;
-  }
-
-  if (args.remove_appended_tag) {
-    auto contents = RemoveAppendedTag(*bin);
-    if (contents.empty()) {
-      std::cerr << "Error while removing appended tag";
+  if (args.get_superfluous_cert_tag) {
+    base::Optional<base::span<const uint8_t>> tag = bin->tag();
+    if (!tag) {
+      std::cerr << "No tag in binary" << std::endl;
       std::exit(1);
     }
-    if (base::WriteFile(out_filename,
-                        reinterpret_cast<char*>(&contents.front()),
-                        contents.size()) == -1) {
-      std::cerr << "Error while writing file: "
-                << logging::GetLastSystemErrorCode();
-      std::exit(1);
-    }
-    did_something = true;
-  }
-
-  if (!args.load_appended_tag.empty()) {
-    int64_t file_size = 0;
-    if (!base::GetFileSize(args.load_appended_tag, &file_size))
-      HandleError(logging::GetLastSystemErrorCode());
-
-    std::vector<uint8_t> tag_contents(file_size);
-    if (base::ReadFile(args.load_appended_tag,
 
-                       reinterpret_cast<char*>(&tag_contents.front()),
-                       tag_contents.size()) == -1) {
-      std::cerr << "Error while reading file: "
-                << logging::GetLastSystemErrorCode();
-      std::exit(1);
-    }
-
-    auto contents = SetAppendedTag(*bin, tag_contents);
-    if (base::WriteFile(out_filename,
-                        reinterpret_cast<char*>(&contents.front()),
-                        contents.size()) == -1) {
-      std::cerr << "Error while writing updated file: "
-                << logging::GetLastSystemErrorCode();
-      std::exit(1);
-    }
-    did_something = true;
+    std::cout << base::HexEncode(*tag) << std::endl;
   }
 
   if (!args.set_superfluous_cert_tag.empty()) {
@@ -238,11 +134,12 @@ int CertificateTagMain() {
     std::vector<uint8_t> tag_contents;
     if (base::StartsWith(args.set_superfluous_cert_tag, kPrefix,
                          base::CompareCase::INSENSITIVE_ASCII)) {
-      if (!base::HexStringToBytes(
-              {std::begin(args.set_superfluous_cert_tag) + base::size(kPrefix),
-               std::end(args.set_superfluous_cert_tag)},
-              &tag_contents)) {
-        std::cerr << "Failed to parse tag contents from command line";
+      const base::StringPiece hex_chars(
+          std::begin(args.set_superfluous_cert_tag) + base::size(kPrefix) - 1,
+          std::end(args.set_superfluous_cert_tag));
+      if (!base::HexStringToBytes(hex_chars, &tag_contents)) {
+        std::cerr << "Failed to parse tag contents from command line"
+                  << std::endl;
         std::exit(1);
       }
     } else {
@@ -250,42 +147,26 @@ int CertificateTagMain() {
                           args.set_superfluous_cert_tag.end());
     }
 
-    if (tag_contents.size() < args.padded_length)
-      tag_contents.resize(0);
-
-    auto contents = SetSuperfluousCertTag(*bin, tag_contents);
-    if (contents.empty()) {
+    auto new_contents = bin->SetTag(tag_contents);
+    if (!new_contents) {
       std::cerr << "Error while setting superfluous certificate tag.";
       std::exit(1);
     }
     if (base::WriteFile(out_filename,
-                        reinterpret_cast<char*>(&contents.front()),
-                        contents.size()) == -1) {
+                        reinterpret_cast<const char*>(new_contents->data()),
+                        new_contents->size()) == -1) {
       std::cerr << "Error while writing updated file "
                 << logging::GetLastSystemErrorCode();
       std::exit(1);
     }
-    did_something = true;
-  }
-
-  if (did_something) {
-    // By default, print basic information.
-    auto appended_tag = AppendedTag(*bin);
-    if (appended_tag.empty()) {
-      std::cout << "No appended tag";
-    } else {
-      std::cout << "Appended tag included, " << appended_tag.size()
-                << " bytes.";
-    }
   }
 
   return EXIT_SUCCESS;
 }
 
-}  // namespace
 }  // namespace tools
 }  // namespace updater
 
-int main() {
-  return updater::tools::CertificateTagMain();
+int main(int argc, char** argv) {
+  return updater::tools::CertificateTagMain(argc, argv);
 }