Bug 184493 – Disallow responses when a response contains invalid header values

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Anne van Kesteren]

See https://github.com/whatwg/xhr/issues/165 for the discussion that lead to this. https://github.com/w3c/web-platform-tests/pull/10424 has updated tests.

[Rob Buis]

Created attachment 386300 [details]
Patch

[Rob Buis]

Created attachment 386316 [details]
Patch

[Rob Buis]

Created attachment 386319 [details]
Patch

[youenn fablet]

Comment on attachment 386319 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        "- Contains no 0x00 (NUL) or HTTP newline bytes."

In theory, I guess we would want to reject all responses, not only responses consumed by XHR/fetch.
This might be fine to start with fetch/XHR.
We should probably try to do it for all responses so probably in networking process, maybe even at CFNetwork level.

Do you know whether other browsers did this change for all responses?
Are there tests for iframes loading responses with 0x00 headers?

[Darin Adler]

Comment on attachment 386319 [details]
Patch

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

r=me as is, some small style-related comments

> Source/WebCore/ChangeLog:3
> +        Disallow responses when a response header value contains 0x00

Title doesn’t seem to match the patch.

Added stripping of leading and trailing HTTP spaces is a change that seems to *allow* more responses and thus *loosens* our checking. This title makes it sound like this patch only *tightens* checks.

As far as that tightening is concerned, nothing in the patch is specific to NUL character checking, so a more accurate description of the change is that we disallow responses when the header value is invalid, which includes the newline checks as well.

> Source/WebCore/Modules/fetch/FetchHeaders.cpp:46
> -    if (!isValidHTTPHeaderValue(value))
> +    if (!isValidHTTPHeaderValue(stripLeadingAndTrailingHTTPSpaces(value)))

This is OK, but to me seems unnecessarily inefficient. We could easily make another version of the isValidHTTPHeaderValue function that could tolerate the leading and trailing spaces. Not sure if this is ever performance-critical enough. We also could change isValidHTTPHeaderValue to take a StringView since all it does is examine the characters in the string. Then we could use the StringView version of stripLeadingAndTrailingHTTPSpaces to avoid allocating and destroying a new WTF::String.

> Source/WebCore/Modules/fetch/FetchResponse.cpp:344
> +            responseCallback(Exception { TypeError, { } });

Calling this a "type" error seems a little peculiar. But not sure I have a better idea. Is this standardized?

> Source/WebCore/xml/XMLHttpRequest.cpp:980
> +        String normalizedValue = stripLeadingAndTrailingHTTPSpaces(header.value);
> +        if (!isValidHTTPHeaderValue(normalizedValue))

Why use a local variable here? I don’t think the word "normalized" is valuable documentation, and the combined line wouldn’t be super-long. And further, seeing the isValidHTTPHeaderValue wrapped around a call to stripLeadingAndTrailingHTTPSpaces might lead us to notice this and do a refactoring in the future to improve performance if it’s a recurring pattern (occurs twice in this patch).

[Darin Adler]

I didn’t see the comment from Youenn, which makes excellent points and asks relevant questions that could be important to answer before landing?

[Rob Buis]

