Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Issue 651443 Security: Histogram Type Confusion Crashes the Browser Process
Starred by 1 user Reported by secur...@cesg.gsi.gov.uk, Sep 29 2016 Back to list
Status: Fixed
Owner:
Closed: Sep 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug-Security



Sign in to add a comment
CESG Vulnerability Report - Google Chrome - September 2016

CESG REF: 55489501 / VULNERABILITY ID: 429718

Summary
-------
A type confusion vulnerability has been discovered in the histogram collector feature of Google Chrome.  It affects the latest version for which source code is available, Chrome 55.  The vulnerability was discovered in the 32-bit build of Chromium on Windows, although it may also affect the 64-bit build.  The vulnerability can be used to crash the browser process.


CVSS
----
This vulnerability has a Severity Score of 4.9 and a Medium Severity Rating: AV:L/AC:L/Au:N/C:N/I:N/A:C.  The Common Weakness Enumeration ID is CWE-843: Access of Resource Using Incompatible Type ('Type Confusion').


Details
-------
In order to track performance and usage metrics throughout the browser, Chrome makes use of a number of feature-specific histograms. Statistics for these histograms are collected within each of the processes (including the un-sandboxed browser process and sandboxed renderer processes), and collected by the browser process. Renderer processes can send histogram data to the browser via the ChildProcessHostMsg_ChildHistogramData IPC message, which includes a std::vector of histograms that have been serialised into strings.

The browser process receives these serialised histograms and starts processing them in base::DeserializeHistogramInfo. This function reads an integer from the serialised histograms which defines what type of histogram it is, and calls a deserialization function based on this value. This function then reads more data, depending on the type of the histogram provided. In all cases, this data includes a string for the name of the histogram which is used to identify it.

Once this data has been read, the corresponding FactoryGet function is called. The job of this function is to return a histogram with the requested name, either by retrieving the existing histogram with that name or by creating one if none exist. Depending on the type of histogram, checks may be performed on the instance returned by the FactoryGet function to ensure that the histogram is not corrupt (such as the checks carried out by the ValidateRangeChecksum function). Once a valid histogram has been retrieved, the included data samples are deserialised and recorded to the histogram.

Histograms track their type using their histogram_type_ member variable, but instances of histogram objects are referenced using a pointer to their common base class, HistogramBase. The following diagram shows the class hierarchy for histogram objects.

==========================================================================
			-----------------
			| HistogramBase |
			-----------------
				|
		--------------------------------
		|				|
	-------------------		-------------			
	| SparseHistogram |		| Histogram |
	-------------------		-------------
						|
				-------------------------
				|			|
			-------------------	-------------------
			| LinearHistogram |	| CustomHistogram |
			-------------------	-------------------
				|
				|
			--------------------				
			| BooleanHistogram |
			--------------------
==========================================================================

The general pattern for processing serialised histograms is as follows:
•	read the serialised arguments;
•	lookup the histogram by name, creating one if one doesn’t exist with that name;
•	optionally, perform some checks that the returned histogram is valid;
•	record the data to the histogram.

One of the check functions, ValidateRangeChecksum, is implemented as follows.

/base/metrics/histogram.cc
==========================================================================
bool ValidateRangeChecksum(const HistogramBase& histogram,
                           uint32 range_checksum) {
  const Histogram& casted_histogram =
      static_cast<const Histogram&>(histogram);
  return casted_histogram.bucket_ranges()->checksum() == range_checksum;
}
==========================================================================

The checksum property of a histogram is something only implemented by descendants of the Histogram class, so the specified histogram is cast to a const Histogram&. This would appear to be safe, as this function is not called when deserialising a SparseHistogram.  There are no checks, however, to ensure that the histogram returned from FactoryGet is of the correct type--there is a DCHECK macro call to confirm this, but this code is not present in a release configuration.

To trigger this vulnerability, it is possible to send a single IPC message containing two serialised histograms. The first should be a sparse histogram with a name that has not been registered before. This will cause a SparseHistogram object to be registered using the given name. The second can be of any type other than a sparse histogram, with the same name as the first one. This will cause the FactoryGet function to return the SparseHistogram created when processing the first histogram, and then call the ValidateRangeChecksum function on it.

This results in a bad cast from SparseHistogram to Histogram, and the subsequent access of the bucket_ranges_ and checksum_ fields.


Impact
------
To confirm the impact of this bug, it is necessary to look at the memory layout of both object types. Because of the size of the objects involved, the invalid accesses happen within the memory of the objects, as opposed to accessing adjacent memory on the heap. The following diagram describes the layout of the two objects in a 32-bit build of Chromium.

