xpcshell test netwerk/test/unit/test_MIME_params.js fails on AddressSanitizer

RESOLVED FIXED in Firefox 16

Status

()

--
critical
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: decoder, Assigned: julian.reschke)

Tracking

({sec-high, testcase})

Trunk
mozilla18
sec-high, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox16+ fixed, firefox17+ fixed, firefox18 fixed, firefox-esr1016+ fixed)

Details

(Whiteboard: [advisory-tracking+][asan][asan-test-failure][qa-])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
The xpcshell test netwerk/test/unit/test_MIME_params.js shows an AddressSanitizer error on mozilla-central revision f7cf60910637:


==60029== ERROR: AddressSanitizer heap-buffer-overflow on address 0x2b9c8cf9cac8 at pc 0x2b9c7660df1c bp 0x7fff28d1b650 sp 0x7fff28d1b648
READ of size 1 at 0x2b9c8cf9cac8 thread T0
    #0 0x2b9c7660df1c in nsMIMEHeaderParamImpl::DoParameterInternal(char const*, char const*, nsMIMEHeaderParamImpl::ParamDecoding, char**, char**, char**) mozilla-central/netwerk/mime/nsMIMEHeaderParamImpl.cpp:449
    #1 0x2b9c7660c72d in nsMIMEHeaderParamImpl::DoGetParameter(nsACString_internal const&, char const*, nsMIMEHeaderParamImpl::ParamDecoding, nsACString_internal const&, bool, char**, nsAString_internal&) mozilla-central/netwerk/mime/nsMIMEHeaderParamImpl.cpp:82
    #2 0x2b9c7660c47f in nsMIMEHeaderParamImpl::GetParameter(nsACString_internal const&, char const*, nsACString_internal const&, bool, char**, nsAString_internal&) mozilla-central/netwerk/mime/nsMIMEHeaderParamImpl.cpp:49
    #3 0x2b9c793e7490 in NS_InvokeByIndex_P mozilla-central/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:165
    #4 0x2b9c783686ad in CallMethodHelper::Invoke() mozilla-central/js/xpconnect/src/XPCWrappedNative.cpp:3118
    #5 0x2b9c78368186 in XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) mozilla-central/js/xpconnect/src/XPCWrappedNative.cpp:2418
    #6 0x2b9c7837f22a in XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) mozilla-central/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1480
    #7 0x2b9c8cc0c36f in
    #8 0x2b9c7a7f3b67 in js::mjit::EnterMethodJIT(JSContext*, js::StackFrame*, void*, JS::Value*, bool) mozilla-central/js/src/methodjit/MethodJIT.cpp:1016
    #9 0x2b9c7a7f4a24 in CheckStackAndEnterMethodJIT(JSContext*, js::StackFrame*, void*, bool) mozilla-central/js/src/methodjit/MethodJIT.cpp:1074
    #10 0x2b9c7a33352f in js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) mozilla-central/js/src/jsinterp.cpp:1475
    #11 0x2b9c7a2fe9bd in js::RunScript(JSContext*, JSScript*, js::StackFrame*) mozilla-central/js/src/jsinterp.cpp:308
    #12 0x2b9c7a35588b in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::ExecuteType, js::StackFrame*, JS::Value*) mozilla-central/js/src/jsinterp.cpp:492
    #13 0x2b9c7a355f60 in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) mozilla-central/js/src/jsinterp.cpp:529
    #14 0x2b9c7a165f36 in JS::Evaluate(JSContext*, JS::Handle<JSObject*>, JS::CompileOptions, unsigned short const*, unsigned long, JS::Value*) mozilla-central/js/src/jsapi.cpp:5708
    #15 0x2b9c7a1678b7 in JS::Evaluate(JSContext*, JS::Handle<JSObject*>, JS::CompileOptions, char const*, unsigned long, JS::Value*) mozilla-central/js/src/jsapi.cpp:5723
    #16 0x4110cb in ProcessArgs(JSContext*, JSObject*, char**, int) mozilla-central/js/xpconnect/shell/xpcshell.cpp:1215
    #17 0x2b9c7ec0b30d in __libc_start_main /build/buildd/eglibc-2.13/csu/libc-start.c:258
