825927 - Audit/Clean up buffer handling in nICEr STUN code

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect, P3)

Description

[Adam Roach [:abr]]

Basically, the same task as Bug 820410, but for the ICE code rather than the STUN code.

Comment 1

[Adam Roach [:abr]]

Comment from dmose:

just ran into a log message in nicer with a %s and no param - 

ice_component.c:234

    cand=TAILQ_FIRST(&component->candidates);
    if(!cand){
      r_log(LOG_ICE,LOG_DEBUG,"ICE(%s): couldn't create any valid candidates");
      ABORT(R_NOT_FOUND);
    }

Comment 2

[Adam Roach [:abr]]

Whoops, I meant ehugg, not dmose...

Comment 3

[Eric Rescorla (:ekr)]

thanks. We can fix that....

Comment 4

[Adam Roach [:abr]]

Attachment: , patch 1: Fix log message formats

Comment 5

[Adam Roach [:abr]]

Attachment: Patch 0: Clean up whitespace

Comment 6

[Adam Roach [:abr]]

Attachment: Patch 1: Fix log message formats

Comment 7

[Adam Roach [:abr]]

Attachment: Patch 0: Clean up whitespace

Comment 8

[Adam Roach [:abr]]

Attachment: Patch 2: Lengths, leaks, and contracts

Comment 9

[Adam Roach [:abr]]

Pushed to try from Alder: https://tbpl.mozilla.org/?tree=Try&rev=20b0746e730b

Comment 10

[Adam Roach [:abr]]

Comment on attachment 704890 [details] [diff] [review]
Patch 0: Clean up whitespace

Patch 0 really just needs a rubberstamp review -- there are literally no changes other than whitespace. The vast majority of the changes are trimming trailing whitespace. There are a small number of changes that entail replacing tab characters with an appropriate number of spaces.

This patch makes it easier to see the actual file changes in the other two patches.

Comment 11

[Adam Roach [:abr]]

Comment on attachment 704035 [details] [diff] [review]
Patch 1: Fix log message formats

Patch 1 is the result of temporarily substituting printf for r_log and then correcting all the resulting compiler warnings. The issues can be broken down roughly into three categories: (1) format specifier type mismatches; (2) format specifier cardinality mismatches (too few or too many parameters); and (3) r_log statements where the third parameter was an integer result code (valid range = 1 to 15) rather than a formatting string.

Warnings of type (1) are relatively innocuous. Warnings of type (2) can result in uninitialized memory reads (and potentially process crashes), but are otherwise unexploitable. The log statements that resulted in warnings of type (3) are guaranteed to result in segfaults (save on the most esoteric architectures), but should not otherwise be exploitable.

Comment 12

[Adam Roach [:abr]]

Comment on attachment 705490 [details] [diff] [review]
Patch 2: Lengths, leaks, and contracts

Patch 3 is the result of a hand-check for general buffer handling. Its changes fall roughly into the following categories: (1) buffer leak cleanup in exceptional circumstances; (2) additional run-time contract enforcement via conditionals and/or asserts; (3) adding compile-time contract enforcement for C11 compilers where applicable (using _Static_assert); (4) replacing UNIMPLEMENTED with assert()/ABORT(); (5) replacing inet_ntoa() and addr2ascii() with inet_ntop() for thread safety and improved buffer handling, respectively.

With the exception of some rather weak memory exhaustion attacks leveraging the various leaks (at least one of which is trivial to trigger with carefully selected input), none of these issues have any obvious exploits.

Comment 13

[Adam Roach [:abr]]

Comment on attachment 704035 [details] [diff] [review]
Patch 1: Fix log message formats

Need to fix a couple of problems that the try push turned up.

Comment 14

[Adam Roach [:abr]]

Attachment: Patch 1: Fix log message formats

Comment 15

[Adam Roach [:abr]]

Comment on attachment 705587 [details] [diff] [review]
Patch 1: Fix log message formats

Push to try showed some errors in the handling for patch 1. Patch is now revised, and here's a new try: https://tbpl.mozilla.org/?tree=Try&rev=5e03eff715f0

Note that the class (3) errors from comment 11 were due to a misreading on my part, and were not actual problems with the code. This patch undoes the changes that resulted from my confusion. It also solves a problem with 4-byte integer formatting in util.c in a less fragile way than the previous patch.

Comment 16

[Adam Roach [:abr]]

Argh! I canceled the wrong try. Okay, here we go one more (and hopefully final) time:

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

Comment 17

[Adam Roach [:abr]]

Try run shows up a linking issue with Windows. I know what the problem is, and should have an updated patch later today.

Comment 18

[Adam Roach [:abr]]

