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!
(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?!
(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?
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/
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.
(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.
Related PhantomJS comment: http://code.google.com/p/phantomjs/issues/detail?id=45#c5
Changing the bug title to clarify this is not a security problem.
Created attachment 119879 [details] Patch
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.
> 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
Comment on attachment 119879 [details] Patch Attachment 119879 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10934582
Created attachment 119881 [details] Patch
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.
Comment on attachment 119881 [details] Patch Attachment 119881 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10928607
Created attachment 119884 [details] Patch
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.
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.
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?
Created attachment 119908 [details] Patch
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.
Comment on attachment 119908 [details] Patch Attachment 119908 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10943036
(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...
Created attachment 119911 [details] Patch
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.
Created attachment 119916 [details] Patch
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.
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
(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...
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.
(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 :)
Created attachment 120003 [details] Patch Sounds good Simon, remove() for the win. New patch...
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.
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?
(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.
(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 :)
Comment on attachment 120003 [details] Patch In that case, let's give it a go :)
(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.
Comment on attachment 120003 [details] Patch Clearing flags on attachment: 120003 Committed r103320: <http://trac.webkit.org/changeset/103320>
All reviewed patches have been landed. Closing bug.