Fix incorrectly masked seq number for GCM IV.

Fix incorrectly masked seq number for GCM IV.

During the IV calculation, the sequence number has it's endianess
reversed before clearing the most significant bit, bit 31.

On little endian platforms, this results in the incorrect clearing
of bit 7. The fix is to clear the most significant bit prior to
swapping the byte order.

Please note that this change results in incompatibility with libsrtp
releases without this change.

This is a backport of https://github.com/cisco/libsrtp/pull/259 and
https://github.com/cisco/libsrtp/pull/262

BUG=webrtc:7288
TESTED=Ran all tests using:
gn gen out/libsrtp --args=build_libsrtp_tests=true
ninja -C out/libsrtp srtp_tests
out/libsrtp/srtp_tests/run_all_tests.sh

Committed: https://chromium.googlesource.com/chromium/deps/libsrtp.git/+/d15364a801e42d164486b6c8522bde271b24761d

[joachim, 2017-03-01 17:49:07]

jbauch@webrtc.org changed reviewers:
+ juberti@google.com

[joachim, 2017-03-01 17:49:07]

Ptal, this is the backport for the issue you sent.

[juberti1, 2017-03-01 17:55:54]

Description was changed from

==========
Fix incorrectly masked seq number for GCM IV.

During the IV calculation, the sequence number has it's endianess
reversed before clearing the most significant bit, bit 31.

On little endian platforms, this results in the incorrect clearing
of bit 7. The fix is to clear the most significant bit prior to
swapping the byte order.

Please note that this change results in incompatibility with libsrtp
releases without this change.

This is a backport of https://github.com/cisco/libsrtp/pull/259

BUG=None
==========

to

==========
Fix incorrectly masked seq number for GCM IV.

During the IV calculation, the sequence number has it's endianess
reversed before clearing the most significant bit, bit 31.

On little endian platforms, this results in the incorrect clearing
of bit 7. The fix is to clear the most significant bit prior to
swapping the byte order.

Please note that this change results in incompatibility with libsrtp
releases without this change.

This is a backport of https://github.com/cisco/libsrtp/pull/259

BUG=None
==========

[juberti1, 2017-03-01 17:55:54]

juberti@webrtc.org changed reviewers:
+ pthatcher@chromium.org

[juberti1, 2017-03-01 17:56:55]

On 2017/03/01 17:49:07, joachim wrote:
> Ptal, this is the backport for the issue you sent.

What's the impact to existing users? I kind of forget the state of play here - I
don't think we've enabled this for Chromium, but is this available by default to
standalone webrtc users?

[joachim, 2017-03-01 19:38:35]

On 2017/03/01 17:56:55, juberti1 wrote:
> On 2017/03/01 17:49:07, joachim wrote:
> > Ptal, this is the backport for the issue you sent.
> 
> What's the impact to existing users? I kind of forget the state of play here -
I
> don't think we've enabled this for Chromium, but is this available by default
to
> standalone webrtc users?

GCM is not available for Chromium, but it is available for standalone WebRTC
(not by default, must be explicitly enabled through the PeerConnectionFactory to
be used).

[joachim, 2017-03-01 23:30:26]

Description was changed from

==========
Fix incorrectly masked seq number for GCM IV.

During the IV calculation, the sequence number has it's endianess
reversed before clearing the most significant bit, bit 31.

On little endian platforms, this results in the incorrect clearing
of bit 7. The fix is to clear the most significant bit prior to
swapping the byte order.

Please note that this change results in incompatibility with libsrtp
releases without this change.

This is a backport of https://github.com/cisco/libsrtp/pull/259

BUG=None
==========

to

==========
Fix incorrectly masked seq number for GCM IV.

During the IV calculation, the sequence number has it's endianess
reversed before clearing the most significant bit, bit 31.

On little endian platforms, this results in the incorrect clearing
of bit 7. The fix is to clear the most significant bit prior to
swapping the byte order.

Please note that this change results in incompatibility with libsrtp
releases without this change.

This is a backport of https://github.com/cisco/libsrtp/pull/259

BUG=webrtc:5222
==========

[pthatcher2, 2017-03-01 23:40:25]

