Closed Bug 1008792 Opened 8 years ago Closed 7 years ago

Null pointer deref on out of memory in nr_socket_buffered_stun_create

Categories

(Core :: WebRTC: Networking, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed
backlog parking-lot

People

(Reporter: ekr, Assigned: mjf)

Details

Attachments

(1 file)

if (!(sock = RCALLOC(sizeof(nr_socket_buffered_stun))))
    ABORT(R_NO_MEMORY);

  sock->inner = inner;

...

  if (_status) {
    void *sock_v = sock;
    sock->inner = 0;  /* Give up ownership so we don't destroy */
    nr_socket_buffered_stun_destroy(&sock_v);
  }

We need if (sock)  around sock->inner
Credit to Sylvestre Ledru for static analysis that caught this.
This is a potential null-deref in OOM, that doesn't appear to be a sec issue to me.
Flags: needinfo?(abillings)
Group: core-security
Flags: needinfo?(abillings)
backlog: --- → parking-lot
Bug 1008792: Check for valid pointer before using.
Attachment #8655089 - Flags: review?(docfaraday)
Comment on attachment 8655089 [details]
MozReview Request: Bug 1008792: Check for valid pointer before using.

https://reviewboard.mozilla.org/r/17815/#review15907
Attachment #8655089 - Flags: review?(docfaraday) → review+
Assignee: nobody → mfroman
Keywords: checkin-needed
https://reviewboard.mozilla.org/r/17815/#review16021

::: media/mtransport/third_party/nICEr/src/stun/nr_socket_buffered_stun.c:168
(Diff revision 1)
>    if (_status) {
>      void *sock_v = sock;
> +    if (sock) {
> -    sock->inner = 0;  /* Give up ownership so we don't destroy */
> +      sock->inner = 0;  /* Give up ownership so we don't destroy */
> +    }
>      nr_socket_buffered_stun_destroy(&sock_v);
>    }

This is an odd change. Why are you calling nr_socket_buffered_stun_destroy() on a pointer you know points to NULL? It's safe, but silly.

You should put the entire destructor clause inside the if (sock).
removed checkin-needed because of c6
Flags: needinfo?(mfroman)
Keywords: checkin-needed
Attachment #8655089 - Flags: review+ → review?(docfaraday)
Comment on attachment 8655089 [details]
MozReview Request: Bug 1008792: Check for valid pointer before using.

Bug 1008792: Check for valid pointer before using.
Comment on attachment 8655089 [details]
MozReview Request: Bug 1008792: Check for valid pointer before using.

https://reviewboard.mozilla.org/r/17815/#review16033
Attachment #8655089 - Flags: review?(docfaraday) → review+
Flags: needinfo?(mfroman)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7b150362b0f4
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.