New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 5 users

Issue metadata

Status: Fixed
Owner:
Closed: Feb 13
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 1
Type: Bug-Security



Sign in to add a comment

Security: V8 Integer overflow in object allocation size

Project Member Reported by markbrand@google.com, Feb 1

Issue description

This template is ONLY for reporting security bugs. If you are reporting a
Download Protection Bypass bug, please use the "Security - Download
Protection" template. For all other reports, please use a different
template.

Please READ THIS FAQ before filing a bug: https://chromium.googlesource.com
/chromium/src/+/master/docs/security/faq.md

Please see the following link for instructions on filing security bugs:
https://www.chromium.org/Home/chromium-security/reporting-security-bugs

NOTE: Security bugs are normally made public once a fix has been widely
deployed.

VULNERABILITY DETAILS
There's an integer overflow in computing the required allocation size when instantiating a new javascript object. 

See the following code in objects.cc

// static
bool JSFunction::CalculateInstanceSizeForDerivedClass(
    Handle<JSFunction> function, InstanceType instance_type,
    int requested_embedder_fields, int* instance_size,
    int* in_object_properties) {
  Isolate* isolate = function->GetIsolate();
  int expected_nof_properties = 0;
  bool result = true;
  for (PrototypeIterator iter(isolate, function, kStartAtReceiver);
       !iter.IsAtEnd(); iter.Advance()) {
    Handle<JSReceiver> current =
        PrototypeIterator::GetCurrent<JSReceiver>(iter);
    if (!current->IsJSFunction()) break;
    Handle<JSFunction> func(Handle<JSFunction>::cast(current));
    // The super constructor should be compiled for the number of expected
    // properties to be available.
    Handle<SharedFunctionInfo> shared(func->shared());
    if (shared->is_compiled() ||
        Compiler::Compile(func, Compiler::CLEAR_EXCEPTION)) {
      DCHECK(shared->is_compiled());
      expected_nof_properties += shared->expected_nof_properties(); // <--- overflow here!
    } else if (!shared->is_compiled()) {
      // In case there was a compilation error for the constructor we will
      // throw an error during instantiation. Hence we directly return 0;
      result = false;
      break;
    }
    if (!IsDerivedConstructor(shared->kind())) {
      break;
    }
  }
  CalculateInstanceSizeHelper(instance_type, true, requested_embedder_fields,
                              expected_nof_properties, instance_size,
                              in_object_properties);
  return result;
}

By supplying a long prototype chain of objects with a large expected_nof_properties we can control the resulting value of instance_size by causing (requested_embedder_fields + requested_in_object_properties) << kPointerSizeLog2 to be overflown to a small negative value, resulting in an allocation smaller than header_size, which is the minimum required size for the base object class being allocated. This results in memory corruption when the object is initialised/used.

void JSFunction::CalculateInstanceSizeHelper(InstanceType instance_type,
                                             bool has_prototype_slot,
                                             int requested_embedder_fields,
                                             int requested_in_object_properties,
                                             int* instance_size,
                                             int* in_object_properties) {
  int header_size = JSObject::GetHeaderSize(instance_type, has_prototype_slot);
  DCHECK_LE(requested_embedder_fields,
            (JSObject::kMaxInstanceSize - header_size) >> kPointerSizeLog2);
  *instance_size =
      Min(header_size +
              ((requested_embedder_fields + requested_in_object_properties)
               << kPointerSizeLog2),
          JSObject::kMaxInstanceSize);
  *in_object_properties = ((*instance_size - header_size) >> kPointerSizeLog2) -
                          requested_embedder_fields;
}

The attached PoC crashes current stable on linux.

See crash report ID: 307546648ba8a84a

This bug is subject to a 90 day disclosure deadline. After 90 days elapse
or a patch has been made broadly available, the bug report will become
visible to the public.


VERSION
Chrome Version: Version 64.0.3282.119 (Official Build) (64-bit)
Operating System: Linux