I see that the PR has not been merged.  Are we confident that it's correct?  It
seems to be to me, but I'm concerned that there seem to be not tests updated in
that PR, meaning that there is probably little verification.

[juberti1, 2017-03-02 06:33:57]

On 2017/03/01 19:38:35, joachim wrote:
> On 2017/03/01 17:56:55, juberti1 wrote:
> > On 2017/03/01 17:49:07, joachim wrote:
> > > Ptal, this is the backport for the issue you sent.
> > 
> > What's the impact to existing users? I kind of forget the state of play here
-
> I
> > don't think we've enabled this for Chromium, but is this available by
default
> to
> > standalone webrtc users?
> 
> GCM is not available for Chromium, but it is available for standalone WebRTC
> (not by default, must be explicitly enabled through the PeerConnectionFactory
to
> be used).

Well, it's a good thing we took our time on this :)

Peter, let's be sure to PSA discuss-webrtc to avoid any unpleasant surprises.

[joachim, 2017-03-02 23:11:29]

Description was changed from

==========
Fix incorrectly masked seq number for GCM IV.

During the IV calculation, the sequence number has it's endianess
reversed before clearing the most significant bit, bit 31.

On little endian platforms, this results in the incorrect clearing
of bit 7. The fix is to clear the most significant bit prior to
swapping the byte order.

Please note that this change results in incompatibility with libsrtp
releases without this change.

This is a backport of https://github.com/cisco/libsrtp/pull/259

BUG=webrtc:5222
==========

to

==========
Fix incorrectly masked seq number for GCM IV.

During the IV calculation, the sequence number has it's endianess
reversed before clearing the most significant bit, bit 31.

On little endian platforms, this results in the incorrect clearing
of bit 7. The fix is to clear the most significant bit prior to
swapping the byte order.

Please note that this change results in incompatibility with libsrtp
releases without this change.

This is a backport of https://github.com/cisco/libsrtp/pull/259

BUG=webrtc:7288
==========

[pthatcher1, 2017-03-03 06:50:56]

On 2017/03/02 06:33:57, juberti1 wrote:
> On 2017/03/01 19:38:35, joachim wrote:
> > On 2017/03/01 17:56:55, juberti1 wrote:
> > > On 2017/03/01 17:49:07, joachim wrote:
> > > > Ptal, this is the backport for the issue you sent.
> > > 
> > > What's the impact to existing users? I kind of forget the state of play
here
> -
> > I
> > > don't think we've enabled this for Chromium, but is this available by
> default
> > to
> > > standalone webrtc users?
> > 
> > GCM is not available for Chromium, but it is available for standalone WebRTC
> > (not by default, must be explicitly enabled through the
PeerConnectionFactory
> to
> > be used).
> 
> Well, it's a good thing we took our time on this :)
> 
> Peter, let's be sure to PSA discuss-webrtc to avoid any unpleasant surprises.

Is this the copy of libsrtp that standalone WebRTC uses?    

And should we PSA before or after the fix goes in?

By the way, there were some changes to the PR (somewhat my fault; sorry about
that; but I think it was for the better).  Should we wait until the PR is
merged?

[joachim, 2017-03-03 07:29:29]

On 2017/03/03 06:50:56, pthatcher1 wrote:
> On 2017/03/02 06:33:57, juberti1 wrote:
> > On 2017/03/01 19:38:35, joachim wrote:
> > > On 2017/03/01 17:56:55, juberti1 wrote:
> > > > On 2017/03/01 17:49:07, joachim wrote:
> > > > > Ptal, this is the backport for the issue you sent.
> > > > 
> > > > What's the impact to existing users? I kind of forget the state of play
> here
> > -
> > > I
> > > > don't think we've enabled this for Chromium, but is this available by
> > default
> > > to
> > > > standalone webrtc users?
> > > 
> > > GCM is not available for Chromium, but it is available for standalone
WebRTC
> > > (not by default, must be explicitly enabled through the
> PeerConnectionFactory
> > to
> > > be used).
> > 
> > Well, it's a good thing we took our time on this :)
> > 
> > Peter, let's be sure to PSA discuss-webrtc to avoid any unpleasant
surprises.
> 
> Is this the copy of libsrtp that standalone WebRTC uses?    