==========================================================================
	chrome!base::SparseHistogram	chrome!base::Histogram
0x0	__VFN_table			__VFN_table
0x4	histogram_name_			histogram_name_
0x8		
0xc		
0x10		
0x14		
0x18		
0x1c	flags_				flags_
0x20	lock_				bucket_ranges_
0x24					declared_min_
0x28					declared_max_
0x2c					samples_
0x30		
0x34		
0x38	samples_	
==========================================================================

From the output above, it can be seen that by interpreting a SparseHistogram as a Histogram and calling the bucket_ranges() function on it, it will return a pointer to the lock_ member of the SparseHistogram. In normal usage this value is null, as the lock is only acquired when adding samples to the histogram. Even if we assumed that it was possible to race two threads such that the lock is acquired when the call takes place, the next call retrieves the checksum value (which lines up with the owning_thread_ref field of the lock_ member), dereferences it, and compares it to a value we have provided. As a result, the likely outcome of exploiting this vulnerability is a near-NULL pointer dereference.


Mitigation
----------
A patch to correct this issue should include substituting the DCHECK calls in FactoryGet which check the type of the returned histogram with code checks that return an error condition when possible. In addition to these checks, there are a number of areas in the code that processes histograms which contain errors or broken checks.  These can cause additional errors in the browser process.

An example of this is the FactoryGet methods for the Histogram and LinearHistogram types.  These call the InspectConstructionArguments function to do additional validation of the arguments passed in, and return a Boolean value indicating whether or not they are valid. This value is only checked using a DCHECK macro, however, so in a release configuration the function essentially does not perform any checks. Even worse than this, the function modifies its arguments in place, which means that some conditions that were previously checked can be invalidated. 

For example, the code in ReadHistogramArguments checks that the minimum sample value is greater than or equal to the maximum sample value, and that both values are a positive integer. However, if both values are set to INT_MAX (0x7FFFFFFF), InspectConstructionArguments sets the maximum sample value to INT_MAX – 1, and later returns false to indicate that the arguments were invalid. This is ignored, and the histogram continues being created with the minimum sample value one greater than the maximum. When creating the ranges for this histogram, the sample value overflows to a large negative number, and a CHECK is subsequently triggered in BucketRanges::set_range(). 


Bug Bounty Payment
------------------
If this vulnerability is eligible for a Bug Bounty payment we ask that the money be donated directly to Engineering Development Trust (England and Wales Registered Charity 1156066 and in Scotland SC039635), www.etrust.org.uk/donate-edt-0.

Please contact the CESG mailbox to inform us of the donation amount and the donation date.


CESG Contact Information
------------------------
The vulnerability disclosure mailbox is ‘security@cesg.gsi.gov.uk’.  Please contact us for our PGP key. 


NCSC—National Cyber Security Centre
-----------------------------------
As announced by the Chancellor of the Exchequer in 2015, CESG will become part of the NCSC in autumn this year, and from this date vulnerability disclosures will be from NCSC. More details about the NCSC can be found in the National Cyber Security Centre Prospectus (www.gov.uk/government/publications/national-cyber-security-centre-prospectus).


Verification, Resolution and Release
------------------------------------
Please inform CESG via the ‘security@cesg.gsi.gov.uk’ mailbox, quoting the CESG Reference above, should you:
•	confirm that this is a security issue;
•	allocate the issue a CVE identifier;
•	determine a date to release a patch;
•	determine a date to publish advisories.


CESG Disclosure Policy
----------------------
CESG has adopted the ISO 29147 approach to vulnerability disclosure, and as-such follows a co-ordinated disclosure approach with affected parties.  We have never publicly disclosed a vulnerability prior to a fix being made available.

CESG recognises that vendors need a reasonable amount of time to mitigate a vulnerability, for example, to understand the impact to customers, to triage against other vulnerabilities, to implement a fix in coordination with others, and to make that fix available to its customers.  As this will vary based on the exact situation CESG does not define a set time frame in which a fix must be made available, and we are happy to discuss the circumstances of any particular disclosure.

If CESG believes a vendor is not making appropriate progress with vulnerability resolution, we may, after discussion with the vendor, choose to share the details appropriately (for example, with service providers and our customers) to ensure that we provide appropriate mitigation of the threat to the UK and to UK interests.


Terms of Reference
------------------
Please note, any CESG findings and recommendations made have not been provided with the intention of avoiding all risks, and following the recommendations will not remove all such risk.  Ownership of information risks remains with the relevant system owner at all times.