REPRODUCTION CASE
Please include a demonstration of the security bug, such as an attached
HTML or binary file that reproduces the bug when loaded in Chrome. PLEASE
make the file as small as possible and remove any content not required to
demonstrate the bug.

FOR CRASHES, PLEASE INCLUDE THE FOLLOWING ADDITIONAL INFORMATION
Type of crash: renderer
Crash State: crash id 307546648ba8a84a
Client ID (if relevant): [see link above]

 
poc.html
406 bytes View Download
Components: Blink>JavaScript
Cc: hpayer@chromium.org cbruni@chromium.org verwa...@chromium.org
Labels: Security_Severity-High Security_Impact-Stable OS-Android OS-Chrome OS-Fuchsia OS-Linux OS-Mac OS-Windows Pri-1
Project Member

Comment 4 by ClusterFuzz, Feb 2

ClusterFuzz is analyzing your testcase. Developers can follow the progress at https://clusterfuzz.com/testcase?key=4864701755031552.
Labels: M-64
Calling it M64 since maybe the fix will be simple enough to make it into a maybe-respin of Stable. Otherwise 65.
See attached for a work-in-progress exploit that reaches an int3 on current stable on Linux and Windows.
exploit.html
7.8 KB View Download
Status: Started (was: Unconfirmed)
Owner: cbruni@chromium.org
Wow, nice catch, thanks for the repro!
The CL mentioned in  issue 806388  essentially prevents an exploit by now since it hard crashes on most invalid values for Map properties. But I presume if you construct the right allocation_size that is valid again but still too small you'd end up in the same situation.
WIP patch https://chromium-review.googlesource.com/c/v8/v8/+/902103
It's already late here and I won't make it today I think, what's the deadline for M64? 
We'd need to bake the proper fix for a while I think, but I could write at least a minimal fix that crashes and prevents an exploit which could be easily backmerged (again this would happen tomorrow). 
Cc: ishell@chromium.org
Labels: Merge-Request-65 Merge-Request-64
Project Member

Comment 14 by sheriffbot@chromium.org, Feb 13

Labels: -Merge-Request-65 Merge-Review-65 Hotlist-Merge-Review
This bug requires manual review: M65 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Cc: awhalley@chromium.org
+ awhalley@ for (Security TPM) for merge review.
Project Member

Comment 16 by sheriffbot@chromium.org, Feb 13

Status: Fixed (was: Started)
Please mark security bugs as fixed as soon as the fix lands, and before requesting merges. This update is based on the merge- labels applied to this issue. Please reopen if this update was incorrect.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 17 by sheriffbot@chromium.org, Feb 14

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
awhalley@ are you reviewing this merge?
govind@ - good for 65

cmasso@ - I was waiting on it obtaining sufficient canary/dev coverage.
Labels: -Merge-Review-65 Merge-Approved-65
Approving merge to M65 branch 3325 based on comment #19. Please merge ASAP so we can pick it up for next beta release. Thank you.
Project Member

Comment 21 by bugdroid1@chromium.org, Feb 16

Labels: -Merge-Approved-65
This is already merged at #21.
We're fairly close to M65 stable, let's wait for that.
Labels: -Merge-Request-64 Merge-Rejected-64
Labels: -M-64 M-65
This is causing crashes on M65 Beta (as well as Canary), see issue 813492.
It is best to revert this change from M65 if it is a safe revert as it causing stability issue on M65. 

awhalley@, are you ok with it?
adamk@, will it be safe to revert this change from M65?
Cc: adamk@chromium.org
Owner: jkummerow@chromium.org
jkummerow is investigating what we should do next. It's not clear if reverting the change on M65 is actually safe (it's a CHECK failure, and removing it might lead to worse problems).
Turns out this crash was already found by clusterfuzz and fixed, see  issue 813427 .
Labels: Release-0-M65
Labels: CVE-2018-6065
Labels: CVE_description-missing
Project Member

Comment 34 by sheriffbot@chromium.org, May 23

Labels: -Restrict-View-SecurityNotify allpublic
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Sign in to add a comment