Yes, this is used by Chromium and rolls into WebRTC together with the Chromium
rolls.

> And should we PSA before or after the fix goes in?

I don't know your rule on sending out PSAs, so I'll let you decide.

> By the way, there were some changes to the PR (somewhat my fault; sorry about
> that; but I think it was for the better).  Should we wait until the PR is
> merged?

We should probably wait until it is merged, but from what I can see, the
additional changes are a separate PR (#262), so we could also change them in a
separate CL here.

[pthatcher1, 2017-03-10 23:55:57]

On 2017/03/03 07:29:29, joachim wrote:
> On 2017/03/03 06:50:56, pthatcher1 wrote:
> > On 2017/03/02 06:33:57, juberti1 wrote:
> > > On 2017/03/01 19:38:35, joachim wrote:
> > > > On 2017/03/01 17:56:55, juberti1 wrote:
> > > > > On 2017/03/01 17:49:07, joachim wrote:
> > > > > > Ptal, this is the backport for the issue you sent.
> > > > > 
> > > > > What's the impact to existing users? I kind of forget the state of
play
> > here
> > > -
> > > > I
> > > > > don't think we've enabled this for Chromium, but is this available by
> > > default
> > > > to
> > > > > standalone webrtc users?
> > > > 
> > > > GCM is not available for Chromium, but it is available for standalone
> WebRTC
> > > > (not by default, must be explicitly enabled through the
> > PeerConnectionFactory
> > > to
> > > > be used).
> > > 
> > > Well, it's a good thing we took our time on this :)
> > > 
> > > Peter, let's be sure to PSA discuss-webrtc to avoid any unpleasant
> surprises.
> > 
> > Is this the copy of libsrtp that standalone WebRTC uses?    
> 
> Yes, this is used by Chromium and rolls into WebRTC together with the Chromium
> rolls.
> 
> > And should we PSA before or after the fix goes in?
> 
> I don't know your rule on sending out PSAs, so I'll let you decide.
> 
> > By the way, there were some changes to the PR (somewhat my fault; sorry
about
> > that; but I think it was for the better).  Should we wait until the PR is
> > merged?
> 
> We should probably wait until it is merged, but from what I can see, the
> additional changes are a separate PR (#262), so we could also change them in a
> separate CL here.

Since it's been merged, could you please update the CL?  I'll approve it.

[joachim, 2017-03-11 00:14:19]

Description was changed from

==========
Fix incorrectly masked seq number for GCM IV.

During the IV calculation, the sequence number has it's endianess
reversed before clearing the most significant bit, bit 31.

On little endian platforms, this results in the incorrect clearing
of bit 7. The fix is to clear the most significant bit prior to
swapping the byte order.

Please note that this change results in incompatibility with libsrtp
releases without this change.

This is a backport of https://github.com/cisco/libsrtp/pull/259

BUG=webrtc:7288
==========

to

==========
Fix incorrectly masked seq number for GCM IV.

During the IV calculation, the sequence number has it's endianess
reversed before clearing the most significant bit, bit 31.

On little endian platforms, this results in the incorrect clearing
of bit 7. The fix is to clear the most significant bit prior to
swapping the byte order.

Please note that this change results in incompatibility with libsrtp
releases without this change.

This is a backport of https://github.com/cisco/libsrtp/pull/259 and
https://github.com/cisco/libsrtp/pull/262

BUG=webrtc:7288
==========

[joachim, 2017-03-11 00:15:36]

Done, ptal.

We will also need somebody to land this as there is no CQ here, or could you do
that?

[pthatcher2, 2017-03-11 00:33:32]

On 2017/03/11 00:15:36, joachim wrote:
> Done, ptal.
> 
> We will also need somebody to land this as there is no CQ here, or could you
do
> that?

lgtm

Although I just realized that I'm not an OWNER and can't commit :).

I'll ask kjellander@.

Emad, you should probably replace matt with yourself in the OWNERS file.

[pthatcher2, 2017-03-11 00:33:55]

pthatcher@chromium.org changed reviewers:
+ kjellander@chromium.org

[kjellander_chromium, 2017-03-13 13:32:09]

Description was changed from

==========
Fix incorrectly masked seq number for GCM IV.

During the IV calculation, the sequence number has it's endianess
reversed before clearing the most significant bit, bit 31.

On little endian platforms, this results in the incorrect clearing
of bit 7. The fix is to clear the most significant bit prior to
swapping the byte order.

Please note that this change results in incompatibility with libsrtp
releases without this change.

This is a backport of https://github.com/cisco/libsrtp/pull/259 and
https://github.com/cisco/libsrtp/pull/262

BUG=webrtc:7288
==========

to

==========
Fix incorrectly masked seq number for GCM IV.

During the IV calculation, the sequence number has it's endianess
reversed before clearing the most significant bit, bit 31.

On little endian platforms, this results in the incorrect clearing
of bit 7. The fix is to clear the most significant bit prior to
swapping the byte order.

Please note that this change results in incompatibility with libsrtp
releases without this change.

This is a backport of https://github.com/cisco/libsrtp/pull/259 and
https://github.com/cisco/libsrtp/pull/262

BUG=webrtc:7288
TESTED=Ran all tests using:
gn gen out/libsrtp --args=build_libsrtp_tests=true
ninja -C out/libsrtp srtp_tests
out/libsrtp/srtp_tests/run_all_tests.sh
==========

[kjellander_chromium, 2017-03-13 13:32:26]

On 2017/03/11 00:33:32, pthatcher2 wrote:
> On 2017/03/11 00:15:36, joachim wrote:
> > Done, ptal.
> > 
> > We will also need somebody to land this as there is no CQ here, or could you
> do
> > that?
> 
> lgtm
> 
> Although I just realized that I'm not an OWNER and can't commit :).
> 
> I'll ask kjellander@.
> 
> Emad, you should probably replace matt with yourself in the OWNERS file.

