Closed Bug 732233 Opened 8 years ago Closed 8 years ago

We should not trust the driver to not overflow, in WebGL uniform array setters

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla14
Tracking Status
firefox13 + wontfix
firefox14 + fixed
firefox-esr10 14+ fixed

People

(Reporter: bjacob, Assigned: bjacob)

References

Details

(Keywords: sec-vector, Whiteboard: [sg:vector-critical (drivers)][advisory-tracking+][qa-] webgl-conformance webgl-next)

Attachments

(2 files, 2 obsolete files)

Currently, in the implementation of uniform array setters, we are happily passing too-long array sizes to glUniformXXv and relying the the specified behavior that anything that falls outside array boundaries gets silently ignored.

So a call like this,

  gl.uniform4fv(uniV4, [1, 2, 3, 4, 5, 6, 7, 8]);

results in a GL call like this,

  glUniform4fv(location, 2, ptr);

and we are not checking that the uniform array is really of size >= 2. We are relying on the GL to check that.

Instead, we should clamp these sizes ourselves. That requires knowing what the real array sizes are. ANGLE gives us that, and we're already querying that : ShGetActiveUniform in CompileShader. We just need to store these values somewhere and use them.
Group: core-security
bjacob mentions on IRC that this should be s-s.
Duplicate of this bug: 732145
Whiteboard: webgl-conformance
Whiteboard: webgl-conformance → webgl-conformance webgl-next
Try:
  https://tbpl.mozilla.org/?tree=Try&rev=d47add43e96b

Ugly code.

Notes:

 1. the only reasonable way to get uniform array sizes is from ANGLE. See how we query this from ANGLE ShGetActiveUniform in CompileShader. Well, we could also do the crazy approach we do in GetUniform, using GL getters, but it's very convoluted and the ANGLE getter (ShGetActiveUniform) was added in the first place because GL implementations are not trustworthy in this area. So it's the GetUniform implementation that should be rewritten to be like this.

 2. I didn't want to put the array size information in WebGLMappedIdentifier because identifier mapping is pending big changes, see http://code.google.com/p/angleproject/issues/detail?id=315 , it should become purely a parser thing instead of being specific to uniforms/attribs as it currently is.

 3. Sorry that this adds more stuff to the ugly macros implementing uniform getters. We need to refactor all of this anyway to no longer use macros. But for now I wanted a minimal patch, to minimally disrupt Boris's ongoing new-DOM-bindings work and to get this fixed quickly as this is both a security issue and a conformance issue.
Attachment #615218 - Flags: review?(jgilbert)
The try run is expected to give an orange on Windows due to unexpected pass in conformance/more/functions/uniformfArrayLen1.html . That's the last entry in failing_tests_windows.txt! So once we remove it, we'll be fully passing 1.0.1 conformance on the Windows test slaves.
Keywords: sec-vector
Comment on attachment 615218 [details] [diff] [review]
Check uniform array lengths in uniform setters

cancelling review request for now, test failure in conformance/uniforms/gl-uniform-arrays.html
Attachment #615218 - Flags: review?(jgilbert)
This time we're green at least on my machine.

Try:
https://tbpl.mozilla.org/?tree=Try&rev=6b1148729ae1
Attachment #615218 - Attachment is obsolete: true
Attachment #615322 - Flags: review?(jgilbert)
Now with adjusted failing_test_* files. Also note that on Mac, the failing tests list was manipulated directly in the test runner, some of that is now passing with this patch, the rest was redundant with failing_tests_mac.txt, so all is now gone.

Try:
https://tbpl.mozilla.org/?tree=Try&rev=4efbd53b6bd1
Attachment #615353 - Flags: review?(jgilbert)
Attachment #615322 - Attachment is obsolete: true
Attachment #615322 - Flags: review?(jgilbert)
Comment on attachment 615353 [details] [diff] [review]
Check uniform array lengths in uniform setters

Review of attachment 615353 [details] [diff] [review]:
-----------------------------------------------------------------

Exciting, but answer me these questions three. Well, two questions and a style nit.

::: content/canvas/src/WebGLContext.h
@@ +1808,5 @@
>      return true;
>  }
>  
>  typedef nsDataHashtable<nsCStringHashKey, nsCString> CStringHash;
> +typedef nsDataHashtable<nsCStringHashKey, WebGLUniformInfo> CStringToUniformInfoHash;

This does match the above line in shortening 'Hashtable' to 'Hash', but 'Table' or 'Map' would be more descriptive.

@@ +2005,5 @@
> +     * the uniform with given mapped identifier.
> +     *
> +     * Note: the input string |name| is the mapped identifier, not the original identifier.
> +     */
> +    WebGLUniformInfo GetUniformInfoForMappedIdentifier(const nsACString& name) {

If I remember right, this looks familiar. How hard would it be to fold some of this code out to a shared function?

::: content/canvas/src/WebGLContextGL.cpp
@@ +4198,2 @@
>      }                                                                           \
> +    int elementSize = location_object->ElementSize();                           \

