Closed Bug 818293 Opened 7 years ago Closed 7 years ago

Unchecked memcpy in nr_stun_msg_create2

Categories

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

x86
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla20
Tracking Status
firefox17 --- unaffected
firefox18 - disabled
firefox19 - disabled
firefox20 + fixed
firefox-esr10 --- unaffected
firefox-esr17 --- unaffected

People

(Reporter: ekr, Assigned: abr)

References

Details

(Keywords: sec-critical, Whiteboard: [WebRTC] [blocking-webrtc+] [nICEr][qa-][adv-main20-])

Attachments

(1 file)

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);
}
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.
Jesup points out that I am wrong about c1. There are physical networks with MTUs > 2048, so this is a real issue.
*** This bug exists in nICEr. Any fix must be coordinated with resprocate.org
Duplicate of this bug: 818292
Priority: -- → P1
Whiteboard: [WebRTC] [blocking-webrtc+]
Slight correction - resiprocate.org
Randell, am I right that this is disabled in 18 and 19? (Or is this codepath excercised outside of WebRTC?)
This code path should not be accessible outside webrtc.

So it's only accessible if you set the webrtc flags.
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.
(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.
Assignee: nobody → adam
Attachment #690573 - Flags: review?(ekr)
Status: NEW → ASSIGNED
Attachment #690573 - Flags: review?(ekr) → review?(rjesup)
Whiteboard: [WebRTC] [blocking-webrtc+] → [WebRTC] [blocking-webrtc+] [nICEr]
Attachment #690573 - Flags: review?(rjesup) → review+
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.
Attachment #690573 - Flags: sec-approval?
Attachment #690573 - Flags: sec-approval? → sec-approval+
sec-approval+ for trunk.

We should talk with release management about whether we want to take this for 18 and 19.
Adding Alex from Release Management.
Given where we are in the cycle, we'll avoid unnecessary change for pref'd off features.
Attachment #690573 - Flags: checkin?(rjesup)
Attachment #690573 - Flags: checkin?(rjesup) → checkin+
https://hg.mozilla.org/mozilla-central/rev/7963fe83e7a2
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [WebRTC] [blocking-webrtc+] [nICEr] → [WebRTC] [blocking-webrtc+] [nICEr][qa-]
Whiteboard: [WebRTC] [blocking-webrtc+] [nICEr][qa-] → [WebRTC] [blocking-webrtc+] [nICEr][qa-][adv-main20-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.