Bug 67820 - Initialize ExceptionCode in Element::removeAttribute
Summary: Initialize ExceptionCode in Element::removeAttribute
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-08 17:19 PDT by Adam Klein
Modified: 2011-09-13 00:56 PDT (History)
6 users (show)

See Also:


Attachments
Patch (1.48 KB, patch)
2011-09-08 17:24 PDT, Adam Klein
no flags Details | Formatted Diff | Diff
Patch (1.45 KB, patch)
2011-09-09 10:39 PDT, Adam Klein
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Klein 2011-09-08 17:19:42 PDT
Initialize ignored ExceptionCode in DatasetDOMStringMap::deleteItem
Comment 1 Adam Klein 2011-09-08 17:24:14 PDT
Created attachment 106821 [details]
Patch
Comment 2 Alexey Proskuryakov 2011-09-08 22:26:45 PDT
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...
Comment 3 Adam Klein 2011-09-09 10:38:29 PDT
(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.
Comment 4 Adam Klein 2011-09-09 10:39:18 PDT
Created attachment 106887 [details]
Patch
Comment 5 WebKit Review Bot 2011-09-09 11:36:12 PDT
Comment on attachment 106887 [details]
Patch

Clearing flags on attachment: 106887

Committed r94869: <http://trac.webkit.org/changeset/94869>
Comment 6 WebKit Review Bot 2011-09-09 11:36:16 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Alexey Proskuryakov 2011-09-12 21:46:11 PDT
What's the security aspect here?
Comment 8 Abhishek Arya 2011-09-12 21:55:52 PDT
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.
Comment 9 Abhishek Arya 2011-09-12 22:11:43 PDT
(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.
Comment 10 Alexey Proskuryakov 2011-09-12 22:47:36 PDT
> 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.
Comment 11 Abhishek Arya 2011-09-12 23:43:48 PDT
(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 ()
Comment 12 Alexey Proskuryakov 2011-09-13 00:01:12 PDT
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.
Comment 13 Abhishek Arya 2011-09-13 00:56:17 PDT
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);
}