Can we not split out these (near-)duplicate code pieces? It would also reduce the backslash spam.
(In reply to Jeff Gilbert [:jgilbert] from comment #8)
> Comment on attachment 615353 [details] [diff] [review]
> Check uniform array lengths in uniform setters
> 
> Review of attachment 615353 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Exciting, but answer me these questions three. Well, two questions and a
> style nit.
> 
> ::: content/canvas/src/WebGLContext.h
> @@ +1808,5 @@
> >      return true;
> >  }
> >  
> >  typedef nsDataHashtable<nsCStringHashKey, nsCString> CStringHash;
> > +typedef nsDataHashtable<nsCStringHashKey, WebGLUniformInfo> CStringToUniformInfoHash;
> 
> This does match the above line in shortening 'Hashtable' to 'Hash', but
> 'Table' or 'Map' would be more descriptive.

You're right. I took this from MXR,
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsNavHistory.h#348

but I agree with you.

> 
> @@ +2005,5 @@
> > +     * the uniform with given mapped identifier.
> > +     *
> > +     * Note: the input string |name| is the mapped identifier, not the original identifier.
> > +     */
> > +    WebGLUniformInfo GetUniformInfoForMappedIdentifier(const nsACString& name) {
> 
> If I remember right, this looks familiar. How hard would it be to fold some
> of this code out to a shared function?

Probably reminds you of the MapIdentifier and ReverseMapIdentifier functions just above in this file. It's possible that some could be factored together. But once ANGLE bug 315 is fixed, these MapIdentifier functions are going to change significantly as they will no longer be specific to uniforms and attribs, and will instead map arbitrary tokens. So, I would like to keep things this way for now. See bug 744382.


> 
> ::: content/canvas/src/WebGLContextGL.cpp
> @@ +4198,2 @@
> >      }                                                                           \
> > +    int elementSize = location_object->ElementSize();                           \
> 
> Can we not split out these (near-)duplicate code pieces? It would also
> reduce the backslash spam.

We can, but we should go even farther and completely refactot this code to use functions, not macros. I've just filed bug 745840 for that.
Comment on attachment 615353 [details] [diff] [review]
Check uniform array lengths in uniform setters

Review of attachment 615353 [details] [diff] [review]:
-----------------------------------------------------------------

R+ after comments, though please do rename CStringToUniformInfoHash.
Attachment #615353 - Flags: review?(jgilbert) → review+
http://hg.mozilla.org/mozilla-central/rev/b56db6eab47c

(did the renaming, also on CStringHash)
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
Comment on attachment 615353 [details] [diff] [review]
Check uniform array lengths in uniform setters