Attachment: Patch 2: Lengths, leaks, and contracts

Comment 19

[Adam Roach [:abr]]

Pushing the new patch to try for WIN32:
https://tbpl.mozilla.org/?tree=Try&rev=90a74b2027ad

Comment 20

[Adam Roach [:abr]]

Comment on attachment 706753 [details] [diff] [review]
Patch 2: Lengths, leaks, and contracts

The only difference between this patch and the previous one is the addition of a Windows-specific, POSIX-compliant (as far as is possible) inet_ntop function in util.c and util.h.

Comment 21

[Adam Roach [:abr]]

The try had some unexplained failures (the ones on XP look like they belong with a known bug, but the NT6.1 builds had some process problems). I've retriggered the failing tests on the try above. In the meantime, I'm trying with just patch 0 and patch 1. Even if we need to do more work on patch 2, I'd like to get the other ones landed before they start to rot.

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

Comment 22

[Adam Roach [:abr]]

Attachment: Patch 0: Clean up whitespace

Comment 23

[Adam Roach [:abr]]

Comment on attachment 706809 [details] [diff] [review]
Patch 0: Clean up whitespace

Speaking of bitrot, I've reuploaded the whitespace patch so it applies against m-i tip.

Comment 24

[Adam Roach [:abr]]

Okay, I'm still getting the hang of this push stuff. Apparently, my previous push was without any of my patches. The good news is that it shows some of the same failures, so I think it's safe to ignore those.

Anyway, here's a windows try with patches 0 and 1 (but not 2) applied:

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

Comment 25

[Adam Roach [:abr]]

Hrm. That try turned up some implausible failures that are, I think, related to Bug 513558, although the exact mismatches don't line up perfectly with that bug.

Once more, with feeling (patches 0 and 1 only): https://tbpl.mozilla.org/?tree=Try&rev=fc4fdc9152c7

Comment 26

[Eric Rescorla (:ekr)]

Comment on attachment 706753 [details] [diff] [review]
Patch 2: Lengths, leaks, and contracts

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

This seems generally sane, but there's enough stuff I'd like changed that I am r-ing it.

::: media/mtransport/third_party/nICEr/src/ice/ice_candidate.c
@@ +93,5 @@
>      cand->component=comp;
>      cand->stream=comp->stream;
>  
>      r_log(LOG_ICE,LOG_DEBUG,"ICE(%s): creating candidate %s with type %s",
> +      ctx->label,label,ctype<CTYPE_MAX?nr_ice_candidate_type_names[ctype]:"ERROR");

I would prefer if we are going to have a check to make a STATE_NAME macro to avoid code reuse.

