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
Status: Verified
Owner:
Last visit 23 days ago
Closed: Jul 31
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 2
Type: Bug-Security



Sign in to add a comment
Security: NtQueryValueKey may not return null-terminated string
Reported by xiaoyi...@outlook.com, Apr 22 2017 Back to list
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
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
Components: Internals>PlatformIntegration
Comment 3 by meacer@chromium.org, Apr 24 2017
Labels: Security_Severity-Low Security_Impact-Stable OS-Windows
Owner: penny...@chromium.org
Status: Assigned
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?
Project Member Comment 4 by sheriffbot@chromium.org, Apr 25 2017
Labels: Pri-2
Is there any update on this?
Status: Started
Yup.  Underway!
Comment 7 Deleted
Comment 8 Deleted
Project Member Comment 9 by bugdroid1@chromium.org, Jul 31
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

FYI: Landed in M62, first canary branch 3173.
Status: Fixed
Thank you for fixing it! Does this bug qualify for a bug bounty?
Labels: reward-topanel
Project Member Comment 14 by sheriffbot@chromium.org, Aug 1
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Status: Verified
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!
Labels: -reward-topanel reward-0
Will this get a CVE and be acknowledged in the Chrome 62 release note? Thanks!
Cc: awhalley@chromium.org
+Andrew for the CVE question. As this shipped in Stable, I do believe it gets a CVE. 
Labels: M-62
Yep, this will get a CVE allocated when M62 goes stable in a couple of weeks.
Labels: Release-0-M62
Thank you for the fix, and thank you for the info on CVE!
Labels: CVE-2017-15392
Project Member Comment 24 by sheriffbot@chromium.org, Nov 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
Sign in to add a comment