Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2134083003:
Reject line terminators in HttpUtil::IsValidHeaderValue() (Closed)
DescriptionReject line terminators in HttpUtil::IsValidHeaderValue()
IsValidHeaderValue() is used for validation of untrusted input in at
least two places:
https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/downloads/downloads_api.cc?q=file:%5Esrc/chrome/browser/extensions/api/downloads/downloads_api.cc+IsValidHeaderName
https://cs.chromium.org/chromium/src/content/browser/renderer_host/websocket_host.cc?q=file:%5Esrc/content/browser/renderer_host/websocket_host.cc+IsValidHeaderValue
Currently this method only rejects an explicit CRNL in the header
value. However, some web servers treat a single CR or NL as line
terminators, even when other lines in the same request use CRNL. In
particular, Apache 2.4.10 treats NL this way.
This means that input validated by this function can still inject new
headers, at least for some servers.
This CL modifies the function to reject individual CR and NL in the
header value. This is still not as strict as RFC 7230, but it is
sufficient to prevent the potential privilege escalation.
BUG=627394
Committed: https://crrev.com/b0723324a1ea7ee8b3fc71e61ed8767c045f9883
Cr-Commit-Position: refs/heads/master@{#404987}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Changes from mmenke #
Messages
Total messages: 25 (9 generated)
ricea@chromium.org changed reviewers: + hiroshige@chromium.org
hiroshige, I am sending this to you since you were involved in the discussion at http://crbug.com/455099 This CL strengthens the check but not as much as is proposed in the bug. In practice it shouldn't break anything that is not broken already.
Looks basically good. This CL makes HttpUtil::IsValidHeaderValue() consistent with Blink's isValidHTTPHeaderValue(). HttpRequestHeaders::SetHeader() has DCHECK(HttpUtil::IsValidHeaderValue()). Does the callers of HttpRequestHeaders::SetHeader() ensure that the value doesn't contain a single CR or LF? Is this a part of banning single CR or LF throughout Chromium?
On 2016/07/12 09:17:57, hiroshige wrote: > HttpRequestHeaders::SetHeader() has DCHECK(HttpUtil::IsValidHeaderValue()). > Does the callers of HttpRequestHeaders::SetHeader() ensure that the value > doesn't contain a single CR or LF? Most callers generate the data in a way that wouldn't contain those characters anyway. In the cases I've seen that pass in untrusted data, they validate it with IsValidHeaderValue(), but there may be some cases I've missed. We could temporarily change the DCHECK to a CHECK to try to find any misuse. > Is this a part of banning single CR or LF throughout Chromium? No. The issue with response headers is more difficult because of compatibility concerns. As far as I know, other major browsers still permit "\n" as a line terminator in response headers and nobody wants to be first to change.
Description was changed from ========== Stengthen HttpUtil::IsValidHeaderValue() IsValidHeaderValue() is used for validation of untrusted input in at least two places: https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/downloads/... https://cs.chromium.org/chromium/src/content/browser/renderer_host/websocket_... Currently this method only rejects an explicit CRNL in the header value. However, some web servers treat a single CR or NL as line terminators, even when other lines in the same request use CRNL. In particular, Apache 2.4.10 treats NL this way. This means that input validated by this function can still inject new headers, at least for some servers. This CL modifies the function to reject individual CR and NL in the header value. This is still not as strict as RFC 7230, but it is sufficient to prevent the potential privilege escalation. BUG= ========== to ========== Reject line terminators in HttpUtil::IsValidHeaderValue() IsValidHeaderValue() is used for validation of untrusted input in at least two places: https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/downloads/... https://cs.chromium.org/chromium/src/content/browser/renderer_host/websocket_... Currently this method only rejects an explicit CRNL in the header value. However, some web servers treat a single CR or NL as line terminators, even when other lines in the same request use CRNL. In particular, Apache 2.4.10 treats NL this way. This means that input validated by this function can still inject new headers, at least for some servers. This CL modifies the function to reject individual CR and NL in the header value. This is still not as strict as RFC 7230, but it is sufficient to prevent the potential privilege escalation. BUG= ==========
non-owner lgtm. > We could temporarily change the DCHECK to a CHECK to try to find any misuse. Sounds good. > > Is this a part of banning single CR or LF throughout Chromium? > > No. The issue with response headers is more difficult because of compatibility > concerns. As far as I know, other major browsers still permit "\n" as a line > terminator in response headers and nobody wants to be first to change. I see. Could you associate a crbug entry to this CL to clarify the context of this CL? CL description: > https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/downloads/... > https://cs.chromium.org/chromium/src/content/browser/renderer_host/websocket_... Could you mention the function names in addition to the link? Codesearch links by line numbers will be outdated in the future.
Description was changed from ========== Reject line terminators in HttpUtil::IsValidHeaderValue() IsValidHeaderValue() is used for validation of untrusted input in at least two places: https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/downloads/... https://cs.chromium.org/chromium/src/content/browser/renderer_host/websocket_... Currently this method only rejects an explicit CRNL in the header value. However, some web servers treat a single CR or NL as line terminators, even when other lines in the same request use CRNL. In particular, Apache 2.4.10 treats NL this way. This means that input validated by this function can still inject new headers, at least for some servers. This CL modifies the function to reject individual CR and NL in the header value. This is still not as strict as RFC 7230, but it is sufficient to prevent the potential privilege escalation. BUG= ========== to ========== Reject line terminators in HttpUtil::IsValidHeaderValue() IsValidHeaderValue() is used for validation of untrusted input in at least two places: https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/downloads/... https://cs.chromium.org/chromium/src/content/browser/renderer_host/websocket_... Currently this method only rejects an explicit CRNL in the header value. However, some web servers treat a single CR or NL as line terminators, even when other lines in the same request use CRNL. In particular, Apache 2.4.10 treats NL this way. This means that input validated by this function can still inject new headers, at least for some servers. This CL modifies the function to reject individual CR and NL in the header value. This is still not as strict as RFC 7230, but it is sufficient to prevent the potential privilege escalation. BUG= ==========
Description was changed from ========== Reject line terminators in HttpUtil::IsValidHeaderValue() IsValidHeaderValue() is used for validation of untrusted input in at least two places: https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/downloads/... https://cs.chromium.org/chromium/src/content/browser/renderer_host/websocket_... Currently this method only rejects an explicit CRNL in the header value. However, some web servers treat a single CR or NL as line terminators, even when other lines in the same request use CRNL. In particular, Apache 2.4.10 treats NL this way. This means that input validated by this function can still inject new headers, at least for some servers. This CL modifies the function to reject individual CR and NL in the header value. This is still not as strict as RFC 7230, but it is sufficient to prevent the potential privilege escalation. BUG= ========== to ========== Reject line terminators in HttpUtil::IsValidHeaderValue() IsValidHeaderValue() is used for validation of untrusted input in at least two places: https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/downloads/... https://cs.chromium.org/chromium/src/content/browser/renderer_host/websocket_... Currently this method only rejects an explicit CRNL in the header value. However, some web servers treat a single CR or NL as line terminators, even when other lines in the same request use CRNL. In particular, Apache 2.4.10 treats NL this way. This means that input validated by this function can still inject new headers, at least for some servers. This CL modifies the function to reject individual CR and NL in the header value. This is still not as strict as RFC 7230, but it is sufficient to prevent the potential privilege escalation. BUG=627394 ==========
On 2016/07/12 10:15:31, hiroshige wrote: > > We could temporarily change the DCHECK to a CHECK to try to find any misuse. > > Sounds good. I've create crbug.com/627398 to track this. > I see. Could you associate a crbug entry to this CL to clarify the context of > this CL? Done. > Could you mention the function names in addition to the link? > Codesearch links by line numbers will be outdated in the future. I figured out how to make better links. They should work as long as the files are still there.
ricea@chromium.org changed reviewers: + mmenke@chromium.org
+mmenke for //net OWNERS. You seem to have done most recent reviews in this area, but feel free to reassign.
On 2016/07/12 10:42:10, Adam Rice wrote: > +mmenke for //net OWNERS. > You seem to have done most recent reviews in this area, but feel free to > reassign. LGTM. This seems reasonable, though I'm unsure about fallout.
https://codereview.chromium.org/2134083003/diff/1/net/http/http_util_unittest.cc File net/http/http_util_unittest.cc (right): https://codereview.chromium.org/2134083003/diff/1/net/http/http_util_unittest... net/http/http_util_unittest.cc:1371: static const char* const invalid_values[] = { nit: Don't use static (I think the consensus is it's a bad idea?) https://codereview.chromium.org/2134083003/diff/1/net/http/http_util_unittest... net/http/http_util_unittest.cc:1377: for (std::string value : invalid_values) { nit: const std::string& https://codereview.chromium.org/2134083003/diff/1/net/http/http_util_unittest... net/http/http_util_unittest.cc:1381: } nit: Suggest a blank line here
On 2016/07/12 18:21:26, mmenke wrote: > LGTM. This seems reasonable, though I'm unsure about fallout. My philosophy is that anything relying on consistent behaviour for CR or NL in header values is already broken, and so this can't make it any worse.
https://codereview.chromium.org/2134083003/diff/1/net/http/http_util_unittest.cc File net/http/http_util_unittest.cc (right): https://codereview.chromium.org/2134083003/diff/1/net/http/http_util_unittest... net/http/http_util_unittest.cc:1371: static const char* const invalid_values[] = { On 2016/07/12 18:24:49, mmenke wrote: > nit: Don't use static (I think the consensus is it's a bad idea?) I wasn't aware of that. I'm never quite sure when the compiler can avoid the copy, so I have tended to use static when in doubt (but for POD types only). https://codereview.chromium.org/2134083003/diff/1/net/http/http_util_unittest... net/http/http_util_unittest.cc:1377: for (std::string value : invalid_values) { On 2016/07/12 18:24:49, mmenke wrote: > nit: const std::string& Done. https://codereview.chromium.org/2134083003/diff/1/net/http/http_util_unittest... net/http/http_util_unittest.cc:1381: } On 2016/07/12 18:24:49, mmenke wrote: > nit: Suggest a blank line here Done.
On 2016/07/13 01:46:00, Adam Rice wrote: > On 2016/07/12 18:21:26, mmenke wrote: > > LGTM. This seems reasonable, though I'm unsure about fallout. > > My philosophy is that anything relying on consistent behaviour for CR or NL in > header values is already broken, and so this can't make it any worse. I agree - I'm more concerned about cases where we aren't sanitizing site/extension/Android WebView-set headers, than with Chrome code doing it. https://codereview.chromium.org/2134083003/diff/1/net/http/http_util_unittest.cc File net/http/http_util_unittest.cc (right): https://codereview.chromium.org/2134083003/diff/1/net/http/http_util_unittest... net/http/http_util_unittest.cc:1371: static const char* const invalid_values[] = { On 2016/07/13 01:47:00, Adam Rice wrote: > On 2016/07/12 18:24:49, mmenke wrote: > > nit: Don't use static (I think the consensus is it's a bad idea?) > > I wasn't aware of that. > > I'm never quite sure when the compiler can avoid the copy, so I have tended to > use static when in doubt (but for POD types only). I think the issue is that function-scoped statics tend to be initialized on first call, which has threading implications. Anyhow, I'm no expert on it.
On 2016/07/13 01:52:23, mmenke wrote: > On 2016/07/13 01:46:00, Adam Rice wrote: > > On 2016/07/12 18:21:26, mmenke wrote: > > > LGTM. This seems reasonable, though I'm unsure about fallout. > > > > My philosophy is that anything relying on consistent behaviour for CR or NL in > > header values is already broken, and so this can't make it any worse. > > I agree - I'm more concerned about cases where we aren't sanitizing > site/extension/Android WebView-set headers, than with Chrome code doing it. "Doing it" meaning setting bad headers.
The CQ bit was checked by ricea@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hiroshige@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2134083003/#ps20001 (title: "Changes from mmenke")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Reject line terminators in HttpUtil::IsValidHeaderValue() IsValidHeaderValue() is used for validation of untrusted input in at least two places: https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/downloads/... https://cs.chromium.org/chromium/src/content/browser/renderer_host/websocket_... Currently this method only rejects an explicit CRNL in the header value. However, some web servers treat a single CR or NL as line terminators, even when other lines in the same request use CRNL. In particular, Apache 2.4.10 treats NL this way. This means that input validated by this function can still inject new headers, at least for some servers. This CL modifies the function to reject individual CR and NL in the header value. This is still not as strict as RFC 7230, but it is sufficient to prevent the potential privilege escalation. BUG=627394 ========== to ========== Reject line terminators in HttpUtil::IsValidHeaderValue() IsValidHeaderValue() is used for validation of untrusted input in at least two places: https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/downloads/... https://cs.chromium.org/chromium/src/content/browser/renderer_host/websocket_... Currently this method only rejects an explicit CRNL in the header value. However, some web servers treat a single CR or NL as line terminators, even when other lines in the same request use CRNL. In particular, Apache 2.4.10 treats NL this way. This means that input validated by this function can still inject new headers, at least for some servers. This CL modifies the function to reject individual CR and NL in the header value. This is still not as strict as RFC 7230, but it is sufficient to prevent the potential privilege escalation. BUG=627394 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Reject line terminators in HttpUtil::IsValidHeaderValue() IsValidHeaderValue() is used for validation of untrusted input in at least two places: https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/downloads/... https://cs.chromium.org/chromium/src/content/browser/renderer_host/websocket_... Currently this method only rejects an explicit CRNL in the header value. However, some web servers treat a single CR or NL as line terminators, even when other lines in the same request use CRNL. In particular, Apache 2.4.10 treats NL this way. This means that input validated by this function can still inject new headers, at least for some servers. This CL modifies the function to reject individual CR and NL in the header value. This is still not as strict as RFC 7230, but it is sufficient to prevent the potential privilege escalation. BUG=627394 ========== to ========== Reject line terminators in HttpUtil::IsValidHeaderValue() IsValidHeaderValue() is used for validation of untrusted input in at least two places: https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/downloads/... https://cs.chromium.org/chromium/src/content/browser/renderer_host/websocket_... Currently this method only rejects an explicit CRNL in the header value. However, some web servers treat a single CR or NL as line terminators, even when other lines in the same request use CRNL. In particular, Apache 2.4.10 treats NL this way. This means that input validated by this function can still inject new headers, at least for some servers. This CL modifies the function to reject individual CR and NL in the header value. This is still not as strict as RFC 7230, but it is sufficient to prevent the potential privilege escalation. BUG=627394 Committed: https://crrev.com/b0723324a1ea7ee8b3fc71e61ed8767c045f9883 Cr-Commit-Position: refs/heads/master@{#404987} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/b0723324a1ea7ee8b3fc71e61ed8767c045f9883 Cr-Commit-Position: refs/heads/master@{#404987} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
