We should prevent injections of <base href="...">, since this can be used to load external scripts from a malicious site.
Good catch Dan. We should probably handle this similarly to the <script> tag by checking the unparsed URL against the request.
Created attachment 32284 [details] Patch with tests Implements support for preventing injection of HTML Base tag.
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.
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.
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.
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.
Created attachment 32475 [details] Patch with tests Cleaned up patch.
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.
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