0x2b9c8cf9cac8 is located 0 bytes to the right of 72-byte region [0x2b9c8cf9ca80,0x2b9c8cf9cac8)
allocated by thread T0 here:
    #0 0x43b401 in __interceptor_malloc ??:0
    #1 0x2b9c793f71d7 in nsStringBuffer::Alloc(unsigned long) mozilla-central/xpcom/string/src/nsSubstring.cpp:177
    #2 0x2b9c793fc65a in nsACString_internal::MutatePrep(unsigned int, char**, unsigned int*) mozilla-central/xpcom/string/src/nsTSubstring.cpp:130
    #3 0x2b9c793ff202 in nsACString_internal::SetCapacity(unsigned int, mozilla::fallible_t const&) mozilla-central/xpcom/string/src/nsTSubstring.cpp:553
    #4 0x2b9c793ff143 in nsACString_internal::SetCapacity(unsigned int) mozilla-central/xpcom/string/src/nsTSubstring.cpp:532
    #5 0x2b9c793ff6bc in nsACString_internal::SetLength(unsigned int) mozilla-central/xpcom/string/src/nsTSubstring.cpp:583
    #6 0x2b9c782fe89c in nsACString_internal::Length() const mozilla-central/../../../dist/include/nsTSubstring.h:209
    #7 0x2b9c783704db in CallMethodHelper::ConvertIndependentParam(unsigned char) mozilla-central/js/xpconnect/src/XPCWrappedNative.cpp:2882
    #8 0x2b9c78368513 in CallMethodHelper::ConvertIndependentParams(int*) mozilla-central/js/xpconnect/src/XPCWrappedNative.cpp:2804
    #9 0x2b9c78368186 in XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) mozilla-central/js/xpconnect/src/XPCWrappedNative.cpp:2418
    #10 0x2b9c7837f22a in XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) mozilla-central/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1480
    #11 0x2b9c8cc0c36f in
    #12 0x2b9c7a7f3b67 in js::mjit::EnterMethodJIT(JSContext*, js::StackFrame*, void*, JS::Value*, bool) mozilla-central/js/src/methodjit/MethodJIT.cpp:1016
    #13 0x2b9c7a7f4a24 in CheckStackAndEnterMethodJIT(JSContext*, js::StackFrame*, void*, bool) mozilla-central/js/src/methodjit/MethodJIT.cpp:1074
    #14 0x2b9c7a33352f in js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) mozilla-central/js/src/jsinterp.cpp:1475
    #15 0x2b9c7a2fe9bd in js::RunScript(JSContext*, JSScript*, js::StackFrame*) mozilla-central/js/src/jsinterp.cpp:308
    #16 0x2b9c7a35588b in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::ExecuteType, js::StackFrame*, JS::Value*) mozilla-central/js/src/jsinterp.cpp:492
    #17 0x2b9c7a355f60 in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) mozilla-central/js/src/jsinterp.cpp:529
    #18 0x2b9c7a165f36 in JS::Evaluate(JSContext*, JS::Handle<JSObject*>, JS::CompileOptions, unsigned short const*, unsigned long, JS::Value*) mozilla-central/js/src/jsapi.cpp:5708
    #19 0x2b9c7a1678b7 in JS::Evaluate(JSContext*, JS::Handle<JSObject*>, JS::CompileOptions, char const*, unsigned long, JS::Value*) mozilla-central/js/src/jsapi.cpp:5723
    #20 0x4110cb in ProcessArgs(JSContext*, JSObject*, char**, int) mozilla-central/js/xpconnect/shell/xpcshell.cpp:1215
    #21 0x2b9c7ec0b30d in __libc_start_main /build/buildd/eglibc-2.13/csu/libc-start.c:258

Comment 1

6 years ago
ccing julian in case he has ideas.  I can take a look after the B2G code fork.

