|
|
DescriptionFix boundary check problems in socket_host.cc.
See the bug for the full list of security issues fixed.
BUG=416528
Committed: https://crrev.com/08f0eac49f32b8d24c82bd5cc884938eb494f85e
Cr-Commit-Position: refs/heads/master@{#296287}
Patch Set 1 #
Total comments: 28
Patch Set 2 : address palmer and aedla's comments #
Total comments: 2
Patch Set 3 : for justin's #
Total comments: 4
Patch Set 4 : #
Total comments: 20
Patch Set 5 : rename and nits #
Total comments: 3
Patch Set 6 : nit #Patch Set 7 : #
Messages
Total messages: 27 (6 generated)
jiayl@chromium.org changed reviewers: + sergeyu@chromium.org
P0 needed for M38. PTAL.
On 2014/09/22 18:32:23, jiayl wrote: > P0 needed for M38. > PTAL. Please CC me on the bug so I can access it.
palmer@chromium.org changed reviewers: + palmer@chromium.org
I am contractually obligated to each lunch now. But, this file has Integer Issues. https://codereview.chromium.org/589183002/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/589183002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/p2p/socket_host.cc:36: bool IsTurnChannelData(const char* data, int len) { size_t, or explicitly-sized unsigned type. http://www.chromium.org/Home/chromium-security/education/security-tips-for-ip.... https://codereview.chromium.org/589183002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/p2p/socket_host.cc:37: return len >= 1 && ((*data & 0xC0) == 0x40); Is there a maximum sane size we should check for, too? https://codereview.chromium.org/589183002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/p2p/socket_host.cc:51: if (len < 2) I'd suggest using kMinimumWhateverLength and kMaximumWhateverLength, here and in |IsTurnChannelData|. https://codereview.chromium.org/589183002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/p2p/socket_host.cc:84: if (rtp_hdr_len_without_extn + kRtpExtnHdrLen > length) { Integer overflow potential? Consider using checked_numeric<foo> from base/. https://codereview.chromium.org/589183002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/p2p/socket_host.cc:172: int auth_required_length = len - tag_length + kRocLength; Integer overflow potential?
aedla@chromium.org changed reviewers: + aedla@chromium.org
Some comments from me as well. https://codereview.chromium.org/589183002/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/589183002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/p2p/socket_host.cc:90: uint16 extn_length = rtc::GetBE16(rtp + 2) * 4; Would be nice to fix the integer overflow here. https://codereview.chromium.org/589183002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/p2p/socket_host.cc:149: char* auth_tag = rtp + (len - tag_length); Validate tag_length before we create a bad pointer, not after. https://codereview.chromium.org/589183002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/p2p/socket_host.cc:164: if (tag_length > hmac.DigestLength() || tag_length < kRocLength) { That won't cut it. Say tag_length == hmac.DigestLength() == 20 and len == 16. Then auth_tag == rtp - 4 and second memcpy will OOB write 4 bytes. Failing with (tag_length > len || tag_length < kRocLength) should be ok from a security standpoint. Not sure about semantics. I'll think about it a bit more though. https://codereview.chromium.org/589183002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/p2p/socket_host.cc:310: rtp_begin += sizeof(attr_type) + sizeof(attr_length) + attr_length; Use kAttrHeaderLength here. https://codereview.chromium.org/589183002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/p2p/socket_host.cc:318: rtp_begin += 4; // Skip STUN_DATA_ATTR header. And here. https://codereview.chromium.org/589183002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/p2p/socket_host.cc:321: if (length < rtp_length + rtp_begin) { This check is now redundant. https://codereview.chromium.org/589183002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/p2p/socket_host.cc:406: const char* extn_start = rtp; Having const char* extn_end = extn_start + extn_length; as well would simplify length checks below. https://codereview.chromium.org/589183002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/p2p/socket_host.cc:410: if (rtp + kOneByteHdrLen + len - extn_start >= extn_length) { I think this should be > extn_length. Or if (rtp + kOneByteHdrLen + len > extn_end) {
All comments addressed. PTAL. https://codereview.chromium.org/589183002/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/589183002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/p2p/socket_host.cc:36: bool IsTurnChannelData(const char* data, int len) { On 2014/09/22 19:34:46, Chromium Palmer wrote: > size_t, or explicitly-sized unsigned type. > > http://www.chromium.org/Home/chromium-security/education/security-tips-for-ip.... Done. https://codereview.chromium.org/589183002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/p2p/socket_host.cc:37: return len >= 1 && ((*data & 0xC0) == 0x40); On 2014/09/22 19:34:46, Chromium Palmer wrote: > Is there a maximum sane size we should check for, too? I don't think there is a maximum. https://codereview.chromium.org/589183002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/p2p/socket_host.cc:51: if (len < 2) On 2014/09/22 19:34:46, Chromium Palmer wrote: > I'd suggest using kMinimumWhateverLength and kMaximumWhateverLength, here and in > |IsTurnChannelData|. Used the minimum header length for these methods. https://codereview.chromium.org/589183002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/p2p/socket_host.cc:84: if (rtp_hdr_len_without_extn + kRtpExtnHdrLen > length) { On 2014/09/22 19:34:46, Chromium Palmer wrote: > Integer overflow potential? Consider using checked_numeric<foo> from base/. The max of the sum is 12 + 4 * 31 + 4 * UINT16_MAX, which should not overflow size_t. https://codereview.chromium.org/589183002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/p2p/socket_host.cc:90: uint16 extn_length = rtc::GetBE16(rtp + 2) * 4; On 2014/09/22 20:06:47, aedla wrote: > Would be nice to fix the integer overflow here. Changed the result to size_t to avoid overflow. https://codereview.chromium.org/589183002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/p2p/socket_host.cc:149: char* auth_tag = rtp + (len - tag_length); On 2014/09/22 20:06:47, aedla wrote: > Validate tag_length before we create a bad pointer, not after. Done. https://codereview.chromium.org/589183002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/p2p/socket_host.cc:164: if (tag_length > hmac.DigestLength() || tag_length < kRocLength) { On 2014/09/22 20:06:47, aedla wrote: > That won't cut it. Say tag_length == hmac.DigestLength() == 20 and len == 16. > Then auth_tag == rtp - 4 and second memcpy will OOB write 4 bytes. > > Failing with (tag_length > len || tag_length < kRocLength) should be ok from a > security standpoint. Not sure about semantics. I'll think about it a bit more > though. Done. https://codereview.chromium.org/589183002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/p2p/socket_host.cc:172: int auth_required_length = len - tag_length + kRocLength; On 2014/09/22 19:34:46, Chromium Palmer wrote: > Integer overflow potential? tag_length is guaranteed to be >= kRocLength, so no overflow risk. https://codereview.chromium.org/589183002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/p2p/socket_host.cc:310: rtp_begin += sizeof(attr_type) + sizeof(attr_length) + attr_length; On 2014/09/22 20:06:46, aedla wrote: > Use kAttrHeaderLength here. Done. https://codereview.chromium.org/589183002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/p2p/socket_host.cc:318: rtp_begin += 4; // Skip STUN_DATA_ATTR header. On 2014/09/22 20:06:47, aedla wrote: > And here. Done. https://codereview.chromium.org/589183002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/p2p/socket_host.cc:321: if (length < rtp_length + rtp_begin) { On 2014/09/22 20:06:47, aedla wrote: > This check is now redundant. Done. https://codereview.chromium.org/589183002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/p2p/socket_host.cc:406: const char* extn_start = rtp; On 2014/09/22 20:06:46, aedla wrote: > Having > const char* extn_end = extn_start + extn_length; > as well would simplify length checks below. Done. https://codereview.chromium.org/589183002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/p2p/socket_host.cc:410: if (rtp + kOneByteHdrLen + len - extn_start >= extn_length) { On 2014/09/22 20:06:48, aedla wrote: > I think this should be > extn_length. Or > if (rtp + kOneByteHdrLen + len > extn_end) { You are right. Done.
juberti@chromium.org changed reviewers: + juberti@chromium.org
https://codereview.chromium.org/589183002/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/589183002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/p2p/socket_host.cc:37: return len >= 1 && ((*data & 0xC0) == 0x40); On 2014/09/22 21:39:03, jiayl wrote: > On 2014/09/22 19:34:46, Chromium Palmer wrote: > > Is there a maximum sane size we should check for, too? > > I don't think there is a maximum. We have a max packet size of 2048 that we use elsewhere. If checking that provides some extra safety, we should probably do it when the packet is first received. https://codereview.chromium.org/589183002/diff/20001/content/browser/renderer... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/589183002/diff/20001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:45: bool IsRtcpPacket(const char* data) { for completeness, perhaps check len against 2 here (or add a min RTCP packet len == 8)
https://codereview.chromium.org/589183002/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/589183002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/p2p/socket_host.cc:37: return len >= 1 && ((*data & 0xC0) == 0x40); On 2014/09/22 22:43:56, juberti2 wrote: > On 2014/09/22 21:39:03, jiayl wrote: > > On 2014/09/22 19:34:46, Chromium Palmer wrote: > > > Is there a maximum sane size we should check for, too? > > > > I don't think there is a maximum. > > We have a max packet size of 2048 that we use elsewhere. If checking that > provides some extra safety, we should probably do it when the packet is first > received. Done. https://codereview.chromium.org/589183002/diff/20001/content/browser/renderer... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/589183002/diff/20001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:45: bool IsRtcpPacket(const char* data) { On 2014/09/22 22:43:57, juberti2 wrote: > for completeness, perhaps check len against 2 here (or add a min RTCP packet len > == 8) Done.
https://codereview.chromium.org/589183002/diff/40001/content/browser/renderer... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/589183002/diff/40001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:22: const int kMinRtpHdrLen = 12; should these all become size_ts as well? https://codereview.chromium.org/589183002/diff/40001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:29: const int kMinRtcpLen = 8; suggest kMinRtcpHdrLen, put this next to kMinRtpHdrLen above
https://codereview.chromium.org/589183002/diff/40001/content/browser/renderer... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/589183002/diff/40001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:22: const int kMinRtpHdrLen = 12; On 2014/09/22 23:07:01, juberti2 wrote: > should these all become size_ts as well? Done. https://codereview.chromium.org/589183002/diff/40001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:29: const int kMinRtcpLen = 8; On 2014/09/22 23:07:01, juberti2 wrote: > suggest kMinRtcpHdrLen, put this next to kMinRtpHdrLen above Done.
lgtm
Ping sergeyu.
Ping palmer and aedla. Do you want to take another look?
https://codereview.chromium.org/589183002/diff/60001/content/browser/renderer... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/589183002/diff/60001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:22: const size_t kMinRtpHdrLen = 12; Not related to this CL (feel free to ignore): Ths nms r unrdbl. Should be HeaderLength instead of HdrLen. https://codereview.chromium.org/589183002/diff/60001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:38: bool IsTurnChannelData(const char* data, size_t len) { nit: there is some |len| and some |length| in this file. Maybe rename them all to |length| for consistency, or at least use |length| for the parameters you add in this CL. https://codereview.chromium.org/589183002/diff/60001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:48: if (len < kMinRtcpHdrLen) nit: Add {} or remove them for all other single-line ifs in this file. https://codereview.chromium.org/589183002/diff/60001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:72: size_t cc_count = rtp[0] & 0x0F; Do you need to verify that length > 0 before reading rtp[0]? https://codereview.chromium.org/589183002/diff/60001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:106: if (header_length) nit: add {} https://codereview.chromium.org/589183002/diff/60001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:159: const size_t kRocLength = 4; What is Roc? https://codereview.chromium.org/589183002/diff/60001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:299: const int kAttrHeaderLength = sizeof(attr_type) + sizeof(attr_length); nit: it's better to write =4 instead of sum the of the sizes.
https://codereview.chromium.org/589183002/diff/60001/content/browser/renderer... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/589183002/diff/60001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:72: size_t cc_count = rtp[0] & 0x0F; > Do you need to verify that length > 0 before reading rtp[0]? Yes, and probably in |IsRtpPacket|, too. I almost wonder if rtp[0] != rtp[0] & 0x0F indicates a malformed, damaged, or even malicious packet? Ohh, I see, you need to check the 0x10 bit, too (line 80). Maybe check that no other bits are set, e.g. rtp[0] & 0xe0 == 0? I don't know... https://codereview.chromium.org/589183002/diff/60001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:82: *header_length = rtp_hdr_len_without_extn; Nit: Also a hard-to-read name. I'd go with something like |header_length_without_extension|. Yes, it's long, but in hairy code that manually parses binary formats, you want all the clarity you can get... https://codereview.chromium.org/589183002/diff/60001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:95: uint16 extn_len_32bit_words = rtc::GetBE16(rtp + 2); Nit: Again, standardize on |...length| rather than mixing |...len| and |...length|. https://codereview.chromium.org/589183002/diff/60001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:299: const int kAttrHeaderLength = sizeof(attr_type) + sizeof(attr_length); > nit: it's better to write =4 instead of sum the of the sizes. I disagree; it's nice documentation of the intent. Also, the value of sizeof(foo) is of type size_t, so |kAttrHeaderLength| should be declared as a size_t too.
PTAL! https://codereview.chromium.org/589183002/diff/60001/content/browser/renderer... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/589183002/diff/60001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:22: const size_t kMinRtpHdrLen = 12; On 2014/09/23 18:38:08, Sergey Ulanov wrote: > Not related to this CL (feel free to ignore): Ths nms r unrdbl. Should be > HeaderLength instead of HdrLen. Done. https://codereview.chromium.org/589183002/diff/60001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:38: bool IsTurnChannelData(const char* data, size_t len) { On 2014/09/23 18:38:08, Sergey Ulanov wrote: > nit: there is some |len| and some |length| in this file. Maybe rename them all > to |length| for consistency, or at least use |length| for the parameters you add > in this CL. Done. https://codereview.chromium.org/589183002/diff/60001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:48: if (len < kMinRtcpHdrLen) On 2014/09/23 18:38:08, Sergey Ulanov wrote: > nit: Add {} or remove them for all other single-line ifs in this file. Done. https://codereview.chromium.org/589183002/diff/60001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:72: size_t cc_count = rtp[0] & 0x0F; On 2014/09/23 18:57:32, Chromium Palmer wrote: > > Do you need to verify that length > 0 before reading rtp[0]? > > Yes, and probably in |IsRtpPacket|, too. > > I almost wonder if > > rtp[0] != rtp[0] & 0x0F > > indicates a malformed, damaged, or even malicious packet? > > Ohh, I see, you need to check the 0x10 bit, too (line 80). Maybe check that no > other bits are set, e.g. rtp[0] & 0xe0 == 0? I don't know... Done length check. I don't think we should check other bits. https://codereview.chromium.org/589183002/diff/60001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:82: *header_length = rtp_hdr_len_without_extn; On 2014/09/23 18:57:32, Chromium Palmer wrote: > Nit: Also a hard-to-read name. I'd go with something like > |header_length_without_extension|. Yes, it's long, but in hairy code that > manually parses binary formats, you want all the clarity you can get... Done. https://codereview.chromium.org/589183002/diff/60001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:95: uint16 extn_len_32bit_words = rtc::GetBE16(rtp + 2); On 2014/09/23 18:57:31, Chromium Palmer wrote: > Nit: Again, standardize on |...length| rather than mixing |...len| and > |...length|. Done. https://codereview.chromium.org/589183002/diff/60001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:106: if (header_length) On 2014/09/23 18:38:08, Sergey Ulanov wrote: > nit: add {} Done. https://codereview.chromium.org/589183002/diff/60001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:159: const size_t kRocLength = 4; On 2014/09/23 18:38:08, Sergey Ulanov wrote: > What is Roc? Added a comment. https://codereview.chromium.org/589183002/diff/60001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:299: const int kAttrHeaderLength = sizeof(attr_type) + sizeof(attr_length); On 2014/09/23 18:57:32, Chromium Palmer wrote: > > nit: it's better to write =4 instead of sum the of the sizes. > > I disagree; it's nice documentation of the intent. > > Also, the value of sizeof(foo) is of type size_t, so |kAttrHeaderLength| should > be declared as a size_t too. Acknowledged.
lgtm
This was a lot of work. Thank you! LGTM, but let's see if aedla has any more comments before landing it. https://codereview.chromium.org/589183002/diff/80001/content/browser/renderer... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/589183002/diff/80001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:66: if (length < kMinRtpHeaderLength) { Nit: You could use the same 1-line style here as you do in e.g. |IsTurnChannelData|.
juri.aedla@gmail.com changed reviewers: + juri.aedla@gmail.com
Looking nice, thanks. LGTM https://codereview.chromium.org/589183002/diff/80001/content/browser/renderer... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/589183002/diff/80001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:304: while (packet + rtp_begin < start + stun_message_length) { Perhaps change to simpler but equivalent (rtp_begin < length) https://codereview.chromium.org/589183002/diff/80001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:329: Perhaps do rtp_begin += kAttrHeaderLength; once and remove 3 references to kAttrHeaderLength below.
The CQ bit was checked by jiayl@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/589183002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as b5a185836c2aa532a9a6305b0280411ee95f1f4e
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/08f0eac49f32b8d24c82bd5cc884938eb494f85e Cr-Commit-Position: refs/heads/master@{#296287} |