Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Issue 660854 Security: Incorrect validation of CopyBufferSubData in ANGLE
Starred by 2 users Reported by tjbecker...@gmail.com, Oct 31 Back to list
Status: Fixed
Owner:
Closed: Nov 11
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Security



Sign in to add a comment
VULNERABILITY DETAILS
Please provide a brief explanation of the security issue.

In src/third_party/angle/src/libGLESv2/entry_points_gles_3_0.cpp, in the
function CopyBufferSubData, there is incorrect validation of the
readOffset and writeOffset values:

        if (readOffset < 0 || writeOffset < 0 || size < 0 ||
            static_cast<unsigned int>(readOffset + size) > readBuffer->getSize() ||
            static_cast<unsigned int>(writeOffset + size) > writeBuffer->getSize())
        {
            context->handleError(Error(GL_INVALID_VALUE));
            return;
        }

readOffset and writeOffset are both type GLintptr, which is 64 bits long.
static_cast<unsigned int>(readOffset + size) can result in truncation if
readOffset + size exceeds 32 bits, causing the validation to pass
with invalid offsets.

This is the only validation of these arguments that occurs on the GPU process.

Note that this is only reachable via ES3, but this is now enabled by default
on Windows: https://codereview.chromium.org/2354423002

VERSION
Chrome Version: 55.0.2873.0 + beta
Operating System: Windows

REPRODUCTION CASE
A demonstration of the incorrect validation is attached as poc.cc.
 
poc.cc
2.0 KB View Download
Components: Internals>GPU>ANGLE
Labels: Security_Severity-Medium Security_Impact-Stable OS-All
Owner: zmo@chromium.org
Status: Assigned
zmo@, I wonder if you can take a look at this. Thanks!
Project Member Comment 2 by sheriffbot@chromium.org, Nov 1
Labels: M-55
Project Member Comment 3 by sheriffbot@chromium.org, Nov 1
Labels: Pri-1
Cc: kbr@chromium.org geoffl...@chromium.org cwallez@chromium.org
Owner: jmad...@chromium.org
jmadill: can you take this?

FWIW: chrome's command buffer validation handles overflow checking, so it won't reach ANGLE. That said, it would be nice for ANGLE to beef up.
Status: Started
Looking.
zmo@, could point me at where this validation happens in the command buffer?
Project Member Comment 7 by bugdroid1@chromium.org, Nov 2
The following revision refers to this bug:
  https://chromium.googlesource.com/angle/angle/+/d2f0c74c8517365302fffcc2dc7b86778c2a6211

commit d2f0c74c8517365302fffcc2dc7b86778c2a6211
Author: Jamie Madill <jmadill@chromium.org>
Date: Wed Nov 02 14:34:41 2016

Use safe math in ValidateCopyBufferSubData.

This should fix any potential out of bounds reads/writes.

BUG=chromium:660854

Change-Id: Iffa00e4551d7362115cbf023a09b1d0e15f724c8
Reviewed-on: https://chromium-review.googlesource.com/405816
Commit-Queue: Jamie Madill <jmadill@chromium.org>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>

[modify] https://crrev.com/d2f0c74c8517365302fffcc2dc7b86778c2a6211/src/libANGLE/validationES3.cpp
[modify] https://crrev.com/d2f0c74c8517365302fffcc2dc7b86778c2a6211/src/tests/gl_tests/BufferDataTest.cpp

Labels: reward-topanel
Nice find! Thanks for the report.
Project Member Comment 9 by bugdroid1@chromium.org, Nov 4
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b42220eced72b0a5333ba28c6dd7ccf3a5178d40

commit b42220eced72b0a5333ba28c6dd7ccf3a5178d40
Author: ynovikov <ynovikov@chromium.org>
Date: Fri Nov 04 19:27:25 2016

Roll ANGLE eb66a6e..bbe9fb5

https://chromium.googlesource.com/angle/angle.git/+log/eb66a6e..bbe9fb5

BUG=None,chromium:568170,chromium:660854,chromium:661592

TEST=bots

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel

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

[modify] https://crrev.com/b42220eced72b0a5333ba28c6dd7ccf3a5178d40/DEPS

Status: Fixed
Project Member Comment 11 by sheriffbot@chromium.org, Nov 11
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Project Member Comment 12 by sheriffbot@chromium.org, Nov 13
Labels: Merge-Request-55
Labels: -Merge-Request-55 Merge-Review-55 Hotlist-Merge-Review
[Automated comment] DEPS changes referenced in bugdroid comments, needs manual review.
I don't think this is necessary to merge. As zmo says in #4, command buffer should filter invalid GL calls from reaching ANGLE.

Mo can you confirm?
Cc: awhalley@chromium.org
+awhalley@ (Security TPM).
Cc: zmo@chromium.org
zmo@, could you please reply to comment #14. Thank you.
Yes, I agree with jmadill.  We don't have a security issue in Chrome as far as this bug is concerned, so no need to merge.
Also, FYI, merging this change should not be terribly difficult. It may not have a dependency, would have to check, but I don't think so.
If merge to M55 is NOT is needed, then please remove "Merge-Review-55" label. Thank you.
Labels: -Merge-Review-55
Labels: -Hotlist-Merge-Review -reward-topanel reward-0
No reward for this one as it can't be triggered from Chrome, per comment 17
That's too bad - this bug would become triggered from Chrome once we complete issue 602688 and ship it on Windows. It would be unfortunate to discourage people from looking at bugs in ANGLE's validation that will become serious security issues in the future.
Labels: reward-topanel
From issue 602688 "while still performing security checks" :-)

Thanks for the heads up - I'll send this back to the panel to reconsider given that information.

Labels: Release-0-M55
Labels: -reward-topanel reward-unpaid reward-1000
On second thoughts, the panel decided to reward $1,000 for this bug, thanks!
Awesome!
That's amazing! Thank you!
Labels: -reward-0
Labels: -reward-unpaid reward-inprocess
Labels: CVE-2016-5221
Project Member Comment 32 by sheriffbot@chromium.org, Feb 17
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