[Approval Request Comment]
Regression caused by (bug #): We've had this bug since Firefox 4
User impact if declined: Still no protection from certain buffer overflow vulnerabilities in certain OpenGL drivers
Testing completed (on m-c, etc.): Been on m-c for a few days
Risk to taking this patch (and alternatives if risky): The risk would be that it doesn't fix the problem and users still aren't protected, if there is an unforeseen bug. But it can't be worse security-wise than not taking the patch. Alternatively, an unforeseen bug could also cause WebGL pages to not display correctly... but that risk seems low and wouldn't be a terrible thing to happen, while the security problem fixed by this patch is real.
String changes made by this patch: none
Attachment #615353 - Flags: approval-mozilla-aurora?
Comment on attachment 615353 [details] [diff] [review]
Check uniform array lengths in uniform setters

[Triage Comment]
Approved for Aurora 13 based upon security interests.
Attachment #615353 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 747619
Ouch. patch doesn't apply cleanly on aurora, and I'd rather land this together with the fix for bug 747619 which is a regression caused by this patch.
Whiteboard: webgl-conformance webgl-next → [sg:vector-critical (drivers)] webgl-conformance webgl-next
Depends on: 751643
Is there a way to verify this fix?
Wontfix'ing for Firefox 13, because the fix would be a too large patch (larger than it was for 14+), not worth the risk.
Are we still tracking this for ESR 13+ now or is it going to be later (or should we won't fix this for ESR)?
No longer depends on: 751643
Depends on: 751643
This seems too heavy to backpoint to ESR, as ESR is based on Firefox 10 and this patch relies on large changes landed in 14.
No longer depends on: 747619
s/backpoint/backport
Duplicate of this bug: 745298
(In reply to Benoit Jacob [:bjacob] from comment #18)
> This seems too heavy to backpoint to ESR, as ESR is based on Firefox 10 and
> this patch relies on large changes landed in 14.

We'd really like to address this on the ESR - we're willing to take the additional risk since we don't expect our enterprise community to be affected by WebGL regressions. They could be attacked on this vector though.

Are there any other alternatives to the large patch? Driver blocklists, etc.?
There is another way that I can think of, but it will be a serious performance hit. Indeed, this information can in principle be queried from the GL, but in a very convoluted and inefficient way. Actually, we already do something like that in WebGLContext::GetUniform, and that is complicated and inefficient for sure. Fortunately, WebGLContext::GetUniform is practically never called (and when it's called it's typically as a debugging helper) so we don't care about its performance. To use this approach to fix this bug, we would have to do this in the more performance-sensitive WebGLContext::GetUniformLocation.
...however, since ESR users give a rat's ass about WebGL, I suppose we might as well go for it.
Here's the patch implementing that idea. To be as small as possible, this patch just calls the existing unchanged GetUniform method in where we generate WebGLUniformLocation objects.

Try:
https://tbpl.mozilla.org/?tree=Try&rev=7d10f2615f30

I expect mochitest-1 oranges due to unexpected-passes. Will address this in the failing_test_*.txt files in the next version of this patch.
Attachment #627228 - Flags: review?(jgilbert)
Tryserver is failing with:

configure: error: SDK not found.  When using --with-macos-sdk, you must
specify a valid SDK.  SDKs are installed when the optional cross-development
tools are selected during the Xcode/Developer Tools installation.

Is pushing ESR10 to try not supported?
Attachment #627228 - Flags: review?(jgilbert) → review+
Another try...
https://tbpl.mozilla.org/?tree=Try&rev=c9d6d8610e0e

If pushing-esr10-to-try still doesn't work, I don't know what to do. If it's OK to have M1 oranges on esr10 for a few hours, I could push and land the follow-up patch updating the lists of expected-to-fail tests a few hours later.
Failed again. OK to try what I suggested in comment 26, i.e. going ahead with the push to esr10 and mopping the mochitest floor a few hours after?
(In reply to Benoit Jacob [:bjacob] from comment #26)
> Another try...
> https://tbpl.mozilla.org/?tree=Try&rev=c9d6d8610e0e
> 
> If pushing-esr10-to-try still doesn't work, I don't know what to do. If it's
> OK to have M1 oranges on esr10 for a few hours, I could push and land the
> follow-up patch updating the lists of expected-to-fail tests a few hours
> later.

Yes, it's ok to have M1 oranges on the esr branch if you can push and then get that back to green a few hours after.  Please go ahead and nominate what you intend to land for esr.
Comment on attachment 627228 [details] [diff] [review]
inefficient but non-intrusive patch for ESR10

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: It's marked sg:vector but that is optimistic, as it's very likely that drivers to allow reading illegal memory, so in practice it's more like sg:high.
User impact if declined: Keeping this (potential, but very likely) vulnerability to illegal memory reads in ESR10
Fix Landed on Version: 14
Risk to taking this patch (and alternatives if risky): could break WebGL rendering if something went wrong, but normally the conformance tests (webgl mochitest) would catch that
String or UUID changes made by this patch: none
Attachment #627228 - Flags: approval-mozilla-esr10?
Attachment #627228 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
http://hg.mozilla.org/releases/mozilla-esr10/rev/e0efd9847069

This WILL BE ORANGE and the oranges will be fixed in a followup push later today.
Specifically, mochitest-1 will be orange. Other test suites are not expected to be orange.
http://hg.mozilla.org/releases/mozilla-esr10/rev/3488a16b8dc0

Now I am hoping that this will be all green.
https://hg.mozilla.org/releases/mozilla-esr10/rev/0d2158c1541d

Now this will really be green. At least the mochitest fully passes on my machine which it didn't before (but I was wrongly thinking to be caused by a driver issue).

Sorry for the sketchy landing, I think I underestimated how far m-c and esr10 had diverged.
all backed out:
https://hg.mozilla.org/releases/mozilla-esr10/rev/28d350e964fc

will look into remaining failures tomorrow.
Re-landed:
https://hg.mozilla.org/releases/mozilla-esr10/rev/b53cce148fbd

The only difference is I updated the list of known-to-fail tests on Mac:

- removed uniform[if]BadArgs.html as they are now passing
- added uniformfArrayLen1.html as it is now failing. It sucks to have a test regression, but this is the best compromise I could find: this is a not-so-important test about the difference between uniform non-arrays and uniform arrays of length 1; It was only succeeding before on Mac as the drivers on our Mac slaves were getting it right, but in general drivers often dont get it right so we were failing on this for many users anyway, for example the drivers on our Windows and Linux slaves get it wrong and so this test is already known as failing there; finally, getting this really right like on m-c would require very intrusive changes.
Whiteboard: [sg:vector-critical (drivers)] webgl-conformance webgl-next → [sg:vector-critical (drivers)][advisory-tracking+] webgl-conformance webgl-next
Is there something QA can do to verify this fix?
Whiteboard: [sg:vector-critical (drivers)][advisory-tracking+] webgl-conformance webgl-next → [sg:vector-critical (drivers)][advisory-tracking+][qa?] webgl-conformance webgl-next
Not really; if we made a testcase hammering uniform array setters to try to scribble video memory, maybe one could observe visible symptoms depending on drivers, but we have so many other things to do, and we have good confidence already that this is working, that I'm not advocating us doing that.
Thanks Benoit, given that I'm untracking this for QA verification.
Whiteboard: [sg:vector-critical (drivers)][advisory-tracking+][qa?] webgl-conformance webgl-next → [sg:vector-critical (drivers)][advisory-tracking+][qa-] webgl-conformance webgl-next
Group: core-security
You need to log in before you can comment on or make changes to this bug.