Change-Id: I295b0142b4448a5ee10ca9b092a2c3eaa1fffc86
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60405
Reviewed-by: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
While I'm here, align on the version that compares the lengths
explicitly, rather than subtract. The subtraction trick does actually
work, because the lengths can't be negative and we're two's complement
(so 0 - INT_MAX fits in int). But just comparing avoids needing to think
about it.
Change-Id: Ide6e3539a27e187bb1a405600c367bb8dd82197e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62545
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
While cmake does not care and happily build anyway, Bazel gets
very upset when faced with this in it's generated files from this.
Crbug: 1322914
Change-Id: Ia564be8dfd81bd206b80996bc660113da04de314
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62505
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Auto-Submit: Bob Beck <bbe@google.com>
This also removes fillins/check.h which is no longer needed.
Crbug: 1322914
Change-Id: If5e8355700472bf6703c80809ea276c4c07ddc52
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62485
Auto-Submit: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Bug: b:296302767
Change-Id: I247c02b6b8fbab38f254c9d74576d0b103d93b4a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62425
Reviewed-by: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
we do end up needing this for google3.
crbug: 1322914
Change-Id: I3788170521fff6a7a8075c58d929558b97820a34
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62405
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Change-Id: I43283162ef356f9e7fb959dbc1ec9e0e98ee83ed
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62385
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: Bob Beck <bbe@google.com>
Implemented a generic Aead trait and struct against the EVP_AEAD
API's, which can be used to provide bindings to all of the AEAD's
provided by boringssl. Starting with AES_GCM_SIV, but will expand
to more AEAD's.
Change-Id: I7d4113f3d49ff40de3ccb76424f9a25d25797e82
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/59965
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Someone requested that bssl::ScopedEVP_HPKE_KEY be movable.
Change-Id: I48058567c776b5fe9a746072ccb7ddd723ef2b68
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62265
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
As with large p, q, or g in the preceding CL, an application that uses
Diffie-Hellman incorrectly could be given a large private key length.
Computing the public key and shared secret will then be very slow.
This matters for a (p, g)-only group, where q is unknown. One way or
another, we should bound or clamp dh->priv_length. The two relevant
specifications I could find are SP 800-56A Rev3 and PKCS#3. SP 800-56A
wants a value for q, so we'd need to fabricate one. I believe X9.42's
Diffie-Hellman formulation similarly expects an explicit q. PKCS#3 is
(p, g)-only and seems to match what OpenSSL does. In PKCS#3:
- DH groups have an optional l, such that 2^(l-1) <= p
- For keygen, if l was provided, pick 2^(l-1) <= x < 2^l. Otherwise,
pick 0 < x < p-1
Our current q-less keygen behavior matches this, with
l = num_bits(p) - 1. Interestingly, the first constraint allows
l = num_bits(p), but doing so allows x >= p - 1! This is a bit odd and
will wrap around the multiplicative group.
OpenSSL 3.0 (but not 1.1.1) bounds l in their q-less path, and cites
PKCS#3's 2^(l-1) <= p in a comment. But their actual check doesn't match
and excludes num_bits(p). The two problems cancel each other out.
PKCS#3 is quite old and does not even discuss subgroups or safe primes,
only this uninspiring text:
> Some additional conditions on the choice of prime, base, and
> private-value length may well be taken into account in order to deter
> discrete logarithm computation. These security conditions fall outside
> the scope of this standard.
I'm thus not inclined to give the document much weight in 2023. SP
800-56A Rev3 is not ideal either. First, we must fabricate a value for
q. (p-1)/2 is most natural, though it assumes g indeed generates a
prime-order subgroup and not the whole multiplicative group.
The second annoyance with SP 800-56A Rev3 is its insistance on uniformly
selecting between [1, 2^N-1] instead of [0, 2^N-1] or [1, 2^N]. For all
plausible values of N, this difference will not matter, yet NIST
specifies rejection sampling, defeating the point of a power-of-two
bound.
None of these really matters. p should be large enough to make the
differences between all these schemes negligible. Since we want to
follow NIST for the (p, q, g) path, using the same scheme for the (p, g)
path seems the most reasonable. Thus this CL recasts that path from
PKCS#3 to SP 800-56A, except dh->priv_length is clamped before invoking
the algorithm, to avoid worrying about whether someone expects
DH_set_length(BN_num_bits(p)) to work.
Change-Id: I270f235a6f04c69f8abf59edeaf39d837e2c79f8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62228
Reviewed-by: Bob Beck <bbe@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
I think this dates to when CRYPTO_is_*_capable were inline functions in
public headers, so they couldn't access OPENSSL_armcap_P directly. Now
they can.
Change-Id: Ic06fffa7f5056401118b62d690dfe6b21bc30f86
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62345
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
sha512-armv8.pl and sha512-x86_64.pl implement both SHA-256 and SHA-512
and select which to emit by looking for "512" in the output path.
This can result in a false positive if the output path happens to
contain "512" in it. When the build uses relative paths, it's fine, but
this seems needlessly fragile. If we're generate into a temporary file,
there's a small but non-negligible probability that the path has a
"512" in it.
Instead, give those scripts three arguments: flavor hash output, so the
selection is independent of the output file name.
Bug: 542
Change-Id: Idf256abed1c07003034d3eb4544552125e3289e5
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62325
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Change-Id: I235d81c6e6b013b25488355ccd5de254e7c172b8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62306
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: Bob Beck <bbe@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Section 5.6.1.1.4 of SP 800-56A Rev 3 and Appendix B.1.2 of FIPS 186-4
select the private key out of the range [1, q-1]. We used [2, q-1]. This
distinction is unimportant. 0, 1, 2, 3, 4, etc. all make equally bad
private keys. The defense against each of these is their negligible
probability, not rejection sampling.
Nonetheless, we may as well align with *some* specification, and NIST's
formulation works fine.
Change-Id: I33352061f3fbdbec5b14b576d15be98464a57536
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62227
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
When applications use Diffie-Hellman incorrectly, and use
attacker-supplied domain parameters, rather than known-valid ones (as
required by SP 800-56A, 5.5.2), algorithms that aren't designed with
attacker-supplied parameters in mind become attack surfaces.
CVE-2023-3446 and CVE-2023-3817 in OpenSSL cover problems with the
DH_check function given large p and large q. This CL adds some fast
validity checks to the DH parameters before running any operation. This
differs from upstream in a few ways:
- Upstream only addressed issues with DH_check. We also check in
DH_generate_key and DH_check_pub_key.
- For a more consistent invariant, reuse the existing DH modulus limit.
Ideally we'd enforce these invariants on DH creation, but this is not
possible due to OpenSSL's API. We additionally check some other
cheap invariants.
This does not impact TLS, or any applications that used Diffie-Hellman
correctly, with trusted, well-known domain parameters.
Ultimately, that this comes up at all is a flaw in how DH was specified.
This is analogous to the issues with ECC with arbitrary groups and DSA,
which led to https://github.com/openssl/openssl/issues/20268
CVE-2022-0778, CVE-2020-0601, and likely others. Cryptographic
primitives should be limited to a small set of named, well-known domain
parameters.
Update-Note: Egregiously large or invalid DH p, q, or g values will be
more consistently rejected in DH operations. This does not impact TLS.
Applications should switch to modern primitives such as X25519 or ECDH
with P-256.
Change-Id: I666fe0b9f8b71632f6cf8064c8ea0251e5c286bb
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62226
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Change-Id: I05a3b0cb7ffaee90ed85d2cf795feded8fbad1df
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62305
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: Bob Beck <bbe@google.com>
If the DH object already has a private key, DH_generate_key is actually
a function to compute the corresponding public key. This is very weird,
but as we don't really care about DH, just document and test it.
Change-Id: Idbddfd06839450a198fdf8a34bf2f53b0250c400
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62225
Reviewed-by: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Change-Id: Ib65febca30ce312f2c8fd6d6dbc85f24987b50d8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62245
Auto-Submit: Bob Beck <bbe@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Change-Id: I203a19b59c23def9bca6f01c2b6e8c885b0c9c3f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62205
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: Bob Beck <bbe@google.com>
Use CSlice instead of a regular Rust slice when passing pointers to C
FFI.
Change-Id: Iccd827f4c6f005d860993e97fef5e9caf514885b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60525
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
I think there are a couple projects remaining that still use the old
lists here, but they're in repositories we don't spend as much time in,
and it should be straightforward for them to update when they get here.
Removing these should put us in a good place to check in pre-generated
asm lists. While I'm here, fix a few typos in TODOs I previously added.
Update-Note: If you're one of those projects and have trouble switching
to the new lists, let us know.
Bug: 542
Change-Id: I57559bafc85eceacc7a237e2f29db6eaf492a8cb
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62186
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
The android-cmake one should no longer be needed as of aosp/2673299, and
the JSON one as of https://github.com/grpc/grpc/pull/33700
Bug: 542
Change-Id: I3c7b752806d82a5a01b5ad9180771e88d2810b70
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62185
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
e4acd6cb56 enabled assembly for more
platforms on Android, but the way Android's FIPS build is set up, we
currently require ld -r to work on any platform with assembly. See
b/294399371.
This should be fixable with more time spent on the Android build (and
possibly missing features added to Soong, as Soong is quite limited),
but as we've never had assembly working here, just restore the old state
of things.
Change-Id: I8f4e66979a003a5692389ef7e127c8d9f1630773
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62165
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
While it's the same code path, NIST may consider these different
functions and thus want separate checks for them.
Change-Id: Ic391b5e656b22c5e11d94ec22398346669833bd9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62087
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
They are NIST hash functions, but this service indicator function is
specific to their use in the TLS KDF.
Change-Id: I5a1f9d2865813f436a8e2a7548dffefcb2813c5f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62086
Auto-Submit: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
With b/291102972 resolved, we can try this again.
Bug: 629, b:291102972
Change-Id: Ic04d1855f185ead6ae2e151dcc56493afce40b4f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62105
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
These should not be necessary as of aosp/2673984.
Bug: 542
Change-Id: Ice0d8a6c535bb2bd4549cbf88197a36c8e859e74
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62085
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
cpu_arm_openbsd.c is the same as OPENSSL_STATIC_ARMCAP.
cpu_aarch64_freebsd.c is the same as cpu_aarch64_sysreg.c. (The FreeBSD
one was using the macros in their headers, but those macros expand to
the same inline assembly.)
Also send ANDROID_BAREMETAL + 32-bit Arm to OPENSSL_STATIC_ARMCAP. This
way we can remove OPENSSL_STATIC_ARMCAP from the Android baremetal build
without having to chase down constraining it to aarch64. See
b/291106677#comment6
Update-Note: This is a slight change to the OpenBSD build. Previously,
we assumed OpenBSD on 32-bit Arm implies NEON. Now, we pick it up from
the __ARM_NEON define from ACLE, i.e. whether the compiler has been told
that NEON is available. (This comes from -march or other options.) Doing
that is desirable anyway: if NEON is in your baseline, you should tell
your compiler so it can vectorize loops.
Change-Id: Icd43a2b56bb6e3f04f0fed996ae750fba65e3312
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62066
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Provide an implementation of OPENSSL_cpuid_setup() which identifies the
available CPU features required by the library directly from the system
registers for baremetal AArch64 builds without static initializer
(OPENSSL_NO_STATIC_INITIALIZER) that don't configure static capabilities
(OPENSSL_STATIC_ARMCAP). This assumes that the client code is NOT
running at exception level EL0 (userspace) and is enabled for
ANDROID_BAREMETAL.
Bug: b:265125189
Change-Id: Ifee6fbd24ece823a4661dd984f89473e1e1e3eda
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58586
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Tested in aosp/2673719. As a bonus, this will increase asm coverage on
Android. Right now they're not building the macOS assembly.
Bug: 542
Change-Id: I02f77831566bba55bb204cb08c1e1e972e03b90b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62005
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Pretty much all of std::span and base::span are constexpr. der::Input
similarly has constexpr bits. So we can use bssl::Span in der::Input,
align bssl::Span in constexpr-ness.
Also fix const-ness of first() and last().
Bug: chromium:770501
Change-Id: Ic0031cd955d8ac0af9c3cb928411f23a34820347
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61945
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Adds general_names and crl unittests to pki tests, with
associated data files for crl unittest.
Bug: chromium:1322914
Change-Id: Idda8e4a98ef3744a76717db32628db554c12e415
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61965
Auto-Submit: Bob Beck <bbe@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Like OPENSSL_NO_FILESYSTEM, keep us honest: if the symbol is missing,
don't declare it in the headers. This ensures folks aren't relying on
dead code elimination and then later break when they build in a context
where it doesn't happen.
Bug: 629
Change-Id: I3e56c3879e970aa8d0d6e0e5f1ad046d0f420ef0
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61730
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Detecting errors (i.e. fs-less platforms using fs-only APIs) at compile
time is generally preferable to doing so at runtime, so
https://boringssl-review.googlesource.com/c/boringssl/+/61726 opted to
remove the APIs altogether on applicable targets.
However, Trusty uses rust-openssl somewhere and rust-openssl binds a
bunch of filesystem-dependent APIs unconditionally. To keep that
working, switch to a stub fopen when OPENSSL_NO_FILESYSTEM is set. We
effectively model a platform where the filesystem "exists", but is
empty. Upstream OpenSSL similarly has OPENSSL_NO_STDIO still define the
file BIO (unlike the socket BIO, which is excluded), but in a stub form.
As part of this, I've gone ahead and resolved one of the Trusty TODOs.
It does produce a duplicate symbol with [1], but things seem to link
fine in treehugger. In case it does break, I've bumped
BORINGSSL_API_VERSION, so we can go in and condition it if needed.
[1] https://android.googlesource.com/trusty/lib/+/refs/heads/main/lib/openssl-stubs/bio.c
Bug: 629
Change-Id: I4f20d872a7cde863d21c78090f270b77b03545fa
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61925
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Change-Id: Ic461a3890aa78109dc06ffcf144b6b7a90456ff3
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61748
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: Bob Beck <bbe@google.com>
Seed the corpus from cert_corpus. As part of that, check in the result of minimizing all the corpora.
Note this is just making one of the fuzzers build, I'll adapt
the others and follow on by updating the IMPORT process to do it
in a follow on cl.
Bug: chromium:1322914
Change-Id: Iea1b89f8fee938fa99c0a4d8134bcd0e7023d149
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61765
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Some platforms would not have sys/socket.h, we should guard these
socket related headers with OPENSSL_NO_SOCK.
Bug: 629
Change-Id: I2d7c31ad32d467da46114307fd89c2ba3d41df2c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61845
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
See if that helps discourage their use slightly. (Though pretty much all
the uses are problematic things like Rust and prebuilts, so they
probably won't notice.)
Change-Id: I012b68922d7b36e778488e7455908e81b3de15f7
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61905
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
This is a temporary workaround for b/291994116
Bug: b:291994116
Change-Id: I5b0372e07cf3bdf4b6a5b4b37dabea828ad026a8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61885
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
This addes enough to get it building and running tests
in google3 with some changes in the google3 BUILD file.
Change-Id: I1cf17bc602253a69420456fd6454d10cdcf6b988
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61747
Auto-Submit: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
This warning was being tripped because lib_buf and reason_buf made GCC,
incorrectly, believe that the strings could get that long, and then
attempted to sum up the snprintf to 120, obtained by inlining some
things.
Those buffers were larger than they needed to be, so bringing it down is
sufficient to silence things. That said, the buffer bounds are supplied
by the caller and it is expected that truncation can occur, so the
warning is just incorrect. The warning can also be silenced by checking
the snprintf return value. As we're already trying to detect truncation,
we may as well do it with the return value and skip the extra strlen
call.
Either of the two changes is sufficient to suppress the warning, but
both seem worthwhile, so I've done them both.
Change-Id: Ia1b1de67bba55da6f0d07e3682165a1820ce2c9e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61805
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>