Fix buffer overflow in HMAC validation of STUN messages.

Fix buffer overflow in HMAC validation of STUN messages.

Committed: https://crrev.com/1a2061076412a08f3e4da04a2b33c3795e30b954
Cr-Commit-Position: refs/heads/master@{#13215}

[katrielc1, 2016-06-16 14:15:41]

katrielc@webrtc.org changed reviewers:
+ sergeyu@chromium.org

[katrielc1, 2016-06-16 14:15:41]

I think this isn't hit in the current code path since this particular brand of
evil packet will be filtered out earlier, but the validator should work
regardless.

[Sergey Ulanov, 2016-06-17 00:52:49]

https://codereview.webrtc.org/2071873002/diff/1/webrtc/p2p/base/stun.cc
File webrtc/p2p/base/stun.cc (right):

https://codereview.webrtc.org/2071873002/diff/1/webrtc/p2p/base/stun.cc#newco...
webrtc/p2p/base/stun.cc:152: attr_length = rtc::GetBE16(&data[current_pos +
sizeof(attr_type)]);
I think here we need to verify that current_pos + 4 < size

https://codereview.webrtc.org/2071873002/diff/1/webrtc/p2p/base/stun.cc#newco...
webrtc/p2p/base/stun.cc:157: current_pos + attr_length >= size) {
Should this be "current_pos + sizeof(attr_type) + sizeof(attr_length) +
attr_lenght > size"?

[Sergey Ulanov, 2016-06-17 00:53:41]

Also, please add unittests

[katrielc1, 2016-06-17 08:21:55]

https://codereview.webrtc.org/2071873002/diff/1/webrtc/p2p/base/stun.cc
File webrtc/p2p/base/stun.cc (right):

https://codereview.webrtc.org/2071873002/diff/1/webrtc/p2p/base/stun.cc#newco...
webrtc/p2p/base/stun.cc:152: attr_length = rtc::GetBE16(&data[current_pos +
sizeof(attr_type)]);
On 2016/06/17 00:52:49, Sergey Ulanov wrote:
> I think here we need to verify that current_pos + 4 < size

I think you don't technically need to verify that, since size is guaranteed to
be a multiple of four so if there's anything to read then there's enough.

But it makes sense to add it for robustness, so will do :)

https://codereview.webrtc.org/2071873002/diff/1/webrtc/p2p/base/stun.cc#newco...
webrtc/p2p/base/stun.cc:157: current_pos + attr_length >= size) {
On 2016/06/17 00:52:49, Sergey Ulanov wrote:
> Should this be "current_pos + sizeof(attr_type) + sizeof(attr_length) +
> attr_lenght > size"?

Yes, you're right.

(I think it may be equivalent since current_pos is rounded to the nearest 4
later, but this is the right thing to check.)

[Sergey Ulanov, 2016-06-18 00:25:54]

lgtm

https://codereview.chromium.org/2071873002/diff/20001/webrtc/p2p/base/stun.cc
File webrtc/p2p/base/stun.cc (right):

https://codereview.chromium.org/2071873002/diff/20001/webrtc/p2p/base/stun.cc...
webrtc/p2p/base/stun.cc:148: while (current_pos + 4 < size) {
I think this should be <=, in case there is an attribute of length 0.

[katrielc, 2016-06-20 11:23:57]

https://codereview.webrtc.org/2071873002/diff/20001/webrtc/p2p/base/stun.cc
File webrtc/p2p/base/stun.cc (right):

https://codereview.webrtc.org/2071873002/diff/20001/webrtc/p2p/base/stun.cc#n...
webrtc/p2p/base/stun.cc:148: while (current_pos + 4 < size) {
On 2016/06/18 00:25:54, Sergey Ulanov wrote:
> I think this should be <=, in case there is an attribute of length 0.

Don't think we support any length-0 extensions at the moment, but you're right.

[katrielc, 2016-06-20 11:24:15]

The CQ bit was checked by katrielc@chromium.org

[katrielc, 2016-06-20 11:24:16]

The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org
Link to the patchset: https://codereview.webrtc.org/2071873002/#ps40001 (title:
"Allow length-0 extensions.")

[commit-bot: I haz the power, 2016-06-20 11:24:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2071873002/40001

[commit-bot: I haz the power, 2016-06-20 12:13:20]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-06-20 12:13:29]

Description was changed from

==========
Fix buffer overflow in HMAC validation of STUN messages.
==========

to

==========
Fix buffer overflow in HMAC validation of STUN messages.

Committed: https://crrev.com/1a2061076412a08f3e4da04a2b33c3795e30b954
Cr-Commit-Position: refs/heads/master@{#13215}
==========

[commit-bot: I haz the power, 2016-06-20 12:13:31]

Patchset 3 (id:??) landed as
https://crrev.com/1a2061076412a08f3e4da04a2b33c3795e30b954
Cr-Commit-Position: refs/heads/master@{#13215}