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

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Christian Holler (:decoder)]

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

[Jason Duell]

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?

Comment 2

[Daniel Veditz [:dveditz]]

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.

Comment 3

[Julian Reschke]

I'm looking at http://hg.mozilla.org/mozilla-central/annotate/f7cf60910637/netwerk/mime/nsMIMEHeaderParamImpl.cpp#l449, but can't see anything suspicious...

Comment 4

[Christian Holler (:decoder)]

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.

Comment 5

[Julian Reschke]

(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.

Comment 6

[Julian Reschke]

Attachment: Proposed patch, incl test case

Comment 7

[Jason Duell]

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!

Comment 8

[Jason Duell]

I've fixed up the patch with my quote nit, and will land once inbound is open again...

Comment 9

[Julian Reschke]

Attachment: Proposed patch, incl test case

patch updated to use single quotes in test strings as suggested

Comment 10

[Christian Holler (:decoder)]

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 :)

Comment 11

[Jason Duell]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9da151b4b1c9

Comment 12

[Jason Duell]

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.

Comment 13

[Jason Duell]

hmm, my pull-down menu skillz are atrophying.

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9da151b4b1c9

Comment 15

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

Is ESR affected by this?

Comment 16

[Christian Holler (:decoder)]

(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.

Comment 17

[Alex Keybl [:akeybl]]

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.

Comment 18

[Jason Duell]

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 :)

Comment 19

[Jason Duell]

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

[Jason Duell]

filed bug 790143 for the hg comment-stripping automation.

Comment 21

[Julian Reschke]

(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.

Comment 22

[Alex Keybl [:akeybl]]

Marking ESR as unaffected.

Comment 23

[Alex Keybl [:akeybl]]

Comment 21 means we're still possibly affected actually (thanks decoder).

Comment 24

[Alex Keybl [:akeybl]]

Julian/Jason - can you prepare a patch for the ESR branch?

Comment 25

[Julian Reschke]

Alex: is https://tbpl.mozilla.org/?tree=Mozilla-Esr10 the right branch?

Comment 26

[Andrew McCreight [:mccr8]]

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

It is.

Comment 27

[Julian Reschke]

Attachment: 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.

Comment 28

[Andrew McCreight [:mccr8]]

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.

Comment 29

[Jason Duell]

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

Comment 30

[Alex Keybl [:akeybl]]

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

In the future, please wait for an ESR a+ before landing :)