Bug 166363 - Switch to a blacklist model for restricted Accept headers in simple CORS requests
Summary: Switch to a blacklist model for restricted Accept headers in simple CORS requ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: John Wilander
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-12-21 12:10 PST by John Wilander
Modified: 2017-01-19 02:07 PST (History)
9 users (show)

See Also:


Attachments
Patch (12.67 KB, patch)
2016-12-21 12:20 PST, John Wilander
no flags Details | Formatted Diff | Diff
Patch (16.16 KB, patch)
2016-12-21 12:42 PST, John Wilander
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Wilander 2016-12-21 12:10:50 PST
The whitelist model in https://bugs.webkit.org/show_bug.cgi?id=165178 (and follow-up https://bugs.webkit.org/show_bug.cgi?id=165566) was too strict for Accept headers. We should move to a blacklist of delimiter character instead.
Comment 1 John Wilander 2016-12-21 12:20:47 PST
Created attachment 297616 [details]
Patch
Comment 2 WebKit Commit Bot 2016-12-21 12:21:54 PST
Attachment 297616 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/network/HTTPParsers.cpp:145:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 John Wilander 2016-12-21 12:22:47 PST
(In reply to comment #2)
> Attachment 297616 [details] did not pass style-queue:
> 
> 
> ERROR: Source/WebCore/platform/network/HTTPParsers.cpp:145:  An else if
> statement should be written as an if statement when the prior "if" concludes
> with a return, break, continue or goto statement. 
> [readability/control_flow] [4]
> Total errors found: 1 in 5 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

This is a deliberate violation of the style rules. Please assess in review.
Comment 4 John Wilander 2016-12-21 12:23:15 PST
rdar://problem/29635131
Comment 5 Alex Christensen 2016-12-21 12:24:26 PST
Comment on attachment 297616 [details]
Patch

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

> LayoutTests/http/tests/xmlhttprequest/cors-non-standard-safelisted-headers-should-trigger-preflight.html:116
> +            headersToAdd: [{ name : "Accept", value: allDisallowedDelimiterCharactersForAcceptHeader }],

A better test would be to test that each one of the disallowed characters caused preflight.  With this test method, if you had an allowed character in the set of disallowed characters, the test would still pass.
Comment 6 Alex Christensen 2016-12-21 12:26:22 PST
Comment on attachment 297616 [details]
Patch

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

> Source/WebCore/platform/network/HTTPParsers.cpp:147
> +        else if (isDelimiterCharacter(c))

Yep, this else isn't necessary.
Comment 7 John Wilander 2016-12-21 12:30:24 PST
(In reply to comment #5)
> Comment on attachment 297616 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=297616&action=review
> 
> > LayoutTests/http/tests/xmlhttprequest/cors-non-standard-safelisted-headers-should-trigger-preflight.html:116
> > +            headersToAdd: [{ name : "Accept", value: allDisallowedDelimiterCharactersForAcceptHeader }],
> 
> A better test would be to test that each one of the disallowed characters
> caused preflight.  With this test method, if you had an allowed character in
> the set of disallowed characters, the test would still pass.

Got it. This was an update for a test Youenn requested last time over. I'll update with your suggestion.
Comment 8 John Wilander 2016-12-21 12:31:06 PST
(In reply to comment #6)
> Comment on attachment 297616 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=297616&action=review
> 
> > Source/WebCore/platform/network/HTTPParsers.cpp:147
> > +        else if (isDelimiterCharacter(c))
> 
> Yep, this else isn't necessary.

Oh, now I see what the style complaint was about. Will fix.
Comment 9 John Wilander 2016-12-21 12:42:53 PST
Created attachment 297617 [details]
Patch
Comment 10 WebKit Commit Bot 2016-12-21 14:07:05 PST
Comment on attachment 297617 [details]
Patch

Clearing flags on attachment: 297617

Committed r210077: <http://trac.webkit.org/changeset/210077>
Comment 11 WebKit Commit Bot 2016-12-21 14:07:09 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Anne van Kesteren 2017-01-16 23:58:54 PST
John, I would appreciate if you had communicated this in the Fetch repository. It seems rather strange for only WebKit to enforce this and no other browser.
Comment 13 Chris Dumez 2017-01-17 08:32:47 PST
(In reply to comment #12)
> John, I would appreciate if you had communicated this in the Fetch
> repository. It seems rather strange for only WebKit to enforce this and no
> other browser.

@John, isn't this the change that's breaking Web sites too?
Comment 14 John Wilander 2017-01-17 10:26:32 PST
(In reply to comment #12)
> John, I would appreciate if you had communicated this in the Fetch
> repository. It seems rather strange for only WebKit to enforce this and no
> other browser.

Sure. First, this is not shipping to regular users in Safari. It is in WebKit Nightly, Safari Technology Preview, and of course any other user of WebKit that decides to pick up the patch.
   Our intention is to get real world feedback before we decide whether to ship in Safari or not. We've seen a couple of broken sites which is why the patch has evolved over three commits. Currently there's just one site reported to have problems and it's because it uses '_' in a language tag. We consider it reasonable to require a preflight for characters outside the header spec.

This issue was originally filed under embargo with all major browser engines fall 2014. Since the behavior is by specification and also not a security bug in the browsers themselves it was hard for us to get coordination on a change. We tried.
   When we joined W3C WebAppSec spring 2016 we brought this up at the first face-to-face meeting we attended. Minutes here: https://www.w3.org/2011/webappsec/minutes/2016-05-17-webappsec-minutes.html#item08 .
   We were asked to file a Fetch bug which we did: https://github.com/whatwg/fetch/issues/382 . The discussion got moved to https://github.com/whatwg/fetch/issues/313 where it tapered off after my comment on September 9. Jonas Sicking had left Mozilla by then which might explain why he left it hanging.
   The discussion instead moved to https://github.com/w3c/webappsec-csp/issues/115 and was brought up on the phone meeting November 16. The minutes are not yet available (https://www.w3.org/2011/webappsec/Minutes.html) but my personal notes said "Google expressed a favorable stance on this proposal. Mozilla needs to re-assess it since the engineer who made comments previously has left the company."

With all of the above and the long wait to do something about this we decided to implement and see if there is significant breakage. We will report on our findings to both Fetch and WebAppSec.

What would you like us to do meanwhile? Thanks!
Comment 15 Anne van Kesteren 2017-01-19 02:07:13 PST
John, it seems my comment was somewhat premature. I didn't realize this was only an experiment for now. I thought it was meant to ship. I also didn't know that Google might be on board. Thanks for the update.