(In reply to Darin Adler from comment #6)
> I didn’t see the comment from Youenn, which makes excellent points and asks
> relevant questions that could be important to answer before landing?

Yes, I was planning to address that today (post Christmas). To avoid situations like this I would be fine if Youenn did a r- in cases like this (until the questions are addressed).

[Rob Buis]

Created attachment 386470 [details]
Patch

[Rob Buis]

Comment on attachment 386319 [details]
Patch

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

>> Source/WebCore/ChangeLog:10
>> +        "- Contains no 0x00 (NUL) or HTTP newline bytes."
> 
> In theory, I guess we would want to reject all responses, not only responses consumed by XHR/fetch.
> This might be fine to start with fetch/XHR.
> We should probably try to do it for all responses so probably in networking process, maybe even at CFNetwork level.
> 
> Do you know whether other browsers did this change for all responses?
> Are there tests for iframes loading responses with 0x00 headers?

I did it this way to not slow down the generic case.Seems like we would need to do it for every platform or do it with an extra loop in the network process.

I also feel more comfortable doing this for Fetch/xhr first. OTOH chromium does try to make it generic:
https://chromium.googlesource.com/chromium/src.git/+/900336ff53907fb0263941689ee734103f0ad352

I do not think there are tests for iframes loading responses with 0x00 headers.

[Rob Buis]

Comment on attachment 386319 [details]
Patch

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

>> Source/WebCore/ChangeLog:3
>> +        Disallow responses when a response header value contains 0x00
> 
> Title doesn’t seem to match the patch.
> 
> Added stripping of leading and trailing HTTP spaces is a change that seems to *allow* more responses and thus *loosens* our checking. This title makes it sound like this patch only *tightens* checks.
> 
> As far as that tightening is concerned, nothing in the patch is specific to NUL character checking, so a more accurate description of the change is that we disallow responses when the header value is invalid, which includes the newline checks as well.

I changed the bug title.

>> Source/WebCore/Modules/fetch/FetchResponse.cpp:344
>> +            responseCallback(Exception { TypeError, { } });
> 
> Calling this a "type" error seems a little peculiar. But not sure I have a better idea. Is this standardized?

I don't think it is standardized. I think tests expect TypeError, although setRequestHeader throws SyntaxError:
https://xhr.spec.whatwg.org/#the-setrequestheader()-method

>> Source/WebCore/xml/XMLHttpRequest.cpp:980
>> +        if (!isValidHTTPHeaderValue(normalizedValue))
> 
> Why use a local variable here? I don’t think the word "normalized" is valuable documentation, and the combined line wouldn’t be super-long. And further, seeing the isValidHTTPHeaderValue wrapped around a call to stripLeadingAndTrailingHTTPSpaces might lead us to notice this and do a refactoring in the future to improve performance if it’s a recurring pattern (occurs twice in this patch).

Ok I removed the local variable.

[Anne van Kesteren]

The stripping of whitespace only happens for the API, right? Whereas the 0x00 is about network behavior. It might be good to clarify that or separate those changes. There should also not be any newline changes for network behavior.

Also, I think 0x00 resulting in a network error is ideally generic, though staged rollout might indeed be safer. I can add tests later for Youenn's scenario (and more, e.g., images).

[Anne van Kesteren]

Tests for Youenn: https://github.com/web-platform-tests/wpt/pull/21019. Happy new year! (Chrome indeed passes them.)

[Rob Buis]

Created attachment 399597 [details]
Patch

[youenn fablet]

Comment on attachment 399597 [details]
Patch

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

> Source/WebCore/ChangeLog:27
> +        (WebCore::XMLHttpRequest::didReceiveResponse):

It seems we should do that check for any load. Here we do it for some clients only.
For instance an image load or a document load should give the same error. It would be nice to add such tests.

How about doing that check in ResourceLoader::didReceiveResponse?
We could also do the check in NetworkLoadChecker::validateResponse to prevent this early on.

[Rob Buis]

Comment on attachment 399597 [details]
Patch

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

>> Source/WebCore/ChangeLog:27
>> +        (WebCore::XMLHttpRequest::didReceiveResponse):
> 
> It seems we should do that check for any load. Here we do it for some clients only.
> For instance an image load or a document load should give the same error. It would be nice to add such tests.
> 
> How about doing that check in ResourceLoader::didReceiveResponse?
> We could also do the check in NetworkLoadChecker::validateResponse to prevent this early on.

There are non fetch/XHR tests here https://github.com/web-platform-tests/wpt/pull/21019.

My idea was to implement a more generic fix as a follow up patch, that would fix the tests in https://github.com/web-platform-tests/wpt/pull/21019. I fear this patch would become a bit big otherwise. But let me know what you prefer?

[Rob Buis]

Created attachment 400558 [details]
Patch

[Rob Buis]

Created attachment 400566 [details]
Patch

[Rob Buis]

Created attachment 400569 [details]
Patch

[Rob Buis]

Comment on attachment 399597 [details]
Patch

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

>>> Source/WebCore/ChangeLog:27
>>> +        (WebCore::XMLHttpRequest::didReceiveResponse):
>> 
>> It seems we should do that check for any load. Here we do it for some clients only.
>> For instance an image load or a document load should give the same error. It would be nice to add such tests.
>> 
>> How about doing that check in ResourceLoader::didReceiveResponse?
>> We could also do the check in NetworkLoadChecker::validateResponse to prevent this early on.
> 
> There are non fetch/XHR tests here https://github.com/web-platform-tests/wpt/pull/21019.
> 
> My idea was to implement a more generic fix as a follow up patch, that would fix the tests in https://github.com/web-platform-tests/wpt/pull/21019. I fear this patch would become a bit big otherwise. But let me know what you prefer?

I did some more work on this, SubresourceLoader::didReceiveResponse seems to be a good place to do the check, however the sync path of DocumentThreadableLoader (for xmlhttprequest) needs its own check. I also imported the h1-parsing tests.

[Rob Buis]

Comment on attachment 400569 [details]
Patch

PTAL. The failure in the resources-with-0x00-in-header.window.html test is because on failed iframe loads the existing document in the frame is not cleared (unlike other implementations), however this seems like a different bug/feature. Finally, I am not sure if the error message should be specific or generic here.

[youenn fablet]

Comment on attachment 400569 [details]
Patch

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

> Source/WebCore/Modules/fetch/FetchHeaders.cpp:46
> +    if (!isValidHTTPHeaderValue(stripLeadingAndTrailingHTTPSpaces(value)))

The idea was that the caller of canWriteHeader would do the stripping.
Maybe we should add an ASSERT here and update the two call sites that are not explicitly stripping spaces.

> Source/WebCore/loader/DocumentThreadableLoader.cpp:401
> +    if (!m_async && response.containsInvalidHTTPHeaders()) {

Let's move this check in the synchronous code path, in DocumentThreadableLoader::loadRequest.

> Source/WebCore/loader/DocumentThreadableLoader.cpp:403
> +        didFail(identifier, error);

Could be a one liner.

> Source/WebCore/loader/SubresourceLoader.cpp:440
> +    if (/*m_resource->type() != CachedResource::Type::MainResource &&*/ response.containsInvalidHTTPHeaders()) {

Should remove the comment.
Also, this seems a bit late, we should probably do this check at the beginning of didReceiveResponse.

> Source/WebCore/loader/SubresourceLoader.cpp:442
> +        didFail(error);

Could be a one liner with above line

[Rob Buis]

Created attachment 400588 [details]
Patch

[Rob Buis]

Created attachment 400589 [details]
Patch

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Rob Buis]

Created attachment 400591 [details]
Patch

[EWS]

Committed r262335: <https://trac.webkit.org/changeset/262335>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 400591 [details].

[Radar WebKit Bug Importer]

<rdar://problem/63784115>

[Simon Fraser (smfr)]

This is causing WK1 crashes, as you can see from EWS.

[Yusuke Suzuki]

Re-opened since this is blocked by bug 212571

[Rob Buis]

(In reply to Simon Fraser (smfr) from comment #28)
> This is causing WK1 crashes, as you can see from EWS.

For the record, this is also crashing on WK2, but a) there is no WK2 Debug bot to show this and b) on WK2 some of the tests that crash on WK1 are skipped for WK2.

[Rob Buis]

Created attachment 400722 [details]
Patch

[Rob Buis]

The ASSERT in isValidHTTPHeaderValue that was hit does not seem needed. The code paths using this are XHR setRequestHeader and Fetch. However these APIs will not hit it since the JS side uses ByteString which will throw type error on non Latin1.
Finally, Fetch does not specify non-latin1 to be invalid:
https://fetch.spec.whatwg.org/#concept-header-value

[Rob Buis]

Created attachment 400724 [details]
Patch

[Darin Adler]

Comment on attachment 400724 [details]
Patch

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

> Source/WebCore/Modules/fetch/FetchHeaders.cpp:46
> +    ASSERT(value.isEmpty() || (!isHTTPSpace(value[0]) && !isHTTPSpace(value[value.length() - 1])));

What guarantees what this asserts? Caller responsibility to call stripLeadingAndTrailingHTTPSpaces? What's the benefit of this assertion? Seems like the check will fail in this case, so it won’t "slip by"?

> Source/WebCore/Modules/fetch/FetchHeaders.cpp:81
> +    String normalizedValue = stripLeadingAndTrailingHTTPSpaces(header.value);

Seems unnecessarily inefficient to allocate a new String if there are leading or trailing spaces. Cleaner if some day we could refactor to do these operations with StringView instead, which would avoid memory allocation entirely.

> Source/WebCore/Modules/fetch/FetchHeaders.cpp:212
> +        String normalizedValue = stripLeadingAndTrailingHTTPSpaces(header.value);

Ditto.

> Source/WebCore/platform/network/ResourceResponseBase.cpp:845
> +        if (!isValidHTTPHeaderValue(stripLeadingAndTrailingHTTPSpaces(header.value)))

Ditto.

[Rob Buis]

Comment on attachment 400724 [details]
Patch

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

>> Source/WebCore/Modules/fetch/FetchHeaders.cpp:46
>> +    ASSERT(value.isEmpty() || (!isHTTPSpace(value[0]) && !isHTTPSpace(value[value.length() - 1])));
> 
> What guarantees what this asserts? Caller responsibility to call stripLeadingAndTrailingHTTPSpaces? What's the benefit of this assertion? Seems like the check will fail in this case, so it won’t "slip by"?

This was Youenn's idea :) It is indeed a way to ensure callers call stripLeadingAndTrailingHTTPSpaces before calling canWriteHeader.

>> Source/WebCore/Modules/fetch/FetchHeaders.cpp:81
>> +    String normalizedValue = stripLeadingAndTrailingHTTPSpaces(header.value);
> 
> Seems unnecessarily inefficient to allocate a new String if there are leading or trailing spaces. Cleaner if some day we could refactor to do these operations with StringView instead, which would avoid memory allocation entirely.

That makes sense. You can open a bug and assign it to me if you want. Regardless of the assignee the bug would make sense in fact.

[EWS]

Committed r262511: <https://trac.webkit.org/changeset/262511>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 400724 [details].