[kjellander_chromium, 2017-03-13 13:34:27]

On 2017/03/13 13:32:26, kjellander_chromium wrote:
> On 2017/03/11 00:33:32, pthatcher2 wrote:
> > On 2017/03/11 00:15:36, joachim wrote:
> > > Done, ptal.
> > > 
> > > We will also need somebody to land this as there is no CQ here, or could
you
> > do
> > > that?
> > 
> > lgtm
> > 
> > Although I just realized that I'm not an OWNER and can't commit :).
> > 
> > I'll ask kjellander@.

Sorry, sent to soon. I can CQ this for you, but you still need a DEPS roll to
make the code become be used in Chromium (and WebRTC after auto-rolling our DEPS
as well).
I don't know much about this code, but I added a TESTED section to the CL
description, with the tests I ran without+with this patch applied:
gn gen out/libsrtp --args=build_libsrtp_tests=true
ninja -C out/libsrtp srtp_tests
out/libsrtp/srtp_tests/run_all_tests.sh

both before+after passes on Linux for me.

> > Emad, you should probably replace matt with yourself in the OWNERS file.

That sounds like a good idea, we should clean up mattdr.
lgtm

[joachim, 2017-03-13 13:38:29]

On 2017/03/13 13:34:27, kjellander_chromium wrote:
> On 2017/03/13 13:32:26, kjellander_chromium wrote:
> > On 2017/03/11 00:33:32, pthatcher2 wrote:
> > > On 2017/03/11 00:15:36, joachim wrote:
> > > > Done, ptal.
> > > > 
> > > > We will also need somebody to land this as there is no CQ here, or could
> you
> > > do
> > > > that?
> > > 
> > > lgtm
> > > 
> > > Although I just realized that I'm not an OWNER and can't commit :).
> > > 
> > > I'll ask kjellander@.
> 
> Sorry, sent to soon. I can CQ this for you, but you still need a DEPS roll to
> make the code become be used in Chromium (and WebRTC after auto-rolling our
DEPS
> as well).

Yes, sure. I will create the DEPS roll for Chromium once this has landed
(already did so a couple of times in the past). Can I assign it to you for
review?

> I don't know much about this code, but I added a TESTED section to the CL
> description, with the tests I ran without+with this patch applied:
> gn gen out/libsrtp --args=build_libsrtp_tests=true
> ninja -C out/libsrtp srtp_tests
> out/libsrtp/srtp_tests/run_all_tests.sh
> 
> both before+after passes on Linux for me.

Thanks for adding the TESTED section (and running the tests).

> > > Emad, you should probably replace matt with yourself in the OWNERS file.
> 
> That sounds like a good idea, we should clean up mattdr.
> lgtm

[kjellander_chromium, 2017-03-13 13:42:06]

On 2017/03/13 13:38:29, joachim wrote:
> On 2017/03/13 13:34:27, kjellander_chromium wrote:
> > On 2017/03/13 13:32:26, kjellander_chromium wrote:
> > > On 2017/03/11 00:33:32, pthatcher2 wrote:
> > > > On 2017/03/11 00:15:36, joachim wrote:
> > > > > Done, ptal.
> > > > > 
> > > > > We will also need somebody to land this as there is no CQ here, or
could
> > you
> > > > do
> > > > > that?
> > > > 
> > > > lgtm
> > > > 
> > > > Although I just realized that I'm not an OWNER and can't commit :).
> > > > 
> > > > I'll ask kjellander@.
> > 
> > Sorry, sent to soon. I can CQ this for you, but you still need a DEPS roll
to
> > make the code become be used in Chromium (and WebRTC after auto-rolling our
> DEPS
> > as well).
> 
> Yes, sure. I will create the DEPS roll for Chromium once this has landed
> (already did so a couple of times in the past). Can I assign it to you for
> review?

It was landed as
https://chromium.googlesource.com/chromium/deps/libsrtp.git/+/d15364a801e42d1...
I didn't have credentials so it couldn't update the CL. I'll close it manually
now.

> 
> > I don't know much about this code, but I added a TESTED section to the CL
> > description, with the tests I ran without+with this patch applied:
> > gn gen out/libsrtp --args=build_libsrtp_tests=true
> > ninja -C out/libsrtp srtp_tests
> > out/libsrtp/srtp_tests/run_all_tests.sh
> > 
> > both before+after passes on Linux for me.
> 
> Thanks for adding the TESTED section (and running the tests).
> 
> > > > Emad, you should probably replace matt with yourself in the OWNERS file.
> > 
> > That sounds like a good idea, we should clean up mattdr.
> > lgtm

[kjellander_chromium, 2017-03-13 13:42:17]

Description was changed from

==========
Fix incorrectly masked seq number for GCM IV.

During the IV calculation, the sequence number has it's endianess
reversed before clearing the most significant bit, bit 31.

On little endian platforms, this results in the incorrect clearing
of bit 7. The fix is to clear the most significant bit prior to
swapping the byte order.

Please note that this change results in incompatibility with libsrtp
releases without this change.

This is a backport of https://github.com/cisco/libsrtp/pull/259 and
https://github.com/cisco/libsrtp/pull/262

BUG=webrtc:7288
TESTED=Ran all tests using:
gn gen out/libsrtp --args=build_libsrtp_tests=true
ninja -C out/libsrtp srtp_tests
out/libsrtp/srtp_tests/run_all_tests.sh
==========

to

==========
Fix incorrectly masked seq number for GCM IV.

During the IV calculation, the sequence number has it's endianess
reversed before clearing the most significant bit, bit 31.

On little endian platforms, this results in the incorrect clearing
of bit 7. The fix is to clear the most significant bit prior to
swapping the byte order.

Please note that this change results in incompatibility with libsrtp
releases without this change.

This is a backport of https://github.com/cisco/libsrtp/pull/259 and
https://github.com/cisco/libsrtp/pull/262

BUG=webrtc:7288
TESTED=Ran all tests using:
gn gen out/libsrtp --args=build_libsrtp_tests=true
ninja -C out/libsrtp srtp_tests
out/libsrtp/srtp_tests/run_all_tests.sh

Committed:
https://chromium.googlesource.com/chromium/deps/libsrtp.git/+/d15364a801e42d1...
==========