From 77e9a6c5028a9712490d65214a9882143c329ec5 Mon Sep 17 00:00:00 2001 From: Benoit Lize Date: Tue, 21 Mar 2023 11:14:20 +0000 Subject: [Backport] CVE-2023-1812: Out of bounds memory access in DOM Bindings Cherry-pick of patch originally reviewed on https://chromium-review.googlesource.com/c/chromium/src/+/4357658: Take encoding into account for ParkableString hashing Hashing is used for string deduplication, must take encoding into account. See linked bug for details. (cherry picked from commit ab66c0409aece5bd57511792a3867920f31c589b) Bug: 1418224 Change-Id: I63c024d0a97e44b1f3323cd1ca4d9e953c2beed1 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4328136 Commit-Queue: Benoit Lize Reviewed-by: Kentaro Hara Cr-Original-Commit-Position: refs/heads/main@{#1117528} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4357658 Bot-Commit: Rubber Stamper Owners-Override: Benoit Lize Auto-Submit: Benoit Lize Cr-Commit-Position: refs/branch-heads/5615@{#696} Cr-Branched-From: 9c6408ef696e83a9936b82bbead3d41c93c82ee4-refs/heads/main@{#1109224} Reviewed-on: https://codereview.qt-project.org/c/qt/qtwebengine-chromium/+/474365 Reviewed-by: Allan Sandfeld Jensen --- .../renderer/platform/bindings/parkable_string.cc | 20 +++++++++++++++----- .../renderer/platform/bindings/parkable_string.h | 3 +++ 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/chromium/third_party/blink/renderer/platform/bindings/parkable_string.cc b/chromium/third_party/blink/renderer/platform/bindings/parkable_string.cc index afd1b69952b..ee0a48bf2c9 100644 --- a/chromium/third_party/blink/renderer/platform/bindings/parkable_string.cc +++ b/chromium/third_party/blink/renderer/platform/bindings/parkable_string.cc @@ -4,6 +4,8 @@ #include "third_party/blink/renderer/platform/bindings/parkable_string.h" +#include + #include "base/allocator/partition_allocator/oom.h" #include "base/allocator/partition_allocator/partition_alloc.h" #include "base/bind.h" @@ -230,18 +232,26 @@ ParkableStringImpl::ParkableMetadata::ParkableMetadata( is_8bit_(string.Is8Bit()), length_(string.length()) {} -// static +// static2 std::unique_ptr ParkableStringImpl::HashString(StringImpl* string) { DigestValue digest_result; - bool ok = ComputeDigest(kHashAlgorithmSha256, - static_cast(string->Bytes()), - string->CharactersSizeInBytes(), digest_result); + + Digestor digestor(kHashAlgorithmSha256); + digestor.Update(base::make_span(static_cast(string->Bytes()), + string->CharactersSizeInBytes())); + // Also include encoding in the digest, otherwise two strings with identical + // byte content but different encoding will be assumed equal, leading to + // crashes when one is replaced by the other one. + std::array is_8bit; + is_8bit[0] = string->Is8Bit(); + digestor.Update(is_8bit); + digestor.Finish(digest_result); // The only case where this can return false in BoringSSL is an allocation // failure of the temporary data required for hashing. In this case, there // is nothing better to do than crashing. - if (!ok) { + if (digestor.has_failed()) { // Don't know the exact size, the SHA256 spec hints at ~64 (block size) // + 32 (digest) bytes. base::TerminateBecauseOutOfMemory(64 + kDigestSize); diff --git a/chromium/third_party/blink/renderer/platform/bindings/parkable_string.h b/chromium/third_party/blink/renderer/platform/bindings/parkable_string.h index 78e2185cced..0d607577347 100644 --- a/chromium/third_party/blink/renderer/platform/bindings/parkable_string.h +++ b/chromium/third_party/blink/renderer/platform/bindings/parkable_string.h @@ -57,6 +57,9 @@ class PLATFORM_EXPORT ParkableStringImpl final constexpr static size_t kDigestSize = 32; // SHA256. using SecureDigest = Vector; // Computes a secure hash of a |string|, to be passed to |MakeParkable()|. + // + // TODO(lizeb): This is the "right" way of hashing a string. Move this code + // into WTF, and make sure it's the only way that is used. static std::unique_ptr HashString(StringImpl* string); // Not all ParkableStringImpls are actually parkable. -- cgit v1.2.1