Bug 188644 - Implement further CORS restrictions
Summary: Implement further CORS restrictions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rob Buis
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-08-16 03:40 PDT by Anne van Kesteren
Modified: 2019-03-12 02:25 PDT (History)
9 users (show)

See Also:


Attachments
Patch (18.54 KB, patch)
2019-03-09 06:06 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (18.94 KB, patch)
2019-03-10 15:02 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.52 MB, application/zip)
2019-03-10 17:05 PDT, Build Bot
no flags Details
Patch (18.90 KB, patch)
2019-03-11 03:15 PDT, Rob Buis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Anne van Kesteren 2018-08-16 03:40:37 PDT
The one change made in https://github.com/whatwg/fetch/pull/736 that is relevant to WebKit is header value length restrictions.
Comment 1 Anne van Kesteren 2018-08-16 03:41:14 PDT
Apologies for submitting this early. I meant to also link https://github.com/web-platform-tests/wpt/pull/11432 which contains a number of tests for these changes.
Comment 2 Rob Buis 2019-03-09 06:06:57 PST
Created attachment 364128 [details]
Patch
Comment 3 Rob Buis 2019-03-10 15:02:31 PDT
Created attachment 364199 [details]
Patch
Comment 4 Darin Adler 2019-03-10 15:22:03 PDT
Comment on attachment 364199 [details]
Patch

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

> Source/WebCore/ChangeLog:26
> +        (WebCore::isCrossOriginSafeRequestHeader): check that header length is less than 128

Code checks for <= 128, but this says < 128. Which is it? Would be nice to have a test that is exactly at the maximum length and another that is exactly one more than the maximum length.

> Source/WebCore/Modules/fetch/FetchHeaders.cpp:59
> +            combinedValue = makeString(headers.get(name), ", ", normalizedValue);

Unfortunate that we need to do this concatenation work both here and inside HTTPHeaderMap::add. Wasteful because we compute the same string twice. Also breaks encapsulation since the ", " separator is now here as well as in the HTTPHeaderMap class. Would be nice to find some way to avoid those problems will still fixing this.

> Source/WebCore/platform/network/HTTPParsers.cpp:882
> +    return value.length() <= 128;

Why not put this check first? Then we wouldn’t need to make those other changes. Also needs a brief "why" comment, I think. Not obvious that a 129 character string isn’t safe, but easy to explain with a reference to where that rule is defined, or even just an "its a rule" remark.
Comment 5 Build Bot 2019-03-10 17:05:24 PDT
Comment on attachment 364199 [details]
Patch

Attachment 364199 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11449851

New failing tests:
imported/w3c/web-platform-tests/webrtc/simplecall-no-ssrcs.https.html
Comment 6 Build Bot 2019-03-10 17:05:26 PDT
Created attachment 364215 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6
Comment 7 Rob Buis 2019-03-11 03:15:10 PDT
Created attachment 364243 [details]
Patch
Comment 8 Rob Buis 2019-03-11 11:34:48 PDT
Comment on attachment 364199 [details]
Patch

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

>> Source/WebCore/ChangeLog:26
>> +        (WebCore::isCrossOriginSafeRequestHeader): check that header length is less than 128
> 
> Code checks for <= 128, but this says < 128. Which is it? Would be nice to have a test that is exactly at the maximum length and another that is exactly one more than the maximum length.

Yes that was wrong, I rephrased and made things consistent so it is hopefully clear.

>> Source/WebCore/Modules/fetch/FetchHeaders.cpp:59
>> +            combinedValue = makeString(headers.get(name), ", ", normalizedValue);
> 
> Unfortunate that we need to do this concatenation work both here and inside HTTPHeaderMap::add. Wasteful because we compute the same string twice. Also breaks encapsulation since the ", " separator is now here as well as in the HTTPHeaderMap class. Would be nice to find some way to avoid those problems will still fixing this.

Yes this code is not very pretty. I think I improved it by trying to do the work of add up front and setting it unconditionally if all checks are fine. Another option is to always pass the combined value as normal value to canWriteHeader, but then the isValidHTTPHeaderValue check would check the overall combined value, where it previously already had checked the existing header value (before combining), doing too much work. Seems no matter how this code is always a bit sub optimal...

>> Source/WebCore/platform/network/HTTPParsers.cpp:882
>> +    return value.length() <= 128;
> 
> Why not put this check first? Then we wouldn’t need to make those other changes. Also needs a brief "why" comment, I think. Not obvious that a 129 character string isn’t safe, but easy to explain with a reference to where that rule is defined, or even just an "its a rule" remark.

I would like to stick to the spec here and keep it at Step 3. Note that for header names not listed in the switch, putting it first means extra work, since although we would always return false for these anyway, we would still do the length check.
I think for the reference, the reference listed on isCrossOriginSafeRequestHeader covers all.
Comment 9 Rob Buis 2019-03-12 00:46:44 PDT
(In reply to Rob Buis from comment #8)
> >> Source/WebCore/platform/network/HTTPParsers.cpp:882
> >> +    return value.length() <= 128;
> > 
> > Why not put this check first? Then we wouldn’t need to make those other changes. Also needs a brief "why" comment, I think. Not obvious that a 129 character string isn’t safe, but easy to explain with a reference to where that rule is defined, or even just an "its a rule" remark.

It is not really clear from going through the related issue(s), maybe on purpose if there is a security issue involved. Closest I could find was "make it harder for attackers to determine cookie size". Anne would know more.
Comment 10 WebKit Commit Bot 2019-03-12 01:13:32 PDT
Comment on attachment 364243 [details]
Patch

Clearing flags on attachment: 364243

Committed r242786: <https://trac.webkit.org/changeset/242786>
Comment 11 WebKit Commit Bot 2019-03-12 01:13:34 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Radar WebKit Bug Importer 2019-03-12 01:14:18 PDT
<rdar://problem/48800887>
Comment 13 Anne van Kesteren 2019-03-12 02:14:09 PDT
128 was somewhat arbitrary, so "it's a rule" applies. The goal was reducing the number of bytes in request headers the attacker has control over, based on a paper (not sure if published) that shows some realistic attacks around that.
Comment 14 Rob Buis 2019-03-12 02:20:07 PDT
(In reply to Anne van Kesteren from comment #13)
> 128 was somewhat arbitrary, so "it's a rule" applies. The goal was reducing
> the number of bytes in request headers the attacker has control over, based
> on a paper (not sure if published) that shows some realistic attacks around
> that.

Thanks, Anne. BTW I know Chrome implements this rule but do you know if Firefox does as well? I would have put "Matches Chrome and Firefox behavior" in the ChangeLog, but was not sure about Firefox.
Comment 15 Anne van Kesteren 2019-03-12 02:25:44 PDT
Yeah, per https://bugzilla.mozilla.org/show_bug.cgi?id=1483815 it should.