790083 - libsrtp reads past the end of a key buffer when setting up the internal prng (used in testing)

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Description

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Patch for past-end-of-key read

Found via ASAN.  Ironically, the code documents that it reads 2 bytes past the end of the buffer.

Also ironically, Google (in their Android version of libsrtp) carries a patch for this exact problem.  I have my own, but it may make more sense to take theirs if it has the same characteristics -- and I'm not sure their patch is 100% safe in all cases, though in all current cases it probably is.

We will plan to upstream this (or google's) patch.  I'm a libsrtp maintainer, so I can update the master source as well.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Google's patch from Android

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Fix over-read in srtp (m-c direct import version)

This applies on top of the patches in bug 792325 - direct import of srtp into netwerk

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Note this also 'fixes' a case where passing certain non multiple-of-16-plus-14 sized buffers might cause it to use random stack garbage for a key.  Really that's a bad case anyways, this just stops it from doing something unpredictable.

Comment 4

[Timothy B. Terriberry (:derf)]

Comment on attachment 659874 [details] [diff] [review]
Patch for past-end-of-key read

Review of attachment 659874 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/webrtc/trunk/third_party/libsrtp/srtp/crypto/cipher/aes_icm.c
@@ +174,4 @@
>    else
>      return err_status_bad_param;
>  
> +  /* 

Adds trailing whitespace.

@@ +182,5 @@
> +  v128_set_to_zero(&c->offset);
> +
> +  copy_len = key_len - base_key_len;
> +  /* force last two octets of the offset to be left zero (for srtp compatibility) */
> +  if (copy_len > 14)

By my reading of the code at the top of this function, key_len may have the values 17...30, 38, or 46, and the corresponding base_key_len has the values 16, 24, or 32, respectively. So key_len - base_key_len is never larger than 14, and this check should be unnecessary.

Harmless to leave it in, of course (unless you wanted to do branch coverage analysis some day). Unfortunately libsrtp doesn't appear to use asserts, or I'd recommend just replacing it with one of those to guard against future changes to the code.

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

ekr: waiting for a review from you on this...

Comment 7

[Eric Rescorla (:ekr)]

Comment on attachment 662570 [details] [diff] [review]
Fix over-read in srtp (m-c direct import version)

Review of attachment 662570 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/srtp/src/crypto/cipher/aes_icm.c
@@ +173,5 @@
>      base_key_len = key_len - 14;
>    else
>      return err_status_bad_param;
>  
> +  /* 

NIt: trailing whitespac.e

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3b1ee99a8e61

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3b1ee99a8e61

Should this have a test?

Comment 10

[Eric Rescorla (:ekr)]

Without this fix, any execution of libsrtp under ASAN fails. Is that enough?

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

We can't easily test ASAN issues in non-ASAN builds.  As this was an already-known safe read-past (with no security impact) unless the allocation happened to be at the end of a page followed by an unmapped page, there's really nothing to test.

Comment 12

[Eric Rescorla (:ekr)]

Comment on attachment 659874 [details] [diff] [review]
Patch for past-end-of-key read

Review of attachment 659874 [details] [diff] [review]:
-----------------------------------------------------------------

Cleaning up unfinished reviews

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

FYI, this patch has been upstreamed on the new github master for libsrtp (it's Issue 7 there)