Initialize ExceptionCode in Element::removeAttribute

[Adam Klein]

Initialize ignored ExceptionCode in DatasetDOMStringMap::deleteItem

[Adam Klein]

Created attachment 106821 [details]
Patch

[Alexey Proskuryakov]

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...

[Adam Klein]

(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.

[Adam Klein]

Created attachment 106887 [details]
Patch

[WebKit Review Bot]

Comment on attachment 106887 [details]
Patch

Clearing flags on attachment: 106887

Committed r94869: <http://trac.webkit.org/changeset/94869>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

What's the security aspect here?

[Abhishek Arya]

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.

[Abhishek Arya]

(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.

[Alexey Proskuryakov]

> 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.

[Abhishek Arya]

(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 ()

[Alexey Proskuryakov]

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.

[Abhishek Arya]

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);
}