Closed Bug 1278320 Opened 3 years ago Closed 3 years ago

NULL deref crash [@ nsDOMDataChannel::BufferedAmount]

Categories

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

x86_64
Unspecified
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox47 --- unaffected
firefox48 --- unaffected
firefox49 --- fixed
firefox50 --- fixed

People

(Reporter: truber, Assigned: baku)

References

(Blocks 1 open bug)

Details

(Keywords: crash, regression, testcase, Whiteboard: btpp-followup-2016-07-05)

Attachments

(3 files, 1 obsolete file)

The attached testcase crashes on mozilla-central tinderbox build 1465042225.

Backtrace:

==30031==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000048 (pc 0x7f8fab0c8edc sp 0x7ffe46d86710 bp 0x7ffe46d86730 T0)
    #0 0x7f8fab0c8edb in Lock /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dist/include/mozilla/Mutex.h:69
    #1 0x7f8fab0c8edb in operator-> /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dist/include/mozilla/Mutex.h:164
    #2 0x7f8fab0c8edb in operator-> /builds/slave/m-cen-l64-asan-000000000000000/build/src/netwerk/sctp/datachannel/DataChannel.h:386
    #3 0x7f8fab0c8edb in nsDOMDataChannel::BufferedAmount() const /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/base/nsDOMDataChannel.cpp:215
    #4 0x7f8faca3c3a0 in mozilla::dom::DataChannelBinding::get_bufferedAmount(JSContext*, JS::Handle<JSObject*>, nsDOMDataChannel*, JSJitGetterCallArgs) /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dom/bindings/DataChannelBinding.cpp:164
    #5 0x7f8facf670db in mozilla::dom::GenericBindingGetter(JSContext*, unsigned int, JS::Value*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/bindings/BindingUtils.cpp:2718
    #6 0x7f8fb2ae2bfe in CallJSNative /builds/slave/m-cen-l64-asan-000000000000000/build/src/js/src/jscntxtinlines.h:235
    #7 0x7f8fb2ae2bfe in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/slave/m-cen-l64-asan-000000000000000/build/src/js/src/vm/Interpreter.cpp:452
Attached file Testcase
Component: JavaScript Engine → DOM
Keywords: regression
I think baku knows about this code.
Flags: needinfo?(amarchesini)
Whiteboard: btpp-followup-2016-07-05
Group: dom-core-security
Flags: needinfo?(amarchesini)
Attached patch datachannel.patch (obsolete) — Splinter Review
I don't think it's a good idea to land a test for this bug: it's a security issue. But in case, adding this testcase it's less 10 seconds.
Assignee: nobody → amarchesini
Attachment #8765405 - Flags: review?(bugs)
Comment on attachment 8765405 [details] [diff] [review]
datachannel.patch

I'm not at all familiar with this code.
jesup should review this, but he is on vacation.
Perhaps Nils knows this too?
Attachment #8765405 - Flags: review?(bugs) → review?(drno)
And this doesn't look like a DOM bug.
Component: DOM → WebRTC: Networking
(In reply to Olli Pettay [:smaug] (high review load, please consider other reviewers) from comment #5) 
> I'm not at all familiar with this code.
(meaning that it is hard for me to say whether the fix it the right fix, or should something else be done)
Comment on attachment 8765405 [details] [diff] [review]
datachannel.patch

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

::: netwerk/sctp/datachannel/DataChannel.h
@@ +383,5 @@
>    // Amount of data buffered to send
>    uint32_t GetBufferedAmount()
>    {
> +    if (mStream != INVALID_STREAM) {
> +      return 0;

Sorry but this does not look correct to me. If I'm not mistaken you are now always returning zero as buffered amount, even for valid data channel (= stream - in this case), which sound wrong to me.

I don't have the time right now to test this myself. If
  if (mStream == INVALID_STREAM) {
    return 0;
fixes the problem I would r+ that. Otherwise I can take a look at this problem some time next week or so.
Attachment #8765405 - Flags: review?(drno) → review-
oh oh... right. sorry. != != ==.
Attachment #8765439 - Flags: review?(drno)
Here I use mConnection as GetReadyState does. It seems a better check.
Attachment #8765405 - Attachment is obsolete: true
Comment on attachment 8765439 [details] [diff] [review]
datachannel.patch

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

LGTM
Attachment #8765439 - Flags: review?(drno) → review+
This is a regression caused by bug 1240209 (I'm not setting the dependency fields, as I'm not sure if it is preferred to not increase attention in the original bug by linking a sec bug to it).
Keywords: regression
Comment on attachment 8765439 [details] [diff] [review]
datachannel.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

We use a nullptr value. Not easy at all.

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

No test attached. Not too easy.

Which older supported branches are affected by this flaw?

Tue May 31 23:45:17 2016. I guess, this is aurora and beta, right?

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

bug 1240209

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

Easy to make.

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

0
Attachment #8765439 - Flags: sec-approval?
Comment on attachment 8765439 [details] [diff] [review]
datachannel.patch

sec-approval+. Please make and nominate an Aurora patch also.
Attachment #8765439 - Flags: sec-approval? → sec-approval+
Comment on attachment 8765439 [details] [diff] [review]
datachannel.patch

Approval Request Comment
[Feature/regressing bug #]: bug 1240209
[User impact if declined]: FF crashes
[Describe test coverage new/current, TreeHerder]: test manually.
[Risks and why]: no risks. The patch adds a check before using a pointer.
[String/UUID change made/needed]: none
Attachment #8765439 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/7c4c782250c2
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Comment on attachment 8765439 [details] [diff] [review]
datachannel.patch

Crash fix, recent regression, let's uplift this to aurora.
Attachment #8765439 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Group: dom-core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.