Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1276943003:
Re-disable support for HTTP/0.9 responses < 8 bytes over SSL. (Closed)
DescriptionRe-disable support for HTTP/0.9 responses < 8 bytes over SSL.
This allows an MITM to make the firs byte of a valid HTTP/1.x response
look like a valid HTTP/0.9 reponses, so best to be safe.
BUG=517106
Committed: https://crrev.com/4eb3411a6fadad9636dc2580bf1b316d00f621c6
Cr-Commit-Position: refs/heads/master@{#342365}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Response to comments #Messages
Total messages: 21 (9 generated)
mmenke@chromium.org changed reviewers: + davidben@chromium.org
Maybe we should add ERR_PROBABLY_NOT_A_GOOD_IDEA_TO_SUPPORT_LT_7_BYTE_HTTP_09_RESPONSES_OVER_SSL
lgtm with various comment comments. https://codereview.chromium.org/1276943003/diff/1/net/http/http_stream_parser.cc File net/http/http_stream_parser.cc (right): https://codereview.chromium.org/1276943003/diff/1/net/http/http_stream_parser... net/http/http_stream_parser.cc:787: // vulnerability, so just return an error in that case. Accepting a < 8 Maybe new paragraph after "in that case." and "Accepting" -> Note: If response_header_start_offset_ is -1, this may be a < 8 byte HTTP/0.9 response. However, accepting such a response over HTTPS would allow [...] https://codereview.chromium.org/1276943003/diff/1/net/http/http_stream_parser... net/http/http_stream_parser.cc:788: // byte response over HTTPS would allow a MITM to truncate an HTTP response (Strictly speaking, this is only possible if the peer put a record boundary at the first 8 bytes, which they would ideally do in TLS 1.0 CBC cipher suite for BEAST. Probably not worth mentioning, but for completeness.) https://codereview.chromium.org/1276943003/diff/1/net/http/http_stream_parser... net/http/http_stream_parser.cc:788: // byte response over HTTPS would allow a MITM to truncate an HTTP response Maybe HTTP response -> HTTP/1.x status line https://codereview.chromium.org/1276943003/diff/1/net/http/http_stream_parser... net/http/http_stream_parser.cc:789: // to look like a short HTTP/0.9 responses. Out of paranoia, defend against responses -> response https://codereview.chromium.org/1276943003/diff/1/net/http/http_stream_parser... net/http/http_stream_parser.cc:789: // to look like a short HTTP/0.9 responses. Out of paranoia, defend against Maybe "Out of paranoia" -> "To ensure that all response headers received over HTTPS are pristine", so it's clear exactly what we're trying to enforce. (We don't guarantee that the response body is untruncated. There is technically a way to do this, but it's not reliable in practice.)
https://codereview.chromium.org/1276943003/diff/1/net/http/http_stream_parser.cc File net/http/http_stream_parser.cc (right): https://codereview.chromium.org/1276943003/diff/1/net/http/http_stream_parser... net/http/http_stream_parser.cc:787: // vulnerability, so just return an error in that case. Accepting a < 8 On 2015/08/06 19:01:46, David Benjamin wrote: > Maybe new paragraph after "in that case." and > > "Accepting" -> > > Note: If response_header_start_offset_ is -1, this may be a < 8 byte HTTP/0.9 > response. However, accepting such a response over HTTPS would allow [...] Done. https://codereview.chromium.org/1276943003/diff/1/net/http/http_stream_parser... net/http/http_stream_parser.cc:788: // byte response over HTTPS would allow a MITM to truncate an HTTP response On 2015/08/06 19:01:46, David Benjamin wrote: > (Strictly speaking, this is only possible if the peer put a record boundary at > the first 8 bytes, which they would ideally do in TLS 1.0 CBC cipher suite for > BEAST. Probably not worth mentioning, but for completeness.) Done. https://codereview.chromium.org/1276943003/diff/1/net/http/http_stream_parser... net/http/http_stream_parser.cc:788: // byte response over HTTPS would allow a MITM to truncate an HTTP response On 2015/08/06 19:01:46, David Benjamin wrote: > Maybe HTTP response -> HTTP/1.x status line Done. https://codereview.chromium.org/1276943003/diff/1/net/http/http_stream_parser... net/http/http_stream_parser.cc:789: // to look like a short HTTP/0.9 responses. Out of paranoia, defend against On 2015/08/06 19:01:46, David Benjamin wrote: > Maybe "Out of paranoia" -> "To ensure that all response headers received over > HTTPS are pristine", so it's clear exactly what we're trying to enforce. (We > don't guarantee that the response body is untruncated. There is technically a > way to do this, but it's not reliable in practice.) Done. Though I still say concern about the bogus headers we add to an HTTP/0.9 response is paranoia (though not necessarily unjustified paranoia). :) https://codereview.chromium.org/1276943003/diff/1/net/http/http_stream_parser... net/http/http_stream_parser.cc:789: // to look like a short HTTP/0.9 responses. Out of paranoia, defend against On 2015/08/06 19:01:46, David Benjamin wrote: > responses -> response Done.
The CQ bit was checked by mmenke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from davidben@chromium.org Link to the patchset: https://codereview.chromium.org/1276943003/#ps20001 (title: "Response to comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1276943003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1276943003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by mmenke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1276943003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1276943003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by mmenke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1276943003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1276943003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by mmenke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1276943003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1276943003/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/4eb3411a6fadad9636dc2580bf1b316d00f621c6 Cr-Commit-Position: refs/heads/master@{#342365} |
