Bug 127988 – Add security-checked casts for all WebCore::CachedResource subclasses

• Component: Page Loading
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

Add security-checked casts for all WebCore::CachedResource subclasses.

This is a follow-up to Bug 127967.

[David Kilzer (:ddkilzer)]

Created attachment 222807 [details]
Patch v1

[Alexey Proskuryakov]

Comment on attachment 222807 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=222807&action=review

> Source/WebCore/loader/cache/CachedResource.h:159
> +    bool isRawResource() const { return type() == MainResource || type() == RawResource; }

This looks ultra confusing.

[David Kilzer (:ddkilzer)]

(In reply to comment #2)
> (From update of attachment 222807 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=222807&action=review
> 
> > Source/WebCore/loader/cache/CachedResource.h:159
> > +    bool isRawResource() const { return type() == MainResource || type() == RawResource; }
> 
> This looks ultra confusing.

Are you suggesting that a new method name is needed, or something else?

"This looks ultra confusing" is not an actionable comment.

[David Kilzer (:ddkilzer)]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 222807 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=222807&action=review
> > 
> > > Source/WebCore/loader/cache/CachedResource.h:159
> > > +    bool isRawResource() const { return type() == MainResource || type() == RawResource; }
> > 
> > This looks ultra confusing.
> 
> Are you suggesting that a new method name is needed, or something else?
> 
> "This looks ultra confusing" is not an actionable comment.

BTW, this is how the class is currently used.  I can get rid of isRawResource() (or a differently named method) and just use raw type checks everywhere, but I thought it was better to document the behavior more explicitly in code.

[Darin Adler]

Comment on attachment 222807 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=222807&action=review

>>> Source/WebCore/loader/cache/CachedResource.h:159
>>> +    bool isRawResource() const { return type() == MainResource || type() == RawResource; }
>> 
>> This looks ultra confusing.
> 
> Are you suggesting that a new method name is needed, or something else?
> 
> "This looks ultra confusing" is not an actionable comment.

I think the way to fix this is to put it in its own paragraph with a comment explaining that CachedMainResource is derived from CachedRawResource.

[Darin Adler]

Comment on attachment 222807 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=222807&action=review

> Source/WebCore/loader/cache/CachedRawResource.cpp:42
> +    ASSERT_WITH_SECURITY_IMPLICATION(isRawResource());

I don’t think this assertion is valuable.

We would have a potential security problem if something that is *not* a CachedRawResource returned *true* when asked isRawResource. But this assertion doesn’t check that.

If we have something that *is* a CachedRawResource, but returns *false* from isRawResource, the primary effect is that we will get an assertion when we call toCachedRawResource on it. This additional assertion doesn’t seem to add much, just gives an assertion earlier that we will hit later, and further I don’t think it has a security implication. Making this mistake will cause us to label a safe cast as unsafe, a false positive which is not itself evidence of a security problem.

[Alexey Proskuryakov]

> Are you suggesting that a new method name is needed, or something else?

I don't have a suggestion. I still think that this function is harmful - with or without a comment - because one can't expect a function named isRawResource() to return true for anything but RawResources.

[Darin Adler]

(In reply to comment #7)
> > Are you suggesting that a new method name is needed, or something else?
> 
> I don't have a suggestion. I still think that this function is harmful - with or without a comment - because one can't expect a function named isRawResource() to return true for anything but RawResources.

Since CachedMainResource is derived from CachedRawResource, in what sense a CachedMainResource "not a CachedRawResource"?

[David Kilzer (:ddkilzer)]

(In reply to comment #6)
> (From update of attachment 222807 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=222807&action=review
> 
> > Source/WebCore/loader/cache/CachedRawResource.cpp:42
> > +    ASSERT_WITH_SECURITY_IMPLICATION(isRawResource());
> 
> I don’t think this assertion is valuable.
> 
> We would have a potential security problem if something that is *not* a CachedRawResource returned *true* when asked isRawResource. But this assertion doesn’t check that.
> 
> If we have something that *is* a CachedRawResource, but returns *false* from isRawResource, the primary effect is that we will get an assertion when we call toCachedRawResource on it. This additional assertion doesn’t seem to add much, just gives an assertion earlier that we will hit later, and further I don’t think it has a security implication. Making this mistake will cause us to label a safe cast as unsafe, a false positive which is not itself evidence of a security problem.

Okay, I agree with your statement that this isn't a security concern.  However, I think it's best to catch issues as early as possible, so a plain assertion seems appropriate for developers building Debug builds:

+    ASSERT(isRawResource());

[Alexey Proskuryakov]

(In reply to comment #8)
> Since CachedMainResource is derived from CachedRawResource, in what sense a CachedMainResource "not a CachedRawResource"?

I don't think that this statement is accurate. We don't have a CachedMainResource class in WebKit, and in fact, CachedRawResource is a leaf "final" class.

The root of the confusion is of course that type() is almost the kind of poor man's RTTI that we use all over the place in WebKit, but not quite. The right solution is to split poor man's RTTI and how we determine whether a cached raw resource is main resource or XHR. I think that adding gloss over the current situation in the form of isRawResource() function makes it more confusing, not less.

[David Kilzer (:ddkilzer)]

(In reply to comment #9)
> (In reply to comment #6)
> > (From update of attachment 222807 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=222807&action=review
> > 
> > > Source/WebCore/loader/cache/CachedRawResource.cpp:42
> > > +    ASSERT_WITH_SECURITY_IMPLICATION(isRawResource());
> > 
> > I don’t think this assertion is valuable.
> > 
> > We would have a potential security problem if something that is *not* a CachedRawResource returned *true* when asked isRawResource. But this assertion doesn’t check that.
> > 
> > If we have something that *is* a CachedRawResource, but returns *false* from isRawResource, the primary effect is that we will get an assertion when we call toCachedRawResource on it. This additional assertion doesn’t seem to add much, just gives an assertion earlier that we will hit later, and further I don’t think it has a security implication. Making this mistake will cause us to label a safe cast as unsafe, a false positive which is not itself evidence of a security problem.
> 
> Okay, I agree with your statement that this isn't a security concern.  However, I think it's best to catch issues as early as possible, so a plain assertion seems appropriate for developers building Debug builds:
> 
> +    ASSERT(isRawResource());

I take that back.  +[WebCache imageForURL:] pulls a CachedResource from the cache and checks isImage().  If we created a CachedRawResource with a type of CachedResource::ImageResource by accident, we're going to cast a CachedRawResource* to a CachedImage*, then call CachedImage::hasImage() on the CachedRawResource object.  Boom.

To be fair, incorrect code would have to be written before that would happen, but the idea of the assert is that it would catch the bad code in a debug build, and flag it properly as a potential security issue.

[Darin Adler]

(In reply to comment #11)
> I take that back.  +[WebCache imageForURL:] pulls a CachedResource from the cache and checks isImage().

Then the assertion we need is ASSERT(!isImage());

> To be fair, incorrect code would have to be written before that would happen, but the idea of the assert is that it would catch the bad code in a debug build, and flag it properly as a potential security issue.

But not if we assert the wrong thing.

[David Kilzer (:ddkilzer)]

(In reply to comment #12)
> (In reply to comment #11)
> > I take that back.  +[WebCache imageForURL:] pulls a CachedResource from the cache and checks isImage().
> 
> Then the assertion we need is ASSERT(!isImage());

Note that InspectorPageAgent::cachedResourceContent() also does additional checking of the CachedResource::Type and may cast to CachedCSSStyleSheet and CachedScript, so it's more than just CachedImage that could be cast incorrectly.

> > To be fair, incorrect code would have to be written before that would happen, but the idea of the assert is that it would catch the bad code in a debug build, and flag it properly as a potential security issue.
> 
> But not if we assert the wrong thing.

Stepping back, the root of the issue is that the constructor for CachedRawResource takes a CachedResource::Type enum instead of automatically using one for the developer based on the class itself because CachedRawResource is the only class that can represent more than one CachedResource::Type.

Thus, a coding mistake would be easy to make in the future by passing in the wrong type.  My original defense was to assert that the CachedResource::Type passed in was an acceptable value (MainResource or RawResource).

It didn't make sense to me to check for all the wrong types individually, hence my original code checking for the two acceptable types.  Alexey suggested I change isRawResource() to be more descriptive, so it would now be:

    ASSERT(isMainOrRawResource());

Another way to prevent developers from doing the wrong thing would be to stop letting the developer specify the CachedResource::Type (either through yet another enum, or by subclassing CachedRawResource to CachedMainResource, or by some other mechanism).  I'll add a FIXME by the assertion when landing.

[David Kilzer (:ddkilzer)]

Committed r163242: <http://trac.webkit.org/changeset/163242>