Closed Bug 825927 Opened 7 years ago Closed 7 years ago

Audit/Clean up buffer handling in nICEr STUN code

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla21
Tracking Status
firefox20 --- disabled
firefox21 + fixed
firefox-esr17 --- unaffected
b2g18 --- unaffected

People

(Reporter: abr, Assigned: abr)

References

Details

(Keywords: sec-audit, Whiteboard: [WebRTC][nICEr][blocking-webrtc+][nICEr-upstream-needed][qa-][adv-main21-])

Attachments

(3 files, 9 obsolete files)

13.27 KB, patch
ekr
: review+
jesup
: checkin+
Details | Diff | Splinter Review
303.81 KB, patch
ekr
: review+
jesup
: checkin+
Details | Diff | Splinter Review
28.03 KB, patch
abr
: review+
Details | Diff | Splinter Review
Basically, the same task as Bug 820410, but for the ICE code rather than the STUN code.
Keywords: sec-audit
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);
    }
Whoops, I meant ehugg, not dmose...
thanks. We can fix that....
Blocks: 796463
Whiteboard: [WebRTC][nICEr][blocking-webrtc+] → [WebRTC][nICEr][blocking-webrtc+][nICEr-upstream-needed]
Attached patch Patch 0: Clean up whitespace (obsolete) — Splinter Review
Attached patch Patch 1: Fix log message formats (obsolete) — Splinter Review
Attachment #704027 - Attachment is obsolete: true
Attached patch Patch 0: Clean up whitespace (obsolete) — Splinter Review
Attachment #704034 - Attachment is obsolete: true
Pushed to try from Alder: https://tbpl.mozilla.org/?tree=Try&rev=20b0746e730b
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.
Attachment #704890 - Flags: review?(ekr)
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.
Attachment #704035 - Flags: review?(ekr)
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.
Attachment #705490 - Flags: review?(ekr)
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.
Attachment #704035 - Flags: review?(ekr)
Attachment #704035 - Attachment is obsolete: true
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.
Attachment #705587 - Flags: review?(ekr)
Argh! I canceled the wrong try. Okay, here we go one more (and hopefully final) time:

https://tbpl.mozilla.org/?tree=Try&rev=1124767b7484
Try run shows up a linking issue with Windows. I know what the problem is, and should have an updated patch later today.
Attachment #705490 - Attachment is obsolete: true
Attachment #705490 - Flags: review?(ekr)
Pushing the new patch to try for WIN32:
https://tbpl.mozilla.org/?tree=Try&rev=90a74b2027ad
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.
Attachment #706753 - Flags: review?(ekr)
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
Attachment #704890 - Attachment is obsolete: true
Attachment #704890 - Flags: review?(ekr)
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.
Attachment #706809 - Flags: review?(ekr)
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
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
Attachment #706809 - Flags: review?(ekr) → review+
Attachment #706809 - Flags: checkin?(ekr)
Attachment #705587 - Flags: review?(ekr) → review+
Attachment #705587 - Flags: checkin?(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.
Attachment #706753 - Flags: review?(ekr) → review-
Attachment #706753 - Attachment is obsolete: true
The good news is that Olli's patch *does* make this problem show up on other platforms (OSX 64-bit debug build, at least).
Attachment #707246 - Attachment is obsolete: true
Attachment #707246 - Flags: review?(ekr)
Attachment #705587 - Flags: checkin?(ekr) → checkin?(rjesup)
Attachment #706809 - Flags: checkin?(ekr) → checkin?(rjesup)
Attachment #707752 - Flags: review?(ekr)
Attachment #705587 - Flags: checkin?(rjesup) → checkin+
Attachment #706809 - Flags: checkin?(rjesup) → checkin+
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.
Attachment #707752 - Flags: review?(ekr) → review-
Attachment #707752 - Attachment is obsolete: true
Attachment #708717 - Flags: review?(ekr)
Comment on attachment 708717 [details] [diff] [review]
Patch 2: Lengths, leaks, and contracts

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

lgtm
Attachment #708717 - Flags: review?(ekr) → review+
Attachment #708717 - Attachment is obsolete: true
Attachment #708727 - Flags: review?(ekr)
Depends on: 836876
Comment on attachment 708727 [details] [diff] [review]
Patch 2: Lengths, leaks, and contracts.

Carrying forward r+ from ekr
Attachment #708727 - Flags: review?(ekr) → review+
https://hg.mozilla.org/mozilla-central/rev/dabe7736e4f9
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [WebRTC][nICEr][blocking-webrtc+][nICEr-upstream-needed] → [WebRTC][nICEr][blocking-webrtc+][nICEr-upstream-needed][qa-]
Whiteboard: [WebRTC][nICEr][blocking-webrtc+][nICEr-upstream-needed][qa-] → [WebRTC][nICEr][blocking-webrtc+][nICEr-upstream-needed][qa-][adv-main21-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.