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 2 users

Issue metadata

Status: Verified
Owner:
Last visit > 30 days ago
Closed: Jul 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 2
Type: Bug-Security



Sign in to add a comment
link

Issue 714401: Security: NtQueryValueKey may not return null-terminated string

Reported by xiaoyi...@outlook.com, Apr 22 2017

Issue description

VULNERABILITY DETAILS
In the file chromium\src\chrome_elf\nt_registry\nt_registry.cc, function nt::QueryRegValueSZ uses NtQueryValueKey to get Windows registry values, but this function doesn't ensure |value_bytes| is null-terminated before using it to initialize a std::wstring object.

The MSDN document for RegQueryValueEx says:[1]
> If the data has the REG_SZ, REG_MULTI_SZ or REG_EXPAND_SZ type, the string may not have been stored with the proper terminating null characters. Therefore, even if the function returns ERROR_SUCCESS, the application should ensure that the string is properly terminated before using it; otherwise, it may overwrite a buffer.

The same is true for NtQueryValueKey.

The potential risk for this issue is that if Chrome happens to read a registry data, set by a third party program, that is not null-terminated, then it can potentially cause buffer over-read in Chrome. This is quite unlikely to be exploitable though, because remote attackers can't modify registries or force Chrome to read a certain reg value.

Firefox has the same issue, and we are working on a fix.

[1] https://msdn.microsoft.com/en-us/library/windows/desktop/ms724911(v=vs.85).aspx

VERSION
Chrome Version: all
Operating System: Windows only

REPRODUCTION CASE
Attached PoC.zip is C++ source code to demonstrate nt::QueryRegValueSZ may disclose out-of-bound memory, if a REG_SZ was not null-terminated when it was set.
 
PoC.zip
20.5 KB Download

Comment 1 by xiaoyi...@outlook.com, Apr 22 2017

Here is a patch. It passes a length to wstring constructor, so that it won't over-read.
0001-Ensure-strings-read-from-Windows-Registry-are-null-t.patch
1.2 KB Download

Comment 2 by elawrence@chromium.org, Apr 24 2017

Components: Internals>PlatformIntegration

Comment 3 by mea...@chromium.org, Apr 24 2017

Labels: Security_Severity-Low Security_Impact-Stable OS-Windows
Owner: penny...@chromium.org
Status: Assigned (was: Unconfirmed)
xiaoyin.l: Thanks for the report and the patch. Assigning low severity as this doesn't seem exploitable without running a third party binary.

Penny, can you please take a look?

Comment 4 by sheriffbot@chromium.org, Apr 25 2017

Project Member
Labels: Pri-2

Comment 5 by xiaoyi...@outlook.com, Jun 3 2017

Is there any update on this?

Comment 6 by penny...@chromium.org, Jun 9 2017

Status: Started (was: Assigned)
Yup.  Underway!

Comment 7 Deleted

Comment 8 Deleted

Comment 9 by bugdroid1@chromium.org, Jul 31 2017

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/42c976267d61dceba3ad216d7e475288feeed409

commit 42c976267d61dceba3ad216d7e475288feeed409
Author: pennymac <pennymac@chromium.org>
Date: Mon Jul 31 17:38:40 2017

[NtRegistry] Ensure REG_SZ and REG_MULTI_SZ are null terminated.

Enforced in base QueryRegKeyValue function.

TBR=robertshield
BUG= 714401 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng

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

[modify] https://crrev.com/42c976267d61dceba3ad216d7e475288feeed409/chrome_elf/nt_registry/nt_registry.cc
[modify] https://crrev.com/42c976267d61dceba3ad216d7e475288feeed409/chrome_elf/nt_registry/nt_registry.h
[modify] https://crrev.com/42c976267d61dceba3ad216d7e475288feeed409/chrome_elf/nt_registry/nt_registry_unittest.cc

Comment 10 by penny...@chromium.org, Jul 31 2017

FYI: Landed in M62, first canary branch 3173.

Comment 11 by penny...@chromium.org, Jul 31 2017

Status: Fixed (was: Started)

Comment 12 by xiaoyi...@outlook.com, Jul 31 2017

Thank you for fixing it! Does this bug qualify for a bug bounty?

Comment 13 by wfh@chromium.org, Jul 31 2017

Labels: reward-topanel

Comment 14 by sheriffbot@chromium.org, Aug 1 2017

Project Member
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify

Comment 15 by penny...@chromium.org, Aug 3 2017

Status: Verified (was: Fixed)

Comment 16 by awhalley@google.com, Aug 8 2017

Hello xiaoyin.l@ - the VRP panel looked at this and I'm afraid to say they declined to reward, given the threat model of a malicious registry entry. Many thanks for the report!

Comment 17 by awhalley@google.com, Aug 8 2017

Labels: -reward-topanel reward-0

Comment 18 by xiaoyi...@outlook.com, Oct 12 2017

Will this get a CVE and be acknowledged in the Chrome 62 release note? Thanks!

Comment 19 by elawrence@chromium.org, Oct 12 2017

Cc: awhalley@chromium.org
+Andrew for the CVE question. As this shipped in Stable, I do believe it gets a CVE.

Comment 20 by awhalley@google.com, Oct 12 2017

Labels: M-62
Yep, this will get a CVE allocated when M62 goes stable in a couple of weeks.

Comment 21 by awhalley@google.com, Oct 16 2017

Labels: Release-0-M62

Comment 22 by xiaoyi...@outlook.com, Oct 18 2017

Thank you for the fix, and thank you for the info on CVE!

Comment 23 by awhalley@chromium.org, Oct 18 2017

Labels: CVE-2017-15392

Comment 24 by sheriffbot@chromium.org, Nov 7 2017

Project Member
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

Comment 25 by awhalley@chromium.org, Apr 25 2018

Labels: CVE_description-submitted

Sign in to add a comment