67820 2011-09-08 17:19:42 -0700 Initialize ExceptionCode in Element::removeAttribute 2011-09-13 00:56:17 -0700 1 1 1 Unclassified WebKit DOM 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 adamk webkit-unassigned ap darin dglazkov eae inferno webkit.review.bot oldest_to_newest 464519 0 adamk 2011-09-08 17:19:42 -0700 Initialize ignored ExceptionCode in DatasetDOMStringMap::deleteItem 464520 1 106821 adamk 2011-09-08 17:24:14 -0700 Created attachment 106821 Patch 464648 2 ap 2011-09-08 22:26:45 -0700 This fix doesn't look right. It's the job of code that cares about exception code to initialize it - and if valgrind reports a conditional jump based on uninitialized variable, than this other code failed to do it. Element::removeAttribute() should set ec to 0 before calling removeNamedItem() and checking whether that changed ec. CC'ing Darin just in case I got it wrong again... 464887 3 adamk 2011-09-09 10:38:29 -0700 (In reply to comment #2) > This fix doesn't look right. It's the job of code that cares about exception code to initialize it - and if valgrind reports a conditional jump based on uninitialized variable, than this other code failed to do it. > > Element::removeAttribute() should set ec to 0 before calling removeNamedItem() and checking whether that changed ec. > > CC'ing Darin just in case I got it wrong again... Good call, and in fact that's what the other overload of removeAttribute() does. I've updated the code, the ChangeLog, and the bug title. 464888 4 106887 adamk 2011-09-09 10:39:18 -0700 Created attachment 106887 Patch 464944 5 106887 webkit.review.bot 2011-09-09 11:36:12 -0700 Comment on attachment 106887 Patch Clearing flags on attachment: 106887 Committed r94869: <http://trac.webkit.org/changeset/94869> 464945 6 webkit.review.bot 2011-09-09 11:36:16 -0700 All reviewed patches have been landed. Closing bug. 466124 7 ap 2011-09-12 21:46:11 -0700 What's the security aspect here? 466129 8 inferno 2011-09-12 21:55:52 -0700 Uninitialized reads are equivalent to use after frees bugs. If you don't initialize the value, it might have a initial attacker controlled value in there which can be used for exploitation. I havent got a chance to look into the possibility of exploitation given that ec is only used for exception code. 466133 9 inferno 2011-09-12 22:11:43 -0700 (In reply to comment #8) > Uninitialized reads are equivalent to use after frees bugs. If you don't initialize the value, it might have a initial attacker controlled value in there which can be used for exploitation. I havent got a chance to look into the possibility of exploitation given that ec is only used for exception code. I think this can be used for uninitialized read, since if the ec is not set as NOT_FOUND_ERR, then ec will have default uninitialized value which can be probably read in javascript with try, catch blocks and reading exception.code value. If you think this is not possible at all, please let me know and i will remove the security tags. 466142 10 ap 2011-09-12 22:47:36 -0700 > ec is only used for exception code Right, there are few arbitrary code execution opportunities even if there was a code path where uninitialized ec was used in a substantial way. This code path certainly doesn't count as substantial: if (ec == NOT_FOUND_ERR) ec = 0; > can be probably read in javascript with try, catch blocks I don't know if exposing a single uninitialized local variable to a script can actually be used for anything malicious, but this cannot happen here anyway. All functions that can raise a JavaScript exception initialize ec first thing in generated bindings code. 466164 11 inferno 2011-09-12 23:43:48 -0700 (In reply to comment #10) > > ec is only used for exception code > > Right, there are few arbitrary code execution opportunities even if there was a code path where uninitialized ec was used in a substantial way. This code path certainly doesn't count as substantial: > > if (ec == NOT_FOUND_ERR) > ec = 0; > I am not worried about this path since this one just does == check and changes ec value. I don't see how code execution can happen here. What i do see as attacker wanting to get the value of uninitialized ec through javascript. this uninitialized value might be holding some important info that the attacker didnt had access to, and now he or she does. e.g. cross-origin info. > > can be probably read in javascript with try, catch blocks > > I don't know if exposing a single uninitialized local variable to a script can actually be used for anything malicious, but this cannot happen here anyway. All functions that can raise a JavaScript exception initialize ec first thing in generated bindings code. I agree that most paths are sanitized here since generated bindings code will initialize ec. In this case, we atleast know this one path through v8 bindings, where ec was uninitialized, and which caused the valgrind report in http://code.google.com/p/chromium/issues/detail?id=76490. If you don't mind, we would like to retain security tags on this. UninitCondition Conditional jump or move depends on uninitialised value(s) WebCore::Element::removeAttribute(WTF::String const&, int&) (third_party/WebKit/Source/WebCore/dom/Element.cpp:1448) WebCore::DatasetDOMStringMap::deleteItem(WTF::String const&, int&) (third_party/WebKit/Source/WebCore/dom/DatasetDOMStringMap.cpp:189) WebCore::V8DOMStringMap::namedPropertyDeleter(v8::Local<v8::String>, v8::AccessorInfo const&) (third_party/WebKit/Source/WebCore/bindings/v8/custom/V8DOMStringMapCustom.cpp:76) v8::internal::JSObject::DeletePropertyWithInterceptor(v8::internal::String*) (v8/src/objects.cc:2525) 0xBED0CE43 () 466168 12 ap 2011-09-13 00:01:12 -0700 Please have a look at DatasetDOMStringMap::deleteItem(). It's no surprise that ec is not initialized. I do not care personally, but this is pretty clearly not a security bug. 466177 13 inferno 2011-09-13 00:56:17 -0700 After debugging, i understand that my understanding was wrong here. I was confusing that this uninitialized dummy variable (with ec) was retrievable via javascript, but it is not. Only ec is retrievable, which will get set of zero if this uninitialized dummy == NOT_FOUND_ERR. ec=0 is a common case, so this is not useful value. Removing security tags and sorry for the noise. void DatasetDOMStringMap::deleteItem(const String& name, ExceptionCode& ec) { if (!isValidPropertyName(name)) { ec = SYNTAX_ERR; return; } ExceptionCode dummy; m_element->removeAttribute(convertPropertyNameToAttributeName(name), dummy); } 106821 2011-09-08 17:24:14 -0700 2011-09-09 10:39:15 -0700 Patch bug-67820-20110908172413.patch text/plain 1518 adamk U3VidmVyc2lvbiBSZXZpc2lvbjogOTQ3NDgKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCBlMTA1MmQwMjI3ZDE2YjU2 NTU5MmQ1ZDc1MzhiMjljNWNhYjM1MDlkLi4zMTk5ODkxYmE2MDgyNDliYjg0ZDFmMDQyNTY3OWU1 Yjk0ZmFhOGJhIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvU291 cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTggQEAKKzIwMTEtMDktMDggIEFkYW0g S2xlaW4gIDxhZGFta0BjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgSW5pdGlhbGl6ZSBpZ25vcmVk IEV4Y2VwdGlvbkNvZGUgaW4gRGF0YXNldERPTVN0cmluZ01hcDo6ZGVsZXRlSXRlbQorICAgICAg ICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9Njc4MjAKKworICAgICAg ICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBTaWxlbmNlcyB2YWxncmlu ZCB3YXJuaW5nIHJlcG9ydGVkIGluIGh0dHA6Ly9jcmJ1Zy5jb20vNzY0OTAuCisKKyAgICAgICAg Tm8gbmV3IHRlc3RzIHNpbmNlIHRoaXMgZG9lcyBub3QgYWZmZWN0IHRoZSBiZWhhdmlvciBvZiB0 aGUgY29kZQorICAgICAgICAoYmVjYXVzZSB0aGUgZXhpc3RpbmcgY29kZSB3YXMgYWxyZWFkeSBp Z25vcmluZyB0aGUgdmFsdWUgb2YgdGhlIGVjKS4KKworICAgICAgICAqIGRvbS9EYXRhc2V0RE9N U3RyaW5nTWFwLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OkRhdGFzZXRET01TdHJpbmdNYXA6OmRl bGV0ZUl0ZW0pOiBJbml0aWFsaXplIGR1bW15IEV4Y2VwdGlvbkNvZGUgdG8gMC4KKwogMjAxMS0w OS0wNyAgU2hlcmlmZiBCb3QgIDx3ZWJraXQucmV2aWV3LmJvdEBnbWFpbC5jb20+CiAKICAgICAg ICAgVW5yZXZpZXdlZCwgcm9sbGluZyBvdXQgcjk0Njc0IGFuZCByOTQ2ODkuCmRpZmYgLS1naXQg YS9Tb3VyY2UvV2ViQ29yZS9kb20vRGF0YXNldERPTVN0cmluZ01hcC5jcHAgYi9Tb3VyY2UvV2Vi Q29yZS9kb20vRGF0YXNldERPTVN0cmluZ01hcC5jcHAKaW5kZXggZTY4ZTlkOTU5MjFiMzg2NTA5 NjBhMDFhM2NkMTUyYjZlNGJlNzcxZi4uYzRmMGI3ZTRiN2MyYjE4ZDkwNTE2ZjZkNzA2NDc5ZmEw Y2FhOGQyNCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvZG9tL0RhdGFzZXRET01TdHJpbmdN YXAuY3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL2RvbS9EYXRhc2V0RE9NU3RyaW5nTWFwLmNwcApA QCAtMTk5LDcgKzE5OSw3IEBAIHZvaWQgRGF0YXNldERPTVN0cmluZ01hcDo6ZGVsZXRlSXRlbShj b25zdCBTdHJpbmcmIG5hbWUsIEV4Y2VwdGlvbkNvZGUmIGVjKQogICAgICAgICByZXR1cm47CiAg ICAgfQogCi0gICAgRXhjZXB0aW9uQ29kZSBkdW1teTsKKyAgICBFeGNlcHRpb25Db2RlIGR1bW15 ID0gMDsKICAgICBtX2VsZW1lbnQtPnJlbW92ZUF0dHJpYnV0ZShjb252ZXJ0UHJvcGVydHlOYW1l VG9BdHRyaWJ1dGVOYW1lKG5hbWUpLCBkdW1teSk7CiB9CiAK 106887 2011-09-09 10:39:18 -0700 2011-09-09 11:36:11 -0700 Patch bug-67820-20110909103917.patch text/plain 1480 adamk U3VidmVyc2lvbiBSZXZpc2lvbjogOTQ3NDgKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCBlMTA1MmQwMjI3ZDE2YjU2 NTU5MmQ1ZDc1MzhiMjljNWNhYjM1MDlkLi4xOGZjMWM1YWY0OWQ2OWQ0NmZiOTRiMTA0Yzc2MjNh ZGViYWVkYWU2IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvU291 cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTggQEAKKzIwMTEtMDktMDggIEFkYW0g S2xlaW4gIDxhZGFta0BjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgSW5pdGlhbGl6ZSBFeGNlcHRp b25Db2RlIGluIEVsZW1lbnQ6OnJlbW92ZUF0dHJpYnV0ZQorICAgICAgICBodHRwczovL2J1Z3Mu d2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9Njc4MjAKKworICAgICAgICBSZXZpZXdlZCBieSBO T0JPRFkgKE9PUFMhKS4KKworICAgICAgICBTaWxlbmNlcyB2YWxncmluZCB3YXJuaW5nIHJlcG9y dGVkIGluIGh0dHA6Ly9jcmJ1Zy5jb20vNzY0OTAuCisKKyAgICAgICAgTm8gbmV3IHRlc3RzIHNp bmNlIHRoaXMgd291bGQgb25seSB2ZXJ5IG9jY2FzaW9uYWxseSBiZSBmbGFreSwKKyAgICAgICAg YW5kIGluIHRoZSBjb2RlcGF0aCBpbiB0aGUgdmFsZ3JpbmQgcmVwb3J0LCB0aGUgZWMgaXMgaWdu b3JlZCBhbnl3YXkuCisKKyAgICAgICAgKiBkb20vRWxlbWVudC5jcHA6CisgICAgICAgIChXZWJD b3JlOjpFbGVtZW50OjpyZW1vdmVBdHRyaWJ1dGUpOiBJbml0aWFsaXplIGVjIHRvIDAuCisKIDIw MTEtMDktMDcgIFNoZXJpZmYgQm90ICA8d2Via2l0LnJldmlldy5ib3RAZ21haWwuY29tPgogCiAg ICAgICAgIFVucmV2aWV3ZWQsIHJvbGxpbmcgb3V0IHI5NDY3NCBhbmQgcjk0Njg5LgpkaWZmIC0t Z2l0IGEvU291cmNlL1dlYkNvcmUvZG9tL0VsZW1lbnQuY3BwIGIvU291cmNlL1dlYkNvcmUvZG9t L0VsZW1lbnQuY3BwCmluZGV4IDJmMmI2MjNiN2MyMTFiMmZjNGVmZmFhNTc3ZGJiNWU1MTJiOWVl ZWMuLmRlNGI4ZDY1OTIwZDBlMDc3MmUyYTBhOGIyMTUxMjdmNzk2YzdiNTcgMTAwNjQ0Ci0tLSBh L1NvdXJjZS9XZWJDb3JlL2RvbS9FbGVtZW50LmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9kb20v RWxlbWVudC5jcHAKQEAgLTE1MjAsNiArMTUyMCw3IEBAIHZvaWQgRWxlbWVudDo6cmVtb3ZlQXR0 cmlidXRlKGNvbnN0IFN0cmluZyYgbmFtZSwgRXhjZXB0aW9uQ29kZSYgZWMpCiAgICAgU3RyaW5n IGxvY2FsTmFtZSA9IHNob3VsZElnbm9yZUF0dHJpYnV0ZUNhc2UodGhpcykgPyBuYW1lLmxvd2Vy KCkgOiBuYW1lOwogCiAgICAgaWYgKG1fYXR0cmlidXRlTWFwKSB7CisgICAgICAgIGVjID0gMDsK ICAgICAgICAgbV9hdHRyaWJ1dGVNYXAtPnJlbW92ZU5hbWVkSXRlbShsb2NhbE5hbWUsIGVjKTsK ICAgICAgICAgaWYgKGVjID09IE5PVF9GT1VORF9FUlIpCiAgICAgICAgICAgICBlYyA9IDA7Cg==