(c) Crown Copyright 2016.  CESG shall at all times retain Crown copyright in this document and the permission of CESG must be sought in advance if you want to copy, republish, translate or otherwise reproduce all or any part of the document, or disclose or release all or any part of it to another person.
 
Comment 1 by kenrb@chromium.org, Sep 29 2016
Components: Internals>Metrics
Labels: Security_Severity-Low Security_Impact-Stable OS-All Pri-2
Owner: kenrb@chromium.org
Status: Assigned
Thank you for the report and detailed analysis.

We do not normally consider process crashes to be security bugs (as described here: https://www.chromium.org/Home/chromium-security/security-faq#TOC-Are-denial-of-service-issues-considered-security-bugs-), however a crash resulting from a bad cast in the browser process is concerning.

Your analysis that this is not exploitable aside from a near-NULL pointer dereference looks sound. There is still risk, however, that it could become a lot more severe due to a future change to the memory layout of one or both of the Histogram and SparseHistogram classes. In that case this flaw could represent a sandbox escape vulnerability.

I think the best approach would be to change the DCHECK you mentioned into a CHECK, so that release builds will reliably crash before the bad cast takes place. It does not address the DoS aspect of it, but in other cases we take the approach of terminating the browser process immediately when there is a signal that a sandboxed renderer process might be compromised. A CHECK would be consistent with that philosophy.

I am flagging this as a low-severity security bug out of an abundance of caution, in the event there is some malicious use of this flaw that hasn't been identified.
Comment 2 by kenrb@chromium.org, Sep 29 2016
Cc: isherman@chromium.org
Project Member Comment 3 by bugdroid1@chromium.org, Sep 29 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/03c2e97746a2c471ae136b0c669f8d0c033fe168

commit 03c2e97746a2c471ae136b0c669f8d0c033fe168
Author: kenrb <kenrb@chromium.org>
Date: Thu Sep 29 20:50:57 2016

Convert DCHECKs to CHECKs for histogram types

When a histogram is looked up by name, there is currently a DCHECK that
verifies the type of the stored histogram matches the expected type.

A mismatch represents a significant problem because the returned
HistogramBase is cast to a Histogram in ValidateRangeChecksum,
potentially causing a crash.

This CL converts the DCHECK to a CHECK to prevent the possibility of
type confusion in release builds.

BUG= 651443 
R=isherman@chromium.org

Review-Url: https://codereview.chromium.org/2381893003
Cr-Commit-Position: refs/heads/master@{#421929}

[modify] https://crrev.com/03c2e97746a2c471ae136b0c669f8d0c033fe168/base/metrics/histogram.cc
[modify] https://crrev.com/03c2e97746a2c471ae136b0c669f8d0c033fe168/base/metrics/sparse_histogram.cc

Comment 4 by kenrb@chromium.org, Sep 30 2016
Status: Fixed
Project Member Comment 5 by sheriffbot@chromium.org, Oct 1 2016
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Won't using a CHECK make it trivial to crash the browser process? It would be nice to detect this (by returning nullptr?) and killing the child process instead.
Comment 7 by kenrb@chromium.org, Oct 3 2016
dcheng@: We could, but it doesn't really matter. There are ways to DoS the browser process from just web content, which are more significant. Attackers compromising renderer processes just for the purpose of crashing the browser is not realistic.
Should we file separate bugs for the examples mentioned in the Mitigation section, per:

"In addition to these checks, there are a number of areas in the code that processes histograms which contain errors or broken checks.  These can cause additional errors in the browser process." 

Seems like the CL didn't address those?
Comment 9 by kenrb@chromium.org, Oct 3 2016
asvitkine: Yes, I will file those as separate non-security bugs. More broadly, this code seems to warrant an audit. These bugs are the sort of thing we are supposed to catch in IPC reviews.
Sounds good, thanks!
kenrb@: that's true, but the ipc security guidelines say specifically not to CHECK: https://www.chromium.org/Home/chromium-security/education/security-tips-for-ipc#TOC-Safely-handle-known-bad-input.
CESG became a part of the UK's National Cyber Security Centre (NCSC) in October 2016.  We would therefore like to ask for any public acknowledgement for the discovery of this vulnerability to be given to "the UK's National Cyber Security Centre (NCSC)", not "CESG" as we originally requested in our disclosure report.
Project Member Comment 13 by sheriffbot@chromium.org, Jan 7
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
Labels: Release-0-M56
Labels: CVE-2017-5023
Sign in to add a comment