Do we have a sense of what sec-rating this merits?
The security severity will depend on what it's reading and what it plans to use it for next. Could be anything from sec-critical to benign.

More importantly, though, this prevents us from getting a clean ASan run with the default testsuite, which means we can't stand this up as a regular build-test type and catch regressions.
(Reporter)

Comment 4

6 years ago
I started debugging this and the string it is failing on is: filename*1="\b\a\

The reason is this loop:

>      for (valueEnd = str; *valueEnd; ++valueEnd) {
>        if (*valueEnd == '\\')
>          ++valueEnd;
>        else if (*valueEnd == '"')
>          break;
>      }

With \ at the end, valueEnd is increased twice (stepping over the null-byte) and the loop will go into memory past the string. This is likely to at least disclose memory past the string so I assume this could be sec-high.
Keywords: sec-high
(Assignee)

Comment 5

6 years ago
(In reply to Christian Holler (:decoder) from comment #4)
> I started debugging this and the string it is failing on is:
> filename*1="\b\a\
> 
> The reason is this loop:
> 
> >      for (valueEnd = str; *valueEnd; ++valueEnd) {
> >        if (*valueEnd == '\\')
> >          ++valueEnd;
> >        else if (*valueEnd == '"')
> >          break;
> >      }
> 
> With \ at the end, valueEnd is increased twice (stepping over the null-byte)
> and the loop will go into memory past the string. This is likely to at least
> disclose memory past the string so I assume this could be sec-high.

Thanks for the feedback; I'll have a look.
(Assignee)

Comment 6

6 years ago
Created attachment 656606 [details]
Proposed patch, incl test case
Attachment #656606 - Flags: review?(jduell.mcbugs)

Comment 7

6 years ago
Comment on attachment 656606 [details]
Proposed patch, incl test case

+r with one nit:  use single quotes as the surrounding quote type, so you don't have to escape double quotes within the test string--easier to read that way.

Thanks :decoder and Julian for the fix!
Attachment #656606 - Flags: review?(jduell.mcbugs) → review+

Comment 8

6 years ago
I've fixed up the patch with my quote nit, and will land once inbound is open again...
(Assignee)

Comment 9

6 years ago
Created attachment 656645 [details] [diff] [review]
Proposed patch, incl test case

patch updated to use single quotes in test strings as suggested
Attachment #656606 - Attachment is obsolete: true
Attachment #656645 - Flags: review?(jduell.mcbugs)
(Reporter)

Comment 10

6 years ago
Comment on attachment 656645 [details] [diff] [review]
Proposed patch, incl test case

No need to re-request review for this, you already got r+ with that change :) You can just land this once inbound is open again. Thanks for the fix! It greatly helps our effort to get try green with ASan :)
Attachment #656645 - Flags: review?(jduell.mcbugs) → review+
(Assignee)

Updated

6 years ago
Assignee: nobody → julian.reschke
Keywords: checkin-needed
OS: Linux → All
Hardware: x86_64 → All
Attachment #656645 - Attachment is patch: true

Comment 11

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/9da151b4b1c9
tracking-firefox16: --- → ?
tracking-firefox17: --- → ?

Comment 12

6 years ago
Comment on attachment 656645 [details] [diff] [review]
Proposed patch, incl test case

No known exploits to this march-off-end-of-string security hole.  OTOH very simple fix.
Attachment #656645 - Flags: approval-mozilla-beta?
Attachment #656645 - Flags: approval-mozilla-aurora?

Updated

6 years ago
Flags: in-testsuite-

Comment 13

6 years ago
hmm, my pull-down menu skillz are atrophying.
Flags: in-testsuite- → in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9da151b4b1c9
Status: NEW → RESOLVED
Last Resolved: 6 years ago
status-firefox18: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Is ESR affected by this?
(Reporter)

Comment 16

