1212328 - Crash [@ js::PrintError] with heap-buffer-overflow involving evalInWorker

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 727d765a5ed8 (build with --enable-gczeal --enable-optimize="-O2 -g" --enable-address-sanitizer --enable-posix-nspr-emulation --disable-jemalloc --disable-tests --disable-debug, run with --fuzzing-safe --thread-count=2 --ion-eager --ion-offthread-compile=off):

evalInWorker('if (test("foo\u00ff\u1200").foooo) let x = 42;');



Backtrace:

==26395==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x61200002f576 at pc 0x168fbfe bp 0x7f82a8ea60d0 sp 0x7f82a8ea60c8
READ of size 1 at 0x61200002f576 thread T7
    #0 0x168fbfd in js::PrintError(JSContext*, _IO_FILE*, char const*, JSErrorReport*, bool) js/src/jscntxt.cpp:537
    #1 0x4dbfae in js::shell::my_ErrorReporter(JSContext*, char const*, JSErrorReport*) js/src/shell/js.cpp:5199
    #2 0x16865bf in js::CallErrorReporter(JSContext*, char const*, JSErrorReport*) js/src/jscntxt.cpp:824
    #3 0x16865bf in js::ReportUncaughtException(JSContext*) js/src/jsexn.cpp:661
    #4 0x16718a1 in ~Rooted js/src/jsapi.cpp:3800
    #5 0x16718a1 in Compile(JSContext*, JS::ReadOnlyCompileOptions const&, SyntacticScopeOption, JS::SourceBufferHolder&, JS::MutableHandle<JSScript*>) js/src/jsapi.cpp:4053
    #6 0x1671dbc in Compile(JSContext*, JS::ReadOnlyCompileOptions const&, SyntacticScopeOption, char16_t const*, unsigned long, JS::MutableHandle<JSScript*>) js/src/jsapi.cpp:4060
    #7 0x1671dbc in JS::Compile(JSContext*, JS::ReadOnlyCompileOptions const&, char16_t const*, unsigned long, JS::MutableHandle<JSScript*>) js/src/jsapi.cpp:4119
    #8 0x4feb08 in WorkerMain(void*) js/src/shell/js.cpp:2574
    #9 0xb02638 in nspr::Thread::ThreadRoutine(void*) js/src/vm/PosixNSPR.cpp:45
    #10 0x7f82afda7181 in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x8181)
    #11 0x7f82aec8f47c (/lib/x86_64-linux-gnu/libc.so.6+0xfa47c)

0x61200002f576 is located 0 bytes to the right of 310-byte region [0x61200002f440,0x61200002f576)
allocated by thread T7 here:
    #0 0x4af9d7 in __interceptor_malloc /srv/repos/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:74
    #1 0x507e7b in js_malloc(unsigned long) js/src/opt64asan/js/src/../../dist/include/js/Utility.h:229
    #2 0x507e7b in _ZL13js_pod_mallocIhEPT_m js/src/opt64asan/js/src/../../dist/include/js/Utility.h:384
    #3 0x507e7b in unsigned char* js::MallocProvider<js::ExclusiveContext>::maybe_pod_malloc<unsigned char>(unsigned long) js/src/vm/MallocProvider.h:56
    #4 0x507e7b in unsigned char* js::MallocProvider<js::ExclusiveContext>::pod_malloc<unsigned char>(unsigned long) js/src/vm/MallocProvider.h:90
    #5 0x16b1694 in js::CopyErrorReport(JSContext*, JSErrorReport*) js/src/jsexn.cpp:188
    #6 0x16b4dc5 in js::ErrorToException(JSContext*, char const*, JSErrorReport*, JSErrorFormatString const* (*)(void*, unsigned int), void*) js/src/jsexn.cpp:575
    #7 0x898e0a in js::frontend::CompileError::throwError(JSContext*) js/src/frontend/TokenStream.cpp:583
    #8 0x898e0a in js::frontend::TokenStream::reportCompileErrorNumberVA(unsigned int, unsigned int, unsigned int, __va_list_tag*) js/src/frontend/TokenStream.cpp:712
    #9 0x57867a in js::frontend::Parser<js::frontend::FullParseHandler>::reportHelper(js::frontend::ParseReportKind, bool, unsigned int, unsigned int, __va_list_tag*) js/src/frontend/Parser.cpp:479
    #10 0x57867a in js::frontend::Parser<js::frontend::FullParseHandler>::reportWithOffset(js::frontend::ParseReportKind, bool, unsigned int, unsigned int, ...) js/src/frontend/Parser.h:526
    #11 0x571137 in js::frontend::Parser<js::frontend::FullParseHandler>::checkAndPrepareLexical(bool, js::frontend::TokenPos const&) js/src/frontend/Parser.cpp:4342
    #12 0x551ebc in js::frontend::Parser<js::frontend::FullParseHandler>::lexicalDeclaration(js::frontend::YieldHandling, bool) js/src/frontend/Parser.cpp:4447
    #13 0x551cf9 in js::frontend::Parser<js::frontend::FullParseHandler>::letDeclarationOrBlock(js::frontend::YieldHandling) js/src/frontend/Parser.cpp:4506
    #14 0x583385 in js::frontend::Parser<js::frontend::FullParseHandler>::statement(js::frontend::YieldHandling, bool) js/src/frontend/Parser.cpp:6837
    #15 0x5885a0 in js::frontend::Parser<js::frontend::FullParseHandler>::ifStatement(js::frontend::YieldHandling) js/src/frontend/Parser.cpp:5074
    #16 0x582ca6 in js::frontend::Parser<js::frontend::FullParseHandler>::statement(js::frontend::YieldHandling, bool) js/src/frontend/Parser.cpp:6766
    #17 0x87b53e in BytecodeCompiler::compileScript(JS::Handle<JSObject*>, JS::Handle<JSScript*>) js/src/frontend/BytecodeCompiler.cpp:589
    #18 0x881008 in js::frontend::CompileScript(js::ExclusiveContext*, js::LifoAlloc*, JS::Handle<JSObject*>, JS::Handle<js::ScopeObject*>, JS::Handle<JSScript*>, JS::ReadOnlyCompileOptions const&, JS::SourceBufferHolder&, JSString*, js::SourceCompressionTask*, js::ScriptSourceObject**) js/src/frontend/BytecodeCompiler.cpp:808
    #19 0x1671739 in Compile(JSContext*, JS::ReadOnlyCompileOptions const&, SyntacticScopeOption, JS::SourceBufferHolder&, JS::MutableHandle<JSScript*>) js/src/jsapi.cpp:4050
    #20 0x1671dbc in Compile(JSContext*, JS::ReadOnlyCompileOptions const&, SyntacticScopeOption, char16_t const*, unsigned long, JS::MutableHandle<JSScript*>) js/src/jsapi.cpp:4060
    #21 0x1671dbc in JS::Compile(JSContext*, JS::ReadOnlyCompileOptions const&, char16_t const*, unsigned long, JS::MutableHandle<JSScript*>) js/src/jsapi.cpp:4119
    #22 0x4feb08 in WorkerMain(void*) js/src/shell/js.cpp:2574
    #23 0xb02638 in nspr::Thread::ThreadRoutine(void*) js/src/vm/PosixNSPR.cpp:45
    #24 0x7f82afda7181 in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x8181)

Thread T7 created by T0 here:
    #0 0x4738b6 in __interceptor_pthread_create /srv/repos/llvm/projects/compiler-rt/lib/asan/asan_interceptors.cc:175
    #1 0xa64fe6 in PR_CreateThread(PRThreadType, void (*)(void*), void*, PRThreadPriority, PRThreadScope, PRThreadState, unsigned int) js/src/vm/PosixNSPR.cpp:108
    #2 0x4f23dd in EvalInWorker(JSContext*, unsigned int, JS::Value*) js/src/shell/js.cpp:2618
    #3 0xa9fe58 in js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) js/src/jscntxtinlines.h:235
    #4 0xa9fe58 in js::Invoke(JSContext*, JS::CallArgs const&, js::MaybeConstruct) js/src/vm/Interpreter.cpp:756
    #5 0xaa1f3d in js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) js/src/vm/Interpreter.cpp:823
    #6 0xf8bcb3 in js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) js/src/jit/BaselineIC.cpp:8905

SUMMARY: AddressSanitizer: heap-buffer-overflow js/src/jscntxt.cpp:537 js::PrintError(JSContext*, _IO_FILE*, char const*, JSErrorReport*, bool)
Shadow bytes around the buggy address:
  0x0c247fffde50: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c247fffde60: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c247fffde70: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c247fffde80: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
  0x0c247fffde90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0c247fffdea0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00[06]fa
  0x0c247fffdeb0: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
  0x0c247fffdec0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c247fffded0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 06
  0x0c247fffdee0: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
  0x0c247fffdef0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Heap right redzone:      fb
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack partial redzone:   f4
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Contiguous container OOB:fc
  ASan internal:           fe
==26395==ABORTING


Filing s-s because JS developers could not agree on something else within 10 seconds :D

Comment 1

[Christian Holler (:decoder)]

Needinfo from jandem for this one :)

Comment 2

[Fuzzing Team]

JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.

Comment 3

[Jan de Mooij [:jandem]]

This is pretty bad:

* JSErrorReport has a `char* linebuf` that stores both a part of the line + tokenptr.
* In this case, the line has a null-byte.
* Then CopyErrorReport does strlen(report->linebuf), so the copy only has the first part of the buffer.

JSErrorReport is a mess unfortunately :(

Comment 4

[Jan de Mooij [:jandem]]

Hm I think the only place where we depend on report->(uc)linebuf holding at least tokenptr - linebuf chars is PrintError (where we're crashing here).

There are some places in Gecko where we also do tokenptr - linebuf, but that's just to determine the column number (which seems bogus because linebuf only holds a small part of the string).

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Patch

This patch just adds a linebufLength to JSErrorReport and fixes CopyErrorReport to use that instead of strlen.

JSErrorReport really needs a round of refactoring though. Or ten rounds.

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Patch

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 8670858 [details] [diff] [review]
Patch

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

r=me on what's here, but I think you need a bit more changing in this.  And because it's probably not a great idea individually evaluating each change for safety deferring to another bug or not, we probably should do it all here, even if it makes for a bigger-than-definitely-necessary patch.

Change dom/workers/WorkerPrivate.cpp's use of uclinebuf to copy the whole string based on linebufLength.  Also file a bug to make that file's |columnNumber = aReport->uctokenptr - aReport->uclinebuf;| use aReport->column as well (I think you can, but neither you nor I should eyeball it and do that here -- plus it should change behavior, and so requires tests and bz-level scrutiny).

Also |uint32_t column = err->uctokenptr - err->uclinebuf;| in js/xpconnect/src/XPCComponents.cpp, new bug for that.  And the gratuitous casts three lines down could die, too.  And |uclinebuf ? nsDependentString(uclinebuf) : EmptyString()| just beneath there should be converted to pass in the full uclinebuf length, not the null-terminated bit.

And in js/xpconnect/src/XPCConvert.cpp, a gratuitous cast.  And an nsDependentString truncation.  And in the InitWithWindowID, should pass column and not a subtraction.  More new bug fodder.

netwerk/base/ProxyAutoConfig.cpp should also pass a dependent string with full context, not a truncated form.

Actually, this is enough you probably really should just fix it all here, rather than sort through exactly which changes can be deferred -- safely -- and which cannot.  And you probably should have a Gecko person review the adjustments and associated tests, too, in addition to just me.

::: js/src/frontend/TokenStream.cpp
@@ +690,5 @@
>          // Create the windowed strings.
>          StringBuffer windowBuf(cx);
>          if (!windowBuf.append(userbuf.rawCharPtrAt(windowStart), windowLength) ||
>              !windowBuf.append((char16_t)0))
>              return false;

Mind making that a '\0' and adding the missing braces while you're here?

@@ +701,5 @@
>          if (!err.report.uclinebuf)
>              return false;
>  
>          mozilla::Range<const char16_t> tbchars(err.report.uclinebuf, windowLength);
>          err.report.linebuf = JS::LossyTwoByteCharsToNewLatin1CharsZ(cx, tbchars).c_str();

I think you're missing a fix to this code, and what's just below it.  This line here truncates.  Then later we have:

  err.report.tokenptr = err.report.linebuf + (offset - windowStart);

::: js/src/jit-test/tests/basic/error-report-null-char.js
@@ +1,1 @@
> +evalInWorker('if (test("foo\u00ff\u1200").foooo) let x = 42;');

Minimize this to just the bad string literal -- that's all that's needed here, right?  Too much extraneous stuff here, to have a properly targeted test.

::: js/src/jsexn.cpp
@@ +158,5 @@
>      size_t mallocSize;
>      JSErrorReport* copy;
>      uint8_t* cursor;
>  
>  #define JS_CHARS_SIZE(chars) ((js_strlen(chars) + 1) * sizeof(char16_t))

Can you remove this now?

Comment 8

[Jeff Walden [:Waldo]]

(Consider the first graf of comment 7 as the final word -- some of the rest I wrote before deciding better not to be ticky-tack about it.  Resolve contradictions accordingly.)

Comment 9

[Jan de Mooij [:jandem]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #7)
> >          mozilla::Range<const char16_t> tbchars(err.report.uclinebuf, windowLength);
> >          err.report.linebuf = JS::LossyTwoByteCharsToNewLatin1CharsZ(cx, tbchars).c_str();
> 
> I think you're missing a fix to this code, and what's just below it.  This
> line here truncates.  Then later we have:

Actually it doesn't truncate: LossyTwoByteCharsToNewLatin1CharsZ uses tbchars.length(). So it should be safe, but agreed this is really confusing.

> Minimize this to just the bad string literal -- that's all that's needed
> here, right?  Too much extraneous stuff here, to have a properly targeted
> test.

I think the test then no longer triggers the ASAN failure, but I can repro locally so I can try that (decoder also said the number of o's in foooo matters).

> >  #define JS_CHARS_SIZE(chars) ((js_strlen(chars) + 1) * sizeof(char16_t))
> 
> Can you remove this now?

No, it's also used below, for the message string IIRC :( So yeah, we should probably fix all that too while we're at it.

Are you okay with a new public bug with a patch stack to clean up all this JSErrorReport nonsense? I really don't see anything potentially s-s here and if I'm going to fix these issues I'd prefer cleaning up everything on m-c. If not, which bits do you think should be hidden and uplifted?

Comment 10

[Jeff Walden [:Waldo]]

(In reply to Jan de Mooij [:jandem] from comment #9)
> Actually it doesn't truncate: LossyTwoByteCharsToNewLatin1CharsZ uses
> tbchars.length(). So it should be safe

Oh, right.  Did I see this, then forget it, during reviewing?  :-(  Feh.

> Are you okay with a new public bug with a patch stack to clean up all this
> JSErrorReport nonsense? I really don't see anything potentially s-s here and
> if I'm going to fix these issues I'd prefer cleaning up everything on m-c.
> If not, which bits do you think should be hidden and uplifted?

I don't think I am.  :-(  There's enough variety of change, and truncation in various places, that I really don't feel comfortable picking and choosing my way through which are safe to defer to another bug, and which must be done now.  I'd rather see a slightly-larger patch that fixes the problems everywhere, than one that knowingly misses spots -- but still has to fully review every spot to be sure it's safe to defer there, anyway.  Even if this 5K patch sized up to 10-15K, I still wouldn't be too worried about its size.

Comment 11

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Jan, is this ready for review then landing? (note that sec-approval might be needed first)

Comment 12

[Jan de Mooij [:jandem]]

Attachment: Patch v2

This patch is much nicer than the previous one.

I made these JSErrorReport fields private and added a method to initialize them all at once. This makes it much easier to check the invariants.

It also removes tokenptr/uctokenptr; it's simpler to just store a tokenOffset.

Flagging bz for the changes outside js/src.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8709966 [details] [diff] [review]
Patch v2

>+    // Offset of error token in linebuf_ and uclinebuf_.

Worth documenting whether this is 0-based or what, and that it might make sense to use this as a column number.  Assuming it makes sense, of course.

>+++ b/js/xpconnect/src/XPCComponents.cpp
>+                uclinebuf ? nsDependentString(uclinebuf, err->linebufLength()) : EmptyString(),

Is uclinebuf + err->linebufLength() guaranteed to be a null char?  If not, this should be an nsDependentSubstring(), not an nsDependentString.

Or even simpler,

  Substring(uclinebuf, err->linebufLength())

(which returns an nsDependentSubstring).

>+++ b/js/xpconnect/src/XPCConvert.cpp

Same issue here.

r=me with those nits

Comment 14

[Jan de Mooij [:jandem]]

(In reply to Boris Zbarsky [:bz] from comment #13)
> Worth documenting whether this is 0-based or what, and that it might make
> sense to use this as a column number.  Assuming it makes sense, of course.

Good point, most of these places should probably use the |column| field instead. (Waldo also mentioned that.) I think it matters for long lines, in that case |linebuf| does not contain the whole line. I'll file a followup bug for this.

> Is uclinebuf + err->linebufLength() guaranteed to be a null char? If not,
> this should be an nsDependentSubstring(), not an nsDependentString.

Yes it's guaranteed to be null-terminated (the patch asserts this in JSErrorReport::initLinebuf). I'll add a comment.

Comment 15

[Jan de Mooij [:jandem]]

Waldo, 3-week review ping.

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

Which versions does this affect?  Everything current?

Comment 17

[Liz Henry (:lizzard) (relman/hg->git project)]

Sec-high, tracking on the assumption this likely affects 45+ and we have a patch for possible uplift.

Comment 18

[Jeff Walden [:Waldo]]

Comment on attachment 8709966 [details] [diff] [review]
Patch v2

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

Thanks for doing the extra bits I wanted!  Sorry for the delay here.  :-(

::: js/src/frontend/TokenStream.cpp
@@ +690,5 @@
>  
>          // Create the windowed strings.
>          StringBuffer windowBuf(cx);
>          if (!windowBuf.append(userbuf.rawCharPtrAt(windowStart), windowLength) ||
>              !windowBuf.append((char16_t)0))

'\0'

@@ +703,4 @@
>              return false;
>  
> +        mozilla::Range<const char16_t> tbchars(uclinebuf.get(), windowLength);
> +        UniqueChars linebuf(JS::LossyTwoByteCharsToNewLatin1CharsZ(cx, tbchars).c_str());

Add a comment by this noting that while |linebuf| may contain null characters, its memory will be fully valid for all of |windowLength|.

::: js/src/jscntxt.cpp
@@ +533,3 @@
>          for (int i = 0, j = 0; i < n; i++) {
> +            if (linebuf[i] == '\t') {
> +                for (int k = (j + 8) & ~7; j < k; j++)

Change i/j/k to size_t?

Comment 19

[Jan de Mooij [:jandem]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #16)
> Which versions does this affect?  Everything current?

Yes. (Sorry for the delay.)

Given that we don't have much time, it should be fine to get this in 46 but not 45: this code is ancient and I don't think there's an actual (non-shell) security bug here.

So let's treat it as a security bug, just in case, but since the patch is not trivial, I'd rather not land it (on beta) last minute.

Comment 20

[Liz Henry (:lizzard) (relman/hg->git project)]

OK, thanks!  Can you request sec-approval and uplift ? 
Does this affect esr?

Comment 21

[Jan de Mooij [:jandem]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #18)
> > +        mozilla::Range<const char16_t> tbchars(uclinebuf.get(), windowLength);
> > +        UniqueChars linebuf(JS::LossyTwoByteCharsToNewLatin1CharsZ(cx, tbchars).c_str());
> 
> Add a comment by this noting that while |linebuf| may contain null
> characters, its memory will be fully valid for all of |windowLength|.

I was working on this and then realized the latin1 copy of linebuf in JSErrorReport is *only* used in js::PrintError, but there we can just as easily use uclinebuf.

That lets us remove the confusing LossyTwoByteCharsToNewLatin1CharsZ and I think that's worth doing as part of this bug. Will post an updated patch.

Comment 22

[Jan de Mooij [:jandem]]

Attachment: Patch v3

Waldo, sorry for the churn, but removing the latin1 linebuf made this much nicer and simpler.

Comment 23

[Jeff Walden [:Waldo]]

Comment on attachment 8724700 [details] [diff] [review]
Patch v3

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

::: js/xpconnect/src/XPCComponents.cpp
@@ +2341,5 @@
>  
>          const char16_t* ucmessage =
>              static_cast<const char16_t*>(err->ucmessage);
> +        const char16_t* linebuf =
> +            static_cast<const char16_t*>(err->linebuf());

No cast.

::: js/xpconnect/src/XPCConvert.cpp
@@ +1249,5 @@
>              bestMessage.AssignLiteral("JavaScript Error");
>          }
>  
> +        const char16_t* linebuf =
> +            static_cast<const char16_t*>(report->linebuf());

No need for this cast.

::: js/xpconnect/src/nsXPConnect.cpp
@@ +196,5 @@
>      } else {
>          mFileName.AssignWithConversion(aReport->filename);
>      }
>  
> +    mSourceLine.Assign(static_cast<const char16_t*>(aReport->linebuf()),

Kill the cast.

Comment 24

[Sylvestre Ledru [:Sylvestre]]

Too late for 45 as today is release day!

Comment 25

[Jan de Mooij [:jandem]]

Attachment: Patch

Comment 26

[Jan de Mooij [:jandem]]

Comment on attachment 8729097 [details] [diff] [review]
Patch

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
It's not clear this is even exploitable - the crash here is in a shell-only function. The patch also cleans up some gnarly code elsewhere, though.

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

> Which older supported branches are affected by this flaw?
All, it's ancient code AFAIK.

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

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

Comment 27

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

Comment on attachment 8729097 [details] [diff] [review]
Patch

sec-approval+ for trunk.

Do we want to back port this at all to branches?

Comment 28

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/66a3bf3abe2e

Comment 29

[Jan de Mooij [:jandem]]

Will request uplift next week.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/66a3bf3abe2e

Comment 31

[Jan de Mooij [:jandem]]

Comment on attachment 8729097 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: Ancient code.
[User impact if declined]: Potential security issues.
[Describe test coverage new/current, TreeHerder]: These code paths should be tested on Treeherder.
[Risks and why]: Low risk.
[String/UUID change made/needed]: None.

Comment 32

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Jan, Al: I noticed that the patch is nom'd for uplift to esr38. Given that this is not easily exploitable and the fix is pretty big (not a one-liner), I plan to deny the esr38 uplift and wontfix for that version. Please let me know if you have any concerns.

Comment 33

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

I think we could live without it on ESR38 given that it is close to end of life as long as ESR45 gets it.

Comment 34

[Jan de Mooij [:jandem]]

Yes, not taking this on ESR38 is fine.

Comment 35

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8729097 [details] [diff] [review]
Patch

Sec-high fix, Aurora47+, Beta46+, ESR45+, ESR38-

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1c90e31205c8

Comment 37

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9136c8ca8be3

Comment 38

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-esr45/rev/8abadeccb04b

Comment 39

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This broke all builds on esr45 like https://treeherder.mozilla.org/logviewer.html#?job_id=15249&repo=mozilla-esr45

Backed out in https://hg.mozilla.org/releases/mozilla-esr45/rev/836454ede32d

Comment 40

[Jan de Mooij [:jandem]]

Oops, sorry about that.

https://hg.mozilla.org/releases/mozilla-esr45/rev/0a2953e1d751