I think there's been a basic assumption for a while that the GL driver won't attempt to access the data for unused vertex attributes that are enabled as arrays. SwiftShader was probably the first to do this.

Agree that we could disable such arrays explicitly. We can't bounds-check the unused arrays since WebGL has provided the guarantee to applications for a while that the unused vertex attributes will be ignored.

The shader in question declares 7 vertex attributes (one mat3 and one mat4), and all of them are accessed. Attribute 1 has an enabled vertex array with no data in it. Hence the crash.

SwiftShader does check which attributes are declared by the shader before reading them. The code for this directly descends from ANGLE. Either way this case isn't affected by that. I think ANGLE doesn't crash merely due to robustness guarantees for out-of-bounds accesses on most GPU hardware. SwiftShader does not (yet) make such guarantees.

Note also that the OpenGL ES 2.0 spec states that "The command void DrawArrays( enum mode, int first, sizei count ); constructs a sequence of geometric primitives by successively transferring elements first through first + count − 1 of each enabled array to the GL." The ARB_vertex_buffer_object extension makes it clear that this may result in program termination, and EXT_robustness explicitly refers to that case being precluded.

This is a serious security regression. If you are not able to fix this quickly, please revert the change that introduced it.

If this doesn't affect a release branch, or has not been properly classified for severity, please update the Security_Impact or Security_Severity labels, and remove the ReleaseBlock label. To disable this altogether, apply ReleaseBlock-NA.

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

Ok, so there's actually several parts.
1- Whether the attributes are actually used depends on the driver optimizations. E.g. in this particular case the NVIDIA driver doesn't use them because the fragment shader doesn't use the result of the vertex shader, so it does extensive DCE, and reports all of them as unused. We rely on driver data to decide whether attributes are used.
2- matrix attributes are mishandled by the command buffer logic and don't count all the locations that are used. I think this is the main issue on  crbug.com/736639 
3- we currently rely on the driver not pulling data out of disabled attributes, even though that's not guaranteed by the spec, as mentioned above.


(1) is not a fundamental security problem, but we may get different observable behavior on different drivers. We could decide to do liveness analysis in the translator and apply consistently behavior. This is a lot of work.
(2) is not too difficult to fix, I have a WIP CL - mostly needs more tests at this point
(3) is not too hard to fix logically, simply disabling unused attributes, but I worry about run-time cost (we may need to enable/disable attributes on a per-draw-call basis). I have some ideas about how to limit the cost using bitfields, similar to how we match vertex data types to attribute types on ES3.

Forking items (1) and (3) to another bug, since they are not blocking (either low impact security or not a security issue).

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0b77eb162eec2286c5b7e9a84c2f25362e39b7ad

commit 0b77eb162eec2286c5b7e9a84c2f25362e39b7ad
Author: Antoine Labour <piman@chromium.org>
Date: Sat Jul 08 04:00:44 2017

Fix potential OOB reads in matrix attributes

This fixes how we handle matrix attributes to properly take into account
all the locations that they use instead of just the first one.

Bug:  736639 ,  738228 
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: I0ca7d9b8a962d7de13016f5c04d5ab1d7348f514
Reviewed-on: https://chromium-review.googlesource.com/553746
Reviewed-by: Antoine Labour <piman@chromium.org>
Reviewed-by: Kenneth Russell <kbr@chromium.org>
Commit-Queue: Antoine Labour <piman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#485138}
[modify] https://crrev.com/0b77eb162eec2286c5b7e9a84c2f25362e39b7ad/gpu/BUILD.gn
[modify] https://crrev.com/0b77eb162eec2286c5b7e9a84c2f25362e39b7ad/gpu/command_buffer/service/program_manager.cc
[modify] https://crrev.com/0b77eb162eec2286c5b7e9a84c2f25362e39b7ad/gpu/command_buffer/service/program_manager.h
[modify] https://crrev.com/0b77eb162eec2286c5b7e9a84c2f25362e39b7ad/gpu/command_buffer/service/program_manager_unittest.cc
[add] https://crrev.com/0b77eb162eec2286c5b7e9a84c2f25362e39b7ad/gpu/command_buffer/tests/gl_oob_attrib_unittest.cc

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