Closed
Bug 775234
Opened 12 years ago
Closed 12 years ago
out of bounds read when compiling webgl vertex shader with long identifier name
Categories
(Core :: Graphics: CanvasWebGL, defect)
Tracking
()
RESOLVED
FIXED
mozilla17
People
(Reporter: miaubiz, Assigned: bjacob)
Details
(Keywords: csectype-dos, sec-other, Whiteboard: [asan][adv-track-main17+])
Attachments
(7 files, 2 obsolete files)
|
599 bytes,
text/plain
|
Details | |
|
598 bytes,
text/plain
|
Details | |
|
695 bytes,
text/plain
|
Details | |
|
949 bytes,
text/plain
|
Details | |
|
7.77 KB,
text/plain
|
Details | |
|
8.68 KB,
text/plain
|
Details | |
|
1.54 KB,
patch
|
jgilbert
:
review+
akeybl
:
approval-mozilla-aurora-
akeybl
:
approval-mozilla-beta-
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_4) AppleWebKit/536.11 (KHTML, like Gecko) Chrome/20.0.1132.57 Safari/536.11
Steps to reproduce:
tried to compile a vertex shader with a long identifier name:
<html>
<head>
<script id="x" type="x-shader/x-vertex">
struct Nesting2 {
vec2 #x1#;
};
struct Nesting1 {
Nesting2 #x1#;
};
uniform Nesting1 #x2#;
void main() {
#x2#.#x1#;
}
</script>
<script>
var gl = document.createElement("canvas").getContext("moz-webgl")
var shader = gl.createShader(gl.VERTEX_SHADER)
var bad = x.text.replace(/#x1#/g, Array(128).join("A")).replace(/#x2#/g, Array(257).join("A"))
gl.shaderSource(shader, bad)
gl.compileShader(shader)
</script>
</head>
</html>
Actual results:
asanified firefox crashed
==3265== ERROR: AddressSanitizer heap-buffer-overflow on address 0x0001299bd87f at pc 0x1051bbac4 bp 0x7fff5fbf5cd0 sp 0x7fff5fbf5cc8
READ of size 1 at 0x0001299bd87f thread T0
#0 0x1051bbac4 in mozilla::WebGLContext::CompileShader(mozilla::WebGLShader*) (in XUL) + 5316
#1 0x106ba7b8b in _ZN7mozilla3dom28WebGLRenderingContextBindingL13compileShaderEP9JSContextjPN2JS5ValueE (in XUL) + 715
#2 0x1078c7a99 in js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs
0x0001299bd87f is located 1 bytes to the left of 265-byte region [0x0001299bd880,0x0001299bd989)
allocated by thread T0 here:
#0 0x10000ce6e in (anonymous namespace)::mz_malloc(_malloc_zone_t*, unsigned long) (in firefox) + 46
#1 0x7fff92d583c8 in __strtodg$UNIX2003 (in libsystem_c.dylib) + 945
#2 0x7fff92d591a4 in __strtodg$UNIX2003 (in libsystem_c.dylib) + 4493
#3 0x106cf2b95 in nsStringBuffer::Alloc(unsigned long) (in XUL) + 117
Expected results:
it shouldn't have.
|
||
Comment 6•12 years ago
|
||
This sounds a lot like one we fixed a long while ago by limiting identifier length. Is it back? Merely similar sounding? Fixed in [ANGLE] in a windows-only part of the code? You've got Mac and Linux logs here. Are you implying Windows doesn't have the problem or is it simply not tested by you?
Component: Untriaged → Canvas: WebGL
Product: Firefox → Core
|
|
||
Updated•12 years ago
|
Whiteboard: [asan]
|
Assignee | |
Comment 7•12 years ago
|
||
What Firefox version / mozilla-central revision is this with?
|
|
Assignee | |
Comment 8•12 years ago
|
||
With Valgrind, the first testcase does reproduce an invalid read of size 1 with current mozilla-central on linux 64bit: ==2639== Invalid read of size 1 ==2639== at 0x6C26557: mozilla::WebGLContext::CompileShader(mozilla::WebGLShader*) (WebGLContextGL.cpp:4892) ==2639== by 0x761B081: mozilla::dom::WebGLRenderingContextBinding::compileShader(JSContext*, unsigned int, JS::Value*) (WebGLRenderingContextBinding.cpp:1208) ==2639== by 0x7A323C1: js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) [clone .constprop.374] (jscntxtinlines.h:385) ==2639== by 0x7A46D30: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (jsinterp.cpp:344) ==2639== by 0x7A33418: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2442) ==2639== by 0x7A4680A: js::RunScript(JSContext*, JSScript*, js::StackFrame*) (jsinterp.cpp:301) ==2639== by 0x7A481D5: js::Execute(JSContext*, JSScript*, JSObject&, JS::Value*) (jsinterp.cpp:487) ==2639== by 0x7967F2D: EvaluateUCScriptForPrincipalsCommon(JSContext*, JSObject*, JSPrincipals*, JSPrincipals*, unsigned short const*, unsigned int, char const*, unsigned int, JS::Value*, JSVersion) (jsapi.cpp:5373) ==2639== by 0x79680C9: JS_EvaluateUCScriptForPrincipalsVersionOrigin (jsapi.cpp:5410) ==2639== by 0x6D9C044: nsJSContext::EvaluateString(nsAString_internal const&, JSObject*, nsIPrincipal*, nsIPrincipal*, char const*, unsigned int, JSVersion, nsAString_internal*, bool*) (nsJSEnvironment.cpp:1466) ==2639== by 0x6BB3CA2: nsScriptLoader::EvaluateScript(nsScriptLoadRequest*, nsString const&) (nsScriptLoader.cpp:874) ==2639== by 0x6BB4124: nsScriptLoader::ProcessRequest(nsScriptLoadRequest*) (nsScriptLoader.cpp:767) ==2639== Address 0x5d6026f is not stack'd, malloc'd or (recently) free'd ==2639==
|
|
Assignee | |
Updated•12 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
|
|
Assignee | |
Comment 9•12 years ago
|
||
Today's lesson: all variables named 'length' are not necessarily mutually interchangeable. This fixes the Valgrind error for me. It's also consistent with the fact that all the invalid reads we got were of size 1: we were dereferencing a char* pointer with a wrong offset. Security consequences of this bug: - direct attack: an attacker can use this to test whether bytes are equal to ']'. However, it can only prescribe a not-too-large offset (less than maximum uniform array size) and only from a base pointer that the attacker can't control and will be different on every try (the mapped_name pointer, which is malloc'd just here). So this basically boils down to "test whether random bytes are equal to ']'". This could be used to test the proportion of bytes equal to ']' in memory, I guess. Theoretically, an attacker could couple this with operations that cause this to be related to user-sensitive content, to discover clues about that content. For example, an attacker could draw a cross-origin image, applying to it some sort of filter (using canvas 2d or svg filter, I don't know) that would result in having significant info about the image encoded in the proportion of bytes equal to ']'... but that seems extremely convoluted and would at best take a very long time to exploit. It seems that regular timing attacks on drawing, that are here anyway, are more convenient to conduct. - indirect attack: an attacker could also use this bug to confuse us as to whether a uniform is an array or not, but we still get the correct array length, so even if we get confused into believing that a non-array is an array, we still consider it an array of length 1, which for memory-access-validation purposes doesn't make a difference. So I don't see any way to exploit this for an attacker.
Attachment #643600 -
Flags: review?
|
|
Assignee | |
Comment 10•12 years ago
|
||
Erm, this instead. Earlier I wrongly though that |length| was the uniform array length; it actually is a string length, just not the right string.
Attachment #643600 -
Attachment is obsolete: true
Attachment #643600 -
Flags: review?
Attachment #643601 -
Flags: review?
|
|
Assignee | |
Comment 11•12 years ago
|
||
Attachment #643601 -
Attachment is obsolete: true
Attachment #643601 -
Flags: review?
Attachment #643602 -
Flags: review?
|
|
Assignee | |
Updated•12 years ago
|
Attachment #643602 -
Flags: review? → review?(jgilbert)
|
Reporter | |
Comment 12•12 years ago
|
||
(In reply to Benoit Jacob [:bjacob] from comment #7) > What Firefox version / mozilla-central revision is this with? I am using the builds from: https://people.mozilla.com/~choller/firefox/asan/
|
|
Reporter | |
Comment 13•12 years ago
|
||
(In reply to Daniel Veditz [:dveditz] from comment #6) > You've got Mac and Linux logs here. Are you implying Windows doesn't have > the problem or is it simply not tested by you? I was only able to reproduce under ASAN and it's not (yet) available for windows.
|
|
Reporter | |
Comment 14•12 years ago
|
||
I didn't try/get it working, but here two structs are nested with 'uniform'. would it be possible to nest these deeper to be able to access stuff further away or in a more interesting place?
|
||
Updated•12 years ago
|
Attachment #643602 -
Flags: review?(jgilbert) → review+
|
|
Assignee | |
Comment 15•12 years ago
|
||
(In reply to miaubiz from comment #14) > I didn't try/get it working, but here two structs are nested with 'uniform'. > would it be possible to nest these deeper to be able to access stuff further > away or in a more interesting place? Getting bigger offsets is a bit irrelevent given that the base pointer is different (malloc'd) on every try. This is really more or less random access, if we consider that pointers returned by malloc are random.
|
||
Comment 16•12 years ago
|
||
Provisionally marking sec-low based on comment 9. Please ping/modify if we should become more worried.
Keywords: sec-low
|
|
Assignee | |
Comment 17•12 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/0cb8d17a5b46
Assignee: nobody → bjacob
Target Milestone: --- → mozilla17
|
|
Assignee | |
Comment 18•12 years ago
|
||
Comment on attachment 643602 [details] [diff] [review] we were using an unrelated 'length' variable to get the length of the mapped uniform name [Approval Request Comment] Bug caused by (feature/regressing bug #): since we turned on long identifier mapping around firefox 13 User impact if declined: information-disclosure vuln, but seems minor because it only allows to test if random addresses contain the byte ']'. Testing completed (on m-c, etc.): m-i Risk to taking this patch (and alternatives if risky): not completely trivial, but this only changes the notion of string length that we are using here, and the current situation was to use an arbitrary bad length, so it can't get worse than that. String or UUID changes made by this patch: none
Attachment #643602 -
Flags: approval-mozilla-beta?
Attachment #643602 -
Flags: approval-mozilla-aurora?
|
||
Comment 19•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0cb8d17a5b46
Status: NEW → RESOLVED
Closed: 12 years ago
status-firefox15:
--- → affected
status-firefox16:
--- → affected
status-firefox17:
--- → fixed
Resolution: --- → FIXED
|
||
Comment 20•12 years ago
|
||
Comment on attachment 643602 [details] [diff] [review] we were using an unrelated 'length' variable to get the length of the mapped uniform name [Triage Comment] Given this is sg:low, I don't think we need to introduce any new risk here.
Attachment #643602 -
Flags: approval-mozilla-beta?
Attachment #643602 -
Flags: approval-mozilla-beta-
Attachment #643602 -
Flags: approval-mozilla-aurora?
Attachment #643602 -
Flags: approval-mozilla-aurora-
|
|
||
Updated•12 years ago
|
|
||
Updated•12 years ago
|
Whiteboard: [asan] → [asan][adv-track-main17+]
|
|
||
Updated•12 years ago
|
Alias: CVE-2012-5831
|
|
||
Updated•12 years ago
|
|
|
||
Updated•11 years ago
|
Group: core-security
Flags: sec-bounty-
You need to log in
before you can comment on or make changes to this bug.
Description
•