@@ +122,5 @@
>      *candp=cand;
>  
>      _status=0;
>    abort:
> +    if (_status){

Shouldn't this be nr_ice_cnadidate_destroy

::: media/mtransport/third_party/nICEr/src/ice/ice_candidate_pair.c
@@ +151,5 @@
>    abort:
>      RFREE(l2ruser);
>      if(_status){
>        RFREE(r2lpass);
> +      RFREE(pair);

shouldn't this be nr_ice_candidate_pair_destroy

::: media/mtransport/third_party/nICEr/src/ice/ice_ctx.c
@@ +467,5 @@
>      *attrsp=attrs;
>  
>      _status=0;
>    abort:
> +    if (_status)

Let's brace this/

::: media/mtransport/third_party/nICEr/src/ice/ice_parser.c
@@ +104,5 @@
>      memcpy(tmp, *str, len);
>      tmp[len] = '\0';
>  
>      *str = c;
> +    RFREE(*out);

I'm not convinced this is correct. *out being valid does not appear to be part of the interface guarantee.

::: media/mtransport/third_party/nICEr/src/net/transport_addr.c
@@ +66,5 @@
>          break;
> +      case NR_IPV6:
> +        if (!inet_ntop(AF_INET6, &addr->u.addr6.sin6_addr,buffer,sizeof(buffer)))
> +          strcpy(buffer, "[error]");
> +        snprintf(addr->as_string,sizeof(addr->as_string),"IP6:%s:%d",buffer,ntohs(addr->u.addr4.sin_port));

shouldn't this be addr6.sin_port?

@@ +185,5 @@
>  
> +    if(!res){
> +      if (errno == ENOSPC)
> +        ABORT(R_BAD_ARGS);
> +      else

No need for this else.

::: media/mtransport/third_party/nrappkit/src/util/util.c
@@ +529,5 @@
> +
> +    [EAFNOSUPPORT]
> +        The af argument is invalid.
> +    [ENOSPC]
> +        The size of the result buffer is inadequate.

Where did this text come from?

@@ +533,5 @@
> +        The size of the result buffer is inadequate.
> +
> +*/
> +const char *inet_ntop(int af, const void *src, char *dst, size_t size)
> +  {

I would suggest potentially taking one of the BSD (and hence BSD-licensed) implementations rather than writing this from scratch.

Comment 27

[Adam Roach [:abr]]

Attachment: Patch 2: Lengths, leaks, and contracts

Comment 28

[Adam Roach [:abr]]

Comment on attachment 707246 [details] [diff] [review]
Patch 2: Lengths, leaks, and contracts

Interdiff: https://bugzilla.mozilla.org/attachment.cgi?oldid=706753&action=interdiff&newid=707246&headers=1

Windows try: https://tbpl.mozilla.org/?tree=Try&rev=b7873c31c20e

Comment 29

[Adam Roach [:abr]]

The good news is that Olli's patch *does* make this problem show up on other platforms (OSX 64-bit debug build, at least).

Comment 30

[Adam Roach [:abr]]

Attachment: Patch 2: Lengths, leaks, and contracts

Comment 31

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/01b9474f522c
https://hg.mozilla.org/integration/mozilla-inbound/rev/e228c4f5d681

Comment 32

[Eric Rescorla (:ekr)]

Comment on attachment 707752 [details] [diff] [review]
Patch 2: Lengths, leaks, and contracts

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

r- because of the IPv6 length issue below.

::: media/mtransport/third_party/nICEr/src/ice/ice_candidate.c
@@ +96,5 @@
>  
>      r_log(LOG_ICE,LOG_DEBUG,"ICE(%s): creating candidate %s with type %s",
> +      ctx->label,label,CTYPE_NAME(ctype));
> +
> +    assert(ctype<CTYPE_MAX);

Why not put the assert in CTYPE_NAME? Make it a fxn if you have to?

::: media/mtransport/third_party/nICEr/src/ice/ice_candidate_pair.c
@@ +143,5 @@
>        ABORT(r);
>      if(!(r2lpass=r_strdup(lpwd)))
>        ABORT(R_NO_MEMORY);
>      INIT_DATA(pair->r2l_pwd,(UCHAR *)r2lpass,strlen(r2lpass));
> +    r2lpass=NULL;

Convention in nICEr is =0; also add comment, e.g., "Give up ownership of r2lpass..

::: media/mtransport/third_party/nICEr/src/ice/ice_ctx.c
@@ +468,5 @@
>  
>      _status=0;
>    abort:
> +    if (_status){
> +      RFREE(attrs);

Aren't you still potentially leaking attrs[0]?

::: media/mtransport/third_party/nICEr/src/net/transport_addr.c
@@ +66,5 @@
>          break;
> +      case NR_IPV6:
> +        if (!inet_ntop(AF_INET6, &addr->u.addr6.sin6_addr,buffer,sizeof(buffer)))
> +          strcpy(buffer, "[error]");
> +        snprintf(addr->as_string,sizeof(addr->as_string),"IP6:%s:%d",buffer,ntohs(addr->u.addr6.sin6_port));

Doesn't addr->s_string() need to get longer? If we are going to have a buffer of 39 and then copy with a header into a 40-length buffer.

::: media/mtransport/third_party/nrappkit/src/util/util.c
@@ +504,5 @@
>  }
>  
>  #endif  /* LINUX or WIN32 */
>  
> +#if defined(USE_OWN_INET_NTOP) || defined(WIN32)

Note: I have not reviewed this code.

Comment 33

[Adam Roach [:abr]]

Attachment: Patch 2: Lengths, leaks, and contracts

Comment 34

[Eric Rescorla (:ekr)]

Comment on attachment 708717 [details] [diff] [review]
Patch 2: Lengths, leaks, and contracts

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

lgtm

Comment 35

[Adam Roach [:abr]]

Attachment: Patch 2: Lengths, leaks, and contracts.

Contains ISC-licensed code sourced from:
http://ftp.netbsd.org/pub/NetBSD/NetBSD-release-6/src/lib/libc/inet/inet_ntop.c

Comment 36

[Adam Roach [:abr]]

Comment on attachment 708727 [details] [diff] [review]
Patch 2: Lengths, leaks, and contracts.

Carrying forward r+ from ekr

Comment 37

[Ryan VanderMeulen [:RyanVM]]

(In reply to Randell Jesup [:jesup] from comment #31)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/01b9474f522c
> https://hg.mozilla.org/integration/mozilla-inbound/rev/e228c4f5d681

https://hg.mozilla.org/mozilla-central/rev/e228c4f5d681
https://hg.mozilla.org/mozilla-central/rev/01b9474f522c

Comment 38

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/dabe7736e4f9

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dabe7736e4f9