Closed Bug 1110488 (CVE-2015-0830) Opened 9 years ago Closed 9 years ago

webgl shader compilation log strcpy not allocated memory

Categories

(Core :: Graphics: CanvasWebGL, defect)

34 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37
Tracking Status
firefox34 --- wontfix
firefox35 --- wontfix
firefox36 --- fixed
firefox37 --- fixed
firefox-esr31 --- unaffected
b2g-v1.4 --- unaffected
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: daniele.di.proietto, Assigned: milan)

References

Details

(Keywords: sec-moderate, Whiteboard: [adv-main36+][b2g-adv-main2.2+])

Attachments

(2 files, 2 obsolete files)

Attached file test.html
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:34.0) Gecko/20100101 Firefox/34.0 Iceweasel/34.0
Build ID: 20141203163643

Steps to reproduce:

I opened the attached file with
- Firefox 34 on ubuntu with NVIDIA closed source drivers
- Iceweasel 31 on debian (jessie) with NVIDIA closed source drivers
- Icewease 34 on debian (experimental) with NVIDIA closed source drivers


Actual results:

In all three configurations a segmentation fault happened (consistently). I haven't had the chance to test it with non NVIDIA drivers or on windows. It may be related to NVIDIA drivers, but I suspect the bug to be in FF code.

This is a backtrace (iceweasel 31):

#0  __strcpy_ssse3 () at ../sysdeps/x86_64/multiarch/strcpy-ssse3.S:2289
#1  0x00007ffff3d0f87c in mozilla::WebGLContext::CompileShader (this=0x7fffc3a2e9c0, shader=0x7fffc3a2e940)
    at /tmp/buildd/iceweasel-31.3.0esr/content/canvas/src/WebGLContextGL.cpp:3135
#2  0x00007ffff3916f33 in mozilla::dom::WebGLRenderingContextBinding::compileShader (cx=0x7fffd2d08a80, obj=..., self=0x7fffc34a7c00, args=...)
    at /tmp/buildd/iceweasel-31.3.0esr/build-browser/dom/bindings/WebGLRenderingContextBinding.cpp:8225
#3  0x00007ffff395beea in mozilla::dom::GenericBindingMethod (cx=0x7fffd2d08a80, argc=<optimized out>, vp=<optimized out>)
    at /tmp/buildd/iceweasel-31.3.0esr/dom/bindings/BindingUtils.cpp:2297
#4  0x00007ffff4959a01 in CallJSNative (args=..., native=<optimized out>, cx=<optimized out>)
    at /tmp/buildd/iceweasel-31.3.0esr/js/src/jscntxtinlines.h:239
#5  js::Invoke (cx=0x7fffd2d08a80, args=..., construct=(js::CONSTRUCT | unknown: 4154173016))
    at /tmp/buildd/iceweasel-31.3.0esr/js/src/vm/Interpreter.cpp:475
#6  0x00007ffff49501d2 in Interpret (cx=0x7fffd2d08a80, state=...) at /tmp/buildd/iceweasel-31.3.0esr/js/src/vm/Interpreter.cpp:2620
#7  0x00007ffff4959669 in js::RunScript (cx=0x7fffd2d08a80, state=...) at /tmp/buildd/iceweasel-31.3.0esr/js/src/vm/Interpreter.cpp:422
#8  0x00007ffff49598d9 in js::Invoke (cx=0x7fffd2d08a80, args=..., construct=(js::CONSTRUCT | unknown: 4154173016), construct@entry=js::NO_CONSTRUCT)
    at /tmp/buildd/iceweasel-31.3.0esr/js/src/vm/Interpreter.cpp:494

It appears that

compiler->getInfoSink().info.size() is 0 in ShGetInfo()
lenWithNull is 1 in WebGLContext::CompileShader(), therefore len is 0 and the new memory is not allocated.

I'm not sure this is actually a security issue, but maybe there could be a way to control the pointers involved in the strcpy() and do nasty things.
Component: Untriaged → Canvas: WebGL
Product: Firefox → Core
Dan could reproduce this on OSX, so maybe it is less likely it is a driver issue.

Milan, is there somebody who can look at this?  Thanks.
Flags: needinfo?(milan)
OS: Linux → All
Hardware: x86_64 → All
Yes, I can see this on OS X as well, nightly.  Let me take a look.
Assignee: nobody → milan
Blocks: 1010371
Flags: needinfo?(milan)
Comment on attachment 8542995 [details] [diff] [review]
Checking for empty log needs to cover length 1, as we count the trailing null.

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

::: dom/canvas/WebGLContextGL.cpp
@@ +3371,5 @@
>          ShGetInfo(compiler, SH_INFO_LOG_LENGTH, &lenWithNull);
>  
> +        if (lenWithNull < 2) {
> +            // Error in ShGetInfo.  Since we're counting trailing null,
> +            // anything shorter than 2 means no actual length.

I find this somewhat unsatisfying. It seems like lenWithNull could legitimately be 1, if the InfoLog was the empty string.

I might actually prefer:

if (len) {
  // when strings are 0 length they use a common buffer that we can't write to.
  ShGetInfoLog(compiler, info.BeginWriting());
}

That being said, maybe you can convince me that your patch is better.
Attachment #8542995 - Flags: review?(jmuizelaar) → review-
Comment on attachment 8544181 [details] [diff] [review]
Skip the copying of empty strings. r=jrmuizelaar

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

::: dom/canvas/WebGLContextGL.cpp
@@ +3374,5 @@
>          } else {
>              size_t len = lenWithNull - 1;
>  
>              nsAutoCString info;
> +            if (len) {

Might as well add a comment like: "zero length strings can not be written to"
Attachment #8544181 - Flags: review?(jmuizelaar) → review+
[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Don't think it's easy - pretty sure this will always crash.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

As this is a security patch, didn't want to make the reason for not doing write to zero length string too obvious.

Which older supported branches are affected by this flaw?
34, 35, 36, ESR31 is OK.

If not all supported branches, which bug introduced the flaw?
1010371

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Same patch should apply to aurora and beta.

How likely is this patch to cause regressions; how much testing does it need?
This will just stop a crash, as we try to write to const+static location. Very low chance of regression.
Attachment #8544181 - Attachment is obsolete: true
Attachment #8544667 - Flags: sec-approval?
Attachment #8544667 - Flags: review+
I'm rating this as a sec-moderate since the crash doesn't seem to be exploitable. If this is a correct rating, then you don't need sec-approval to checkin.
Keywords: sec-moderate
Attachment #8544667 - Flags: sec-approval?
Attachment #8542995 - Attachment is obsolete: true
I did a quick test on firefox 31 (Linux 64-bit official build) and it seems to be affected too
https://hg.mozilla.org/mozilla-central/rev/bb194bedf519
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Please request Aurora36 and b2g34 approval on this when you get a chance.
Flags: needinfo?(milan)
Comment on attachment 8544667 [details] [diff] [review]
Skip the copying of empty strings. Carry r=jrmuizelaar

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 1110488 (or perhaps earlier)
User impact if declined: Crash when there are errors in WebGL being run
Testing completed: 
Risk to taking this patch (and alternatives if risky): Low
String or UUID changes made by this patch:

Not asking for ESR31 approval as this is sec-moderate.
Flags: needinfo?(milan)
Attachment #8544667 - Flags: approval-mozilla-b2g34?
Attachment #8544667 - Flags: approval-mozilla-aurora?
Attachment #8544667 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+
Attachment #8544667 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [adv-main36+]
Alias: CVE-2015-0830
Whiteboard: [adv-main36+] → [adv-main36+][b2g-adv-main2.2+]
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.