(cc-ing random people since there's no OWNER of libsrtp in Chromium)

This issue is a security regression. If you are not able to fix this quickly, please revert the change that introduced it.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

This is not a regression at all. tommi@/juberti@ can you help triage? I have no idea who would own something like this.

Adding holmer and mflodman.  I think it makes sense for webrtc to own this in general.

also adding Matt :)  - Is this something you can take a look at?

As far as I can see, the SRTP library does not use Intel AES instructions; not for AEC-GCM nor for AES-CTR. Enabling it would require a fair bit of assembly code, I think. 

Constant time algorithms would also require some assembly in order to dodge too clever compilers from optimizing the code. I'd suggest a branch-free C-code solution as an interim fix.

This doesn't affect Chromium since we use the BoringSSL crypto primitives there (and in all of our mobile clients).

GCM support just landed recently; Matt can comment on when we can start using that.

I'm not sure using BoringSSL is sufficient here. The problem is that libsrtp computes (perhaps in a constant-time fashion) the value that it expects the packet HMAC to take, and then compares the expected and actual HMAC values one byte at a time.

As far as I can tell the only place BoringSSL is used (grep OPENSSL srtp.c) is for AES-GCM crypto policies. I can build a custom version of Chromium when I get in tomorrow to check whether octet_string_is_eq is hit, but from looking at the code I think it is.

Yes, this code is hit regardless of whether we've plugged in BoringSSL. It's only skipped for AES-GCM (independent of the backing AES-GCM implementation):
https://github.com/cisco/libsrtp/blob/f97b18d7686b6e9a750e55f93dc4bc1153ddee52/srtp/srtp.c#L1949
https://github.com/cisco/libsrtp/blob/f97b18d7686b6e9a750e55f93dc4bc1153ddee52/srtp/srtp.c#L3493

That said, any attack that involves using a timing sidechannel to brute force the HMAC is significantly mitigated by the fact that SRTP does replay detection *before* checking the HMAC, and the sequence number that's used for replay detection is included in the authenticated data.

"""
   5. For message authentication and replay protection, first check if
      the packet has been replayed (Section 3.3.2), using the Replay
      List and the index as determined in Step 2.  If the packet is
      judged to be replayed, then the packet MUST be discarded, and the
      event SHOULD be logged.
"""

https://tools.ietf.org/html/rfc3711#section-3.3

As such, I think this merits "Security-Low" at best. It's definitely not "Security-High".

We should submit a fix upstream and then merge it into our local tree. As for the fix itself, I expect something modeled after CRYPTO_memcmp would be best.
https://github.com/google/boringssl/blob/19fdcb523402ed13ab798cf811fb0119e3e7b104/crypto/mem.c#L126


Oh, and just a digression, written here because it ate half an hour of my time:

WebRTC has a macro called "ENABLE_EXTERNAL_AUTH". If it's set, it appears that we install this "external_hmac" auth provider, which returns a static value for all packets:
https://cs.chromium.org/chromium/src/third_party/webrtc/pc/srtpfilter.cc?type=cs&sq=package:chromium&rcl=1468509561&l=729
https://cs.chromium.org/chromium/src/third_party/webrtc/pc/externalhmac.h
https://cs.chromium.org/chromium/src/third_party/webrtc/pc/externalhmac.cc

Then computes its own HMAC-SHA1:
https://cs.chromium.org/chromium/src/third_party/webrtc/media/base/rtputils.cc?dr=C&sq=package:chromium&rcl=1468509561&l=102

See Piper (internal) CL 61902071.

But this is *only on outbound*, so it doesn't mitigate this bug. It *will* cause some interesting behavior if we try to move to AES-GCM because we just summarily override the crypto policy's choice of auth. That work is happening in https://codereview.webrtc.org/1528843005/. I've left a comment there.

Agreed that this isn't high severity.

Agree that this is low-severity and with the proposed fix (can we just call BoringSSL's crypto_memcmp?) No idea why sheriffbot thinks this is ReleaseBlock-Beta or Pri-1?

Nice writeup on ENABLE_EXTERNAL_AUTH, I just looked at it a couple of weeks ago and got confused. Moving to GCM will make this much nicer and leave the crypto to the crypto library where it belongs :)

Sheriffbot marked it appropriately for a high severity regression, but as we've decided since then it wasn't triaged properly. I'm assuming that this has been around for a while.

Moving to GCM may be contingent on deleting the ENABLE_EXTERNAL_AUTH code. Filed https://bugs.chromium.org/p/chromium/issues/detail?id=628400.

proposed: https://github.com/cisco/libsrtp/pull/192

We can't delete the ENABLE_EXTERNAL_AUTH code until we sunset receive-side BWE (I believe). We recompute the HMAC in the browser process so that we can ensure the packet timestamp is as accurate as possible. Happy to explain more if this is unclear.

But yes, this will not play well with GCM.

I believe socket-side packet timestamping applies equally to send-side BWE.

No, this shouldn't be an issue with send-side BWE since the timestamp doesn't have to be transmitted to the receiver. Instead we currently transmit a packet id with each packet, which is only added once and never updated.

libsrtp in Chromium is now up-to-date with master and includes this fix.

See octet_string_is_eq here:
https://cs.chromium.org/chromium/src/third_party/libsrtp/crypto/math/datatypes.c

The discussion about ENABLE_EXTERNAL_AUTH will continue in https://bugs.chromium.org/p/chromium/issues/detail?id=628400.

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot