818293 - Unchecked memcpy in nr_stun_msg_create2

Status: RESOLVED FIXED

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

Description

[Eric Rescorla (:ekr)]

The following code is very unsafe. We need to check length somewhere
or alternately truncate to the buffer length.

int
nr_stun_message_create2(nr_stun_message **msg, UCHAR *buffer, int length)
{
    int r,_status;
    nr_stun_message *m = 0;

    if ((r=nr_stun_message_create(&m)))
        ABORT(r);

    memcpy(m->buffer, buffer, length);
    m->length = length;

    *msg = m;

    _status=0;
  abort:
    return(_status);
}

Comment 1

[Eric Rescorla (:ekr)]

Note that this does not appear to be exercisable on an IPv4 network b/c STUN_MAX_MESSAGE_SIZE == 2048, so you can't actually get a record that big.

Comment 2

[Eric Rescorla (:ekr)]

Jesup points out that I am wrong about c1. There are physical networks with MTUs > 2048, so this is a real issue.

Comment 3

[Eric Rescorla (:ekr)]

*** This bug exists in nICEr. Any fix must be coordinated with resprocate.org

Comment 5

[Matt Wobensmith [:mwobensmith][:matt:]]

Slight correction - resiprocate.org

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

Randell, am I right that this is disabled in 18 and 19? (Or is this codepath excercised outside of WebRTC?)

Comment 7

[Eric Rescorla (:ekr)]

This code path should not be accessible outside webrtc.

So it's only accessible if you set the webrtc flags.

Comment 8

[Daniel Veditz [:dveditz]]

Is it possible the length is checked further up the stacks? It may be OK in practice, but there's enough paths to check and potentially more in the future that we should check it here just in case.

Comment 9

[Eric Rescorla (:ekr)]

(In reply to Daniel Veditz [:dveditz] from comment #8)
> Is it possible the length is checked further up the stacks? It may be OK in
> practice, but there's enough paths to check and potentially more in the
> future that we should check it here just in case.

It's always possible but I didn't see anywhere obvious, so I think we absolutely
should be checking it here.

Comment 10

[Adam Roach [:abr]]

Attachment: Check data length before copying to buffer

Comment 11

[Adam Roach [:abr]]

Comment on attachment 690573 [details] [diff] [review]
Check data length before copying to buffer

[Security approval request comment]
> How easily can the security issue be deduced from the patch?

It's a fairly straightforward buffer overflow, so it's pretty easy to see the issue.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Nothing more than language indicating that a buffer overrun may exist.

> Which older supported branches are affected by this flaw?

FF 18 and 19 contain this code. However, this code is only executed as part of WebRTC handling. WebRTC is preffed off by default in these builds, so exposure in 18 and 19 is very limited.

> If not all supported branches, which bug introduced the flaw?

https://bugzilla.mozilla.org/show_bug.cgi?id=790517

The relevant code is part of changeset b5246d1cd364

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

The source file in question does not appear to have been changed since its introduction. The patch should apply cleanly to all branches.

> How likely is this patch to cause regressions; how much testing does it need?

The fix is a simple length check & return. The prospect that this fix could cause any regressions is vanishingly small.

Comment 12

[Al Billings [:abillings - ex-MoCo]]

sec-approval+ for trunk.

We should talk with release management about whether we want to take this for 18 and 19.

Comment 13

[Al Billings [:abillings - ex-MoCo]]

Adding Alex from Release Management.

Comment 14

[Alex Keybl [:akeybl]]

Given where we are in the cycle, we'll avoid unnecessary change for pref'd off features.

Comment 15

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/7963fe83e7a2

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7963fe83e7a2

Comment 17

[Adam Roach [:abr]]

Landed on resiprocate trunk r9911: http://svn.resiprocate.org/viewsvn/resiprocate?diff_format=l&view=revision&revision=9911