Bug 68560 – [Qt] HTTP header injection via QWebPage::userAgentForUrl()

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Jarred Nicholls]

Created attachment 108210 [details]
userAgentForUrl stripping

This is an interesting issue, that isn't any one particular tier's fault.

When working with PhantomJS (based on QtWebKit), where the user is able to provide their own user agent, we discovered that the user (or attacker?) can set the user agent in such a way to add another HTTP header implicitly to the ResourceRequest:

userAgent = 'My User Agent\nX-My-New-Header: oh noes!';

Which will result in sending the HTTP request w/ headers:

User-Agent: My User Agent
X-My-New-Header: oh noes!

Clearly, this could be problematic.  HTTPHeaderMap doesn't filter the values at all (relying on platform network stack?).  QNetworkRequest clearly just writes the headers onto the stream without a care.  This is either everyone's fault or no one's fault.  While XMLHttpRequest does a validity check of header values (to make sure they don't contain \r or \n) since it's a scriptable API, internal header management does not validate, falling back to the platform's network stack to handle it properly.  Though this is an edge case, QWebPage is offering to the consumer of the QtWebKit API the ability to set the user agent header and doesn't protect it.

Attached is a patch that does the obvious and strips out newline chars to protect this particular scenario, from within WebCore.  But, maybe some handling belongs in QNetwork as well, which would make this a temporary patch.  Or such behavior is acceptable in QNetwork, which means the validation should occur on a higher tier.

Input on this matter is welcomed!

[Jesus Sanchez-Palencia]

(In reply to comment #0)
> Input on this matter is welcomed!

I'm not sure where exactly the "protection" should go, but I agree that we need it. Let's just make sure to have a test covering it as well.

Now that we had this heads-up, is there any other place our API might be prone to injection?!

[Jarred Nicholls]

(In reply to comment #1)
> Now that we had this heads-up, is there any other place our API might be prone to injection?!

Good question; I'll scour the API to see if any relevant issues are present elsewhere.  In the meantime though I will prepare an official patch w/ a test to cover up this one case in particular.  Sound good?

[Ademar Reis]

I fail to see an attack scenario... How could an attacker provide the user-agent? Maybe I'm missing a feature from your PhanonJS, or maybe you don't want to trust the end user at all? Could you please provide a real world attack scenario?

Finally, if this (or any other bug you find) is indeed a security vulnerability, please open a bug against the Security component. This way the bug is kept private while we fix it and give some time to all vendors to fix their applications and distribute the patches to end users (in this case the only vendor would be QtWebKit).

More details about the WebKit security policy here: http://www.webkit.org/security/

[Robert Hogan]

This is an interesting find, but I agree with Ademar it's not a vulnerability. If a client is allowing third-party code to set the UA there isn't much additional leverage to be had from adding a new header. Still worth fixing though.

[Jarred Nicholls]

(In reply to comment #3)
> I fail to see an attack scenario... How could an attacker provide the user-agent? Maybe I'm missing a feature from your PhanonJS, or maybe you don't want to trust the end user at all? Could you please provide a real world attack scenario?
> 
> Finally, if this (or any other bug you find) is indeed a security vulnerability, please open a bug against the Security component. This way the bug is kept private while we fix it and give some time to all vendors to fix their applications and distribute the patches to end users (in this case the only vendor would be QtWebKit).
> 
> More details about the WebKit security policy here: http://www.webkit.org/security/

I consider this more of a straight up bug in handling the result from userAgentFromUrl - not so much a security problem.

My scenario: because PhantomJS exposes a global "phantomjs" variable in the context of a web site (as a tool its often used to scrape web sites), a site could detect its presence and set the user agent overriding some identity headers (cookies, preflight origin w/ CORS? etc.) and who the heck knows what could come next.

Niche, and no big deal.  But, nevertheless, userAgentFromUrl shouldn't return an invalid header value; at least that we can all agree with.

[Ariya Hidayat]

Related PhantomJS comment:

http://code.google.com/p/phantomjs/issues/detail?id=45#c5

[Ademar Reis]

Changing the bug title to clarify this is not a security problem.

[Jarred Nicholls]

Created attachment 119879 [details]
Patch

[WebKit Review Bot]

Attachment 119879 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/qt/ChangeLog', u'Source/WebK..." exit_code: 1

Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:2677:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jarred Nicholls]

> Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:2677:  This { should be at the end of the previous line  [whitespace/braces] [4]
> Total errors found: 1 in 3 files

false positive

[Early Warning System Bot]

Comment on attachment 119879 [details]
Patch

Attachment 119879 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10934582

[Jarred Nicholls]

Created attachment 119881 [details]
Patch

[WebKit Review Bot]

Attachment 119881 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/qt/ChangeLog', u'Source/WebK..." exit_code: 1

Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:2677:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 119881 [details]
Patch

Attachment 119881 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10928607

[Jarred Nicholls]

Created attachment 119884 [details]
Patch

[WebKit Review Bot]

Attachment 119884 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/qt/ChangeLog', u'Source/WebK..." exit_code: 1

Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:2677:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Benjamin Poulain]

Comment on attachment 119884 [details]
Patch

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

> Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:739
> +        return m_webFrame->page()->userAgentForUrl(url).replace(QRegExp(QLatin1String("[\n\r]")), QLatin1String(""));

QLatin1String("") -> QLatin1String().
Using QRegExp() is always a bad idea.

[Benjamin Poulain]

Comment on attachment 119884 [details]
Patch

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

> Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:2680
> +    virtual QString userAgentForUrl(const QUrl& url) const
> +    { return QString("My User Agent\nX-New-Http-Header: Oh Noes!"); }

Coding style.

> Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:2686
> +    QString userAgent("My User AgentX-New-Http-Header: Oh Noes!");

Make that a static const QLatin1String in CustomUserAgentWebPage?

[Jarred Nicholls]

Created attachment 119908 [details]
Patch

[WebKit Review Bot]

Attachment 119908 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/qt/ChangeLog', u'Source/WebK..." exit_code: 1

Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:2677:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 119908 [details]
Patch

Attachment 119908 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10943036

[Jarred Nicholls]

(In reply to comment #21)
> (From update of attachment 119908 [details])
> Attachment 119908 [details] did not pass qt-ews (qt):
> Output: http://queues.webkit.org/results/10943036

lol, thanks Benjamin ;-P  I love cowboying patches...

[Jarred Nicholls]

Created attachment 119911 [details]
Patch

[WebKit Review Bot]

Attachment 119911 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/qt/ChangeLog', u'Source/WebK..." exit_code: 1

Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:2677:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jarred Nicholls]

Created attachment 119916 [details]
Patch

[WebKit Review Bot]

Attachment 119916 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/qt/ChangeLog', u'Source/WebK..." exit_code: 1

Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:2677:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Benjamin Poulain]

Comment on attachment 119916 [details]
Patch

Actually, I know think this is incorrect. A end of line is valid in a header with a continuation (http://www.w3.org/Protocols/rfc2616/rfc2616-sec2.html#sec2.2 ). You might decide not to handle this case in Qt

[Benjamin Poulain]

Comment on attachment 119916 [details]
Patch

Actually, I know think this is incorrect. A end of line is valid in a header with a continuation (http://www.w3.org/Protocols/rfc2616/rfc2616-sec2.html#sec2.2 ). You might decide not to handle this case in Qt

[Jarred Nicholls]

(In reply to comment #28)
> (From update of attachment 119916 [details])
> Actually, I know think this is incorrect. A end of line is valid in a header with a continuation (http://www.w3.org/Protocols/rfc2616/rfc2616-sec2.html#sec2.2 ). You might decide not to handle this case in Qt

We can do a white space check after a crlf before deciding to strip.  I'm dropping this bug for now, someone can choose to merge this as-is, enhance it per the rfc, or forget about it.  For user agent though I can say that it's reasonable to forget about line comtinuations...

[Simon Hausmann]

Comment on attachment 119916 [details]
Patch

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

I think that's a very good solution to the problem. I think this can be written more efficiently though and there's a style nitpick :)

> Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:739
> +        return m_webFrame->page()->userAgentForUrl(url).replace(QLatin1String("\n"), QString()).replace(QLatin1String("\r"), QString());

I think it is better to use remove() instead of replace():

    return m_webFrame->page()->userAgentForUrl(url).remove(QLatin1Char('\n')).remove(QLatin1Char('\r'));

I think in the common case of _no_ newlines/linefeeds being present, the simple iteration that QString::remove(QChar) does is faster
than creating temporary QStrings and firing up the QStringMatcher machinery.

[Simon Hausmann]

(In reply to comment #29)
> (In reply to comment #28)
> > (From update of attachment 119916 [details] [details])
> > Actually, I know think this is incorrect. A end of line is valid in a header with a continuation (http://www.w3.org/Protocols/rfc2616/rfc2616-sec2.html#sec2.2 ). You might decide not to handle this case in Qt
> 
> We can do a white space check after a crlf before deciding to strip.  I'm dropping this bug for now, someone can choose to merge this as-is, enhance it per the rfc, or forget about it.  For user agent though I can say that it's reasonable to forget about line comtinuations...

Yeah, I'm inclined to agree. Let's not worry about continuations while at the same time the fix to this problem is simple enough that we could put it straight into the library :)

[Jarred Nicholls]

Created attachment 120003 [details]
Patch

Sounds good Simon, remove() for the win.  New patch...

[WebKit Review Bot]

Attachment 120003 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/qt/ChangeLog', u'Source/WebK..." exit_code: 1

Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:2677:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Hausmann]

Comment on attachment 120003 [details]
Patch

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

>> Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:2677
>> +{
> 
> This { should be at the end of the previous line  [whitespace/braces] [4]

Style queue is right here.... ;) Want to fix it when landing?

[Jarred Nicholls]

(In reply to comment #34)
> (From update of attachment 120003 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120003&action=review
> 
> >> Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:2677
> >> +{
> > 
> > This { should be at the end of the previous line  [whitespace/braces] [4]
> 
> Style queue is right here.... ;) Want to fix it when landing?

I can but it's consistent with *all* of the other test classes in there; I purposefully ignored the style checker on this one.  I'd rather stay consistent to the file.  Let me know and I'll do one or the other before landing.

[Simon Hausmann]

(In reply to comment #35)
> (In reply to comment #34)
> > (From update of attachment 120003 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=120003&action=review
> > 
> > >> Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:2677
> > >> +{
> > > 
> > > This { should be at the end of the previous line  [whitespace/braces] [4]
> > 
> > Style queue is right here.... ;) Want to fix it when landing?
> 
> I can but it's consistent with *all* of the other test classes in there; I purposefully ignored the style checker on this one.  I'd rather stay consistent to the file.  Let me know and I'll do one or the other before landing.

That's fine with me, too :)

[Simon Hausmann]

Comment on attachment 120003 [details]
Patch

In that case, let's give it a go :)

[Jarred Nicholls]

(In reply to comment #37)
> (From update of attachment 120003 [details])
> In that case, let's give it a go :)

Ok cool :)  Thanks for the review.

[WebKit Review Bot]

Comment on attachment 120003 [details]
Patch

Clearing flags on attachment: 120003

Committed r103320: <http://trac.webkit.org/changeset/103320>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.