6 years ago
(In reply to Al Billings [:abillings] from comment #15)
> Is ESR affected by this?

As far as I can tell from the code, yes. Setting tracking flag for ESR.
status-firefox-esr10: --- → affected
tracking-firefox-esr10: --- → ?

Updated

6 years ago
tracking-firefox-esr10: ? → 16+
tracking-firefox16: ? → +
tracking-firefox17: ? → +
Comment on attachment 656645 [details] [diff] [review]
Proposed patch, incl test case

[Triage Comment]
Low risk sec-high fix and early in the cycle, let's land on all branches. Please prepare an ESR patch as well.
Attachment #656645 - Flags: approval-mozilla-beta?
Attachment #656645 - Flags: approval-mozilla-beta+
Attachment #656645 - Flags: approval-mozilla-aurora?
Attachment #656645 - Flags: approval-mozilla-aurora+

Comment 18

6 years ago
https://hg.mozilla.org/releases/mozilla-beta/rev/51f3fe3dbd81
https://hg.mozilla.org/releases/mozilla-aurora/rev/38bf74b400b7

The memory safety issue here does not exist in the esr branch: it was introduced in bug 384571 which made extensive changes to this file.   I'd remove tracking-esr but I'm not sure if I'm supposed to touch that frob :)
Depends on: 384571

Comment 19

6 years ago
And my apologies for not removing the bug description before landing.  We should train hg to strip off descriptions for security bugs, because apparently I'm the poster boy for user error on that issue :(

Comment 20

6 years ago
filed bug 790143 for the hg comment-stripping automation.
(Assignee)

Comment 21

6 years ago
(In reply to Jason Duell (:jduell) from comment #18)
> https://hg.mozilla.org/releases/mozilla-beta/rev/51f3fe3dbd81
> https://hg.mozilla.org/releases/mozilla-aurora/rev/38bf74b400b7
> 
> The memory safety issue here does not exist in the esr branch: it was
> introduced in bug 384571 which made extensive changes to this file.   I'd
> remove tracking-esr but I'm not sure if I'm supposed to touch that frob :)

Not true. That change only changed some formatting over there.
Marking ESR as unaffected.
status-firefox-esr10: affected → unaffected
tracking-firefox-esr10: 16+ → ---
Comment 21 means we're still possibly affected actually (thanks decoder).
status-firefox-esr10: unaffected → affected
Julian/Jason - can you prepare a patch for the ESR branch?
status-firefox16: --- → fixed
status-firefox17: --- → fixed
tracking-firefox-esr10: --- → 16+
(Assignee)

Comment 25

6 years ago
Alex: is https://tbpl.mozilla.org/?tree=Mozilla-Esr10 the right branch?
(In reply to Julian Reschke from comment #25)
> Alex: is https://tbpl.mozilla.org/?tree=Mozilla-Esr10 the right branch?

It is.
(Assignee)

Comment 27

6 years ago
Created attachment 663032 [details] [diff] [review]
patch ported from mozilla-central

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
Fix Landed on Version:
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch: 

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #663032 - Flags: review?
Attachment #663032 - Flags: approval-mozilla-esr10?
You need to set an explicit person as the reviewer (probably jduell), or if it is basically the same patch as you got reviewed before you probably don't need a re-review.
(Assignee)

Updated

6 years ago
Attachment #663032 - Flags: review? → review?(jduell.mcbugs)

Comment 29

6 years ago
Comment on attachment 663032 [details] [diff] [review]
patch ported from mozilla-central

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

Thanks for doing this Julian--been too swamped to get to it myself.

https://hg.mozilla.org/releases/mozilla-esr10/rev/b46718d66018
Attachment #663032 - Flags: review?(jduell.mcbugs) → review+
status-firefox-esr10: affected → fixed
Comment on attachment 663032 [details] [diff] [review]
patch ported from mozilla-central

In the future, please wait for an ESR a+ before landing :)
Attachment #663032 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
Whiteboard: [asan][asan-test-failure] → [advisory-tracking+][asan][asan-test-failure]

Updated

6 years ago
QA Contact: general
Whiteboard: [advisory-tracking+][asan][asan-test-failure] → [advisory-tracking+][asan][asan-test-failure][qa-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.