1008789 - UMR in nr_stun_client_process_response

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Description

[Eric Rescorla (:ekr)]

Static analysis by Sylvestre Ledru found the following potential
defect.

We start with:
    char *username;
    ...
    UCHAR hmac_key_d[16];
    Data hmac_key;
    int compute_lt_key=0;

Then if the mode is NR_STUN_CLIENT_MODE_BINDING_REQUEST_LONG_TERM_AUTH,
we set compute_lt_key = 1

    case NR_STUN_CLIENT_MODE_BINDING_REQUEST_LONG_TERM_AUTH:
      compute_lt_key = 1;
      /* Fall through */
    case NR_STUN_CLIENT_MODE_BINDING_REQUEST_SHORT_TERM_AUTH:
        password = ctx->params.stun_binding_request.password;
        break;

Later:

    if (compute_lt_key) {
      if (!ctx->realm || !username) {
        r_log(NR_LOG_STUN, LOG_DEBUG, "Long-term auth required but no realm/username specified. Randomizing key");
        /* Fill the key with random bytes to guarantee non-match */
        if (r=nr_crypto_random_bytes(hmac_key_d, sizeof(hmac_key_d)))
          ABORT(r);
      }
      else {
        if (r=nr_stun_compute_lt_message_integrity_password(username, ctx->realm,
                                                            password, &hmac_key))
          ABORT(r);
      }
      password = &hmac_key;
    }

If username is 0, then we just take the first branch and we randomize
hmac_key_d, which is fine.

If username is nonzero (and hence pointing to a random stack variable),
we run nr_stun_compute_lt_message_integrity_password() over a random
zero-terminated region of memory pointed to by username and then
use that to check the MAC on the response. There is some possibility
that this will cause a check which should have failed (because username
was 0 and we randomized the password) to succeed, but more likely to be
a crash because we are reading random memory. This should not cause
memory disclosure because the MAC result is not sent back but rather
used to accept or reject the packet.

However, even this sequence of events seems unlikely because we
currently do not seem to allow long-term auth for binding requests
to be sent at all and it is this mode which controls what happens
here:

http://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/stun/stun_client_ctx.c#425

Also, it appears to never be used:
http://dxr.mozilla.org/mozilla-central/search?q=%2Bmacro-ref%3ANR_STUN_CLIENT_MODE_BINDING_REQUEST_LONG_TERM_AUTH

If it were to be used, it would presumably not work for the reason
above.

I have marked this security because it is intricate and in a security
sensitive region of code. Byron, please verify the above analysis and
if you agree, please un-mark it and file a patch to double-check this
by initializing username to 0 and putting an assert in this branch.

Comment 1

[Byron Campen [:bwc]]

The only potential I see for us to set ctx->mode to this value is if it is uninitialized somehow.

I only see one place where ctx->mode is set:

http://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/stun/stun_client_ctx.c#113

Looking at the tree for calls into here, I don't see anything obvious that would get this to take a bogus value. I think we're ok here.

Comment 2

[Byron Campen [:bwc]]

Someone else will need to unmark this. I'm currently tied up in code review, but when I get done with that I can patch this up.

Comment 3

[Byron Campen [:bwc]]

Attachment: Init username to 0, just in case.

Comment 4

[Nils Ohlmeier [:drno]]

Comment on attachment 8438658 [details] [diff] [review]
Init username to 0, just in case.

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

LGTM

Comment 5

[Byron Campen [:bwc]]

https://tbpl.mozilla.org/?tree=Try&rev=c64e61808813

Comment 6

[Byron Campen [:bwc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/43ab641c471a

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/43ab641c471a