Bug 26918 – XSSAuditor should prevent injection of HTML Base tag

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Daniel Bates]

We should prevent injections of <base href="...">, since this can be used to load external scripts from a malicious site.

[Adam Barth]

Good catch Dan.  We should probably handle this similarly to the <script> tag by checking the unparsed URL against the request.

[Daniel Bates]

Created attachment 32284 [details]
Patch with tests

Implements support for preventing injection of HTML Base tag.

[Adam Barth]

Comment on attachment 32284 [details]
Patch with tests

Generally looks good, but I have a couple nits and a couple questions:

> +        m_hrefRaw = attr->value();

I might call this something like hrefAttrValue.  "Raw" is kind of a vague term.

> +    KURL baseElementURL(m_frame->document()->url(), url);
> +    if (findInRequest(url) && m_frame->document()->url().baseAsString() != baseElementURL.baseAsString()) {

Seems like you should do the != comparison first because it will be faster than findInRequest.

> --- LayoutTests/http/tests/security/xssAuditor/base-href-safe2-expected.txt	(revision 0)
> +++ LayoutTests/http/tests/security/xssAuditor/base-href-safe2-expected.txt	(revision 0)
> @@ -0,0 +1,2 @@
> +ALERT: /XSS/

This looks like a failure.  Can we alert something friendlier here?

> +ALERT: This is a safe script.

Like this ^^^^  :)

> +<iframe src="http://localhost:8000/security/xssAuditor/resources/echo-head-base-href.pl?q=<base href='127.0.0.1:8000/security/xssAuditor/resources/base-href/'>">

I think you mean to have a // in the beginning of this href attribute.

> +<iframe src="http://localhost:8000/security/xssAuditor/resources/echo-head-base-href.pl?q=<base href='//127.0.0.1:8000/security/xssAuditor/resources/base-href/'>">

What's the difference between these to test cases?  It looks like the first isn't actually testing a scheme-relative path.  It's testing a relative path.

[Daniel Bates]

base-href-scheme-relative2.html was just testing that the XSSAuditor can match the URL even though it is missing the scheme and "://". It was not a really good test case, so I removed it.

Instead, I wrote a test case that has an embedded url-encoded null character in the base-href and discovered I was not catching this case. I modified the patch so that inside HTMLBaseElement::parseMappedAttribute, m_ hrefAttrValue = StringImpl::createStrippingNullCharacters(attr->value().characters(), attr->value().length()); that is, I remove any null-characters. 

(In reply to comment #3)
> (From update of attachment 32284 [details])
> Generally looks good, but I have a couple nits and a couple questions:
> 
> > +        m_hrefRaw = attr->value();
> 
> I might call this something like hrefAttrValue.  "Raw" is kind of a vague term.
> 
> > +    KURL baseElementURL(m_frame->document()->url(), url);
> > +    if (findInRequest(url) && m_frame->document()->url().baseAsString() != baseElementURL.baseAsString()) {
> 
> Seems like you should do the != comparison first because it will be faster than
> findInRequest.
> 
> > --- LayoutTests/http/tests/security/xssAuditor/base-href-safe2-expected.txt	(revision 0)
> > +++ LayoutTests/http/tests/security/xssAuditor/base-href-safe2-expected.txt	(revision 0)
> > @@ -0,0 +1,2 @@
> > +ALERT: /XSS/
> 
> This looks like a failure.  Can we alert something friendlier here?
> 
> > +ALERT: This is a safe script.
> 
> Like this ^^^^  :)
> 
> > +<iframe src="http://localhost:8000/security/xssAuditor/resources/echo-head-base-href.pl?q=<base href='127.0.0.1:8000/security/xssAuditor/resources/base-href/'>">
> 
> I think you mean to have a // in the beginning of this href attribute.
> 
> > +<iframe src="http://localhost:8000/security/xssAuditor/resources/echo-head-base-href.pl?q=<base href='//127.0.0.1:8000/security/xssAuditor/resources/base-href/'>">
> 
> What's the difference between these to test cases?  It looks like the first
> isn't actually testing a scheme-relative path.  It's testing a relative path.

[Daniel Bates]

Created attachment 32348 [details]
Patch with tests

Cleaned up patch.

Removed test base-href-scheme-relative2.html. Added test base-href-with-null-char.html.

[Adam Barth]

Comment on attachment 32348 [details]
Patch with tests

This looks pretty good.  Only one detail to change. 

> +        m_hrefAttrValue = StringImpl::createStrippingNullCharacters(attr->value().characters(), attr->value().length());

It doesn't seem right to do the null stripping here.  m_hrefAttrValue should hold the value of the href attr, nulls and all.

> +bool XSSAuditor::canSetBaseElementURL(const String& url) const

We can either do the null stripping here or find a way to tell findInRequest not to strip nulls.

Do we have this same null issue with <script src="..."> or canLoadObject?  If so, we should fix that in a separate bug.

[Daniel Bates]

Created attachment 32475 [details]
Patch with tests

Cleaned up patch.

[Adam Barth]

Comment on attachment 32475 [details]
Patch with tests

Great.  This got much cleaner now that you've done the null / control char thing for the other APIs.

[Adam Barth]

Sending        LayoutTests/ChangeLog
Adding         LayoutTests/http/tests/security/xssAuditor/base-href-control-char-expected.txt
Adding         LayoutTests/http/tests/security/xssAuditor/base-href-control-char.html
Adding         LayoutTests/http/tests/security/xssAuditor/base-href-expected.txt
Adding         LayoutTests/http/tests/security/xssAuditor/base-href-null-char-expected.txt
Adding         LayoutTests/http/tests/security/xssAuditor/base-href-null-char.html
Adding         LayoutTests/http/tests/security/xssAuditor/base-href-safe-expected.txt
Adding         LayoutTests/http/tests/security/xssAuditor/base-href-safe.html
Adding         LayoutTests/http/tests/security/xssAuditor/base-href-safe2-expected.txt
Adding         LayoutTests/http/tests/security/xssAuditor/base-href-safe2.html
Adding         LayoutTests/http/tests/security/xssAuditor/base-href-scheme-relative-expected.txt
Adding         LayoutTests/http/tests/security/xssAuditor/base-href-scheme-relative.html
Adding         LayoutTests/http/tests/security/xssAuditor/base-href.html
Adding         LayoutTests/http/tests/security/xssAuditor/resources/base-href
Adding         LayoutTests/http/tests/security/xssAuditor/resources/base-href/base-href-safe2.html
Adding         LayoutTests/http/tests/security/xssAuditor/resources/base-href/really-safe-script.js
Adding         LayoutTests/http/tests/security/xssAuditor/resources/base-href/safe-script.js
Adding         LayoutTests/http/tests/security/xssAuditor/resources/echo-head-base-href.pl
Adding         LayoutTests/http/tests/security/xssAuditor/resources/safe-script.js
Sending        WebCore/ChangeLog
Sending        WebCore/html/HTMLBaseElement.cpp
Sending        WebCore/html/HTMLBaseElement.h
Sending        WebCore/page/XSSAuditor.cpp
Sending        WebCore/page/XSSAuditor.h
Transmitting file data .......................
Committed revision 45642.
http://trac.webkit.org/changeset/45642