Bug 188644 – Implement further CORS restrictions

• Component: DOM
• Status: RESOLVED FIXED

[Anne van Kesteren]

The one change made in https://github.com/whatwg/fetch/pull/736 that is relevant to WebKit is header value length restrictions.

[Anne van Kesteren]

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.

[Rob Buis]

Created attachment 364128 [details]
Patch

[Rob Buis]

Created attachment 364199 [details]
Patch

[Darin Adler]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Rob Buis]

Created attachment 364243 [details]
Patch

[Rob Buis]

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.

[Rob Buis]

(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.

[WebKit Commit Bot]

Comment on attachment 364243 [details]
Patch

Clearing flags on attachment: 364243

Committed r242786: <https://trac.webkit.org/changeset/242786>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/48800887>

[Anne van Kesteren]

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.

[Rob Buis]

(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.

[Anne van Kesteren]

Yeah, per https://bugzilla.mozilla.org/show_bug.cgi?id=1483815 it should.