1245866 - Crash at nsIDNService::IDNA2008ToUnicode nsUDPSocket::SetMulticastLoopback js::frontend::Parser<js::frontend::FullParseHandler>::functionDef NS_UnescapeURL arena_run_dalloc

Status: VERIFIED FIXED

(Core :: Networking: DNS, defect)

Description

[Carsten Book [:Tomcat]]

Attachment: bughunter stack

Found via bughunter and reproduced on a win 7 trunk debug build based on m-c tip from today

Steps to reproduce:
-> Load http://estimativa.org.br/home/index.php/noticias/257-como-funciona-o-orcamento-publico 

--> Crashes both opt and debug builds 

Windbg reports: STATUS_STACK_BUFFER_OVERRUN encountered 

Signature seems to be 
nsIDNService::IDNA2008ToUnicode nsUDPSocket::SetMulticastLoopback js::frontend::Parser<js::frontend::FullParseHandler>::functionDef NS_UnescapeURL arena_run_dalloc

Comment 1

[Carsten Book [:Tomcat]]

baku, smaug: is this something for you  guys ?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

baku can reproduce this, I can't.
(I'm using clang on linux, he is using gcc. perhaps that matters here.)

Comment 3

[Bob Clary [:bc] (inactive)]

Also Beta/45 and Aurora/46 on Windows and RHEL Linux 64bit. Note bughunter uses tinderbox builds, so you might have some luck with those.

Comment 4

[Bob Clary [:bc] (inactive)]

and Firefox 44 bp-d119c2eb-c2cb-4bd1-a568-ae8fd2160131 which is rated as medium.

Comment 5

[Andrea Marchesini [:baku]]

I'm taking this bug.

Comment 6

[Andrea Marchesini [:baku]]

Attachment: crash.patch

Comment 7

[Andrew McCreight [:mccr8]]

This looks like an out-of-bounds write to the stack with some kind of content-controlled data, so I'm going to mark it sec-critical.

Comment 8

[Carsten Book [:Tomcat]]

sylvester, something for beta (according to #3 and #4) and ride along ?

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8716940 [details] [diff] [review]
crash.patch

I have no idea why this would be right. Sure, shouldn't crash, but
why we use different kind of buffer here comparing to the other case 
uidna_labelToUnicode is being called.

I'm not familiar with this code. Simon or Jonathan should review this.
This is after all a regression from bug 479520.

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Yes, we probably want this in beta. Thanks Carsten!

Comment 11

[Jonathan Kew [:jfkthame]]

Comment on attachment 8716940 [details] [diff] [review]
crash.patch

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

::: netwerk/dns/nsIDNService.cpp
@@ +168,5 @@
>    UIDNAInfo info = UIDNA_INFO_INITIALIZER;
>    UErrorCode errorCode = U_ZERO_ERROR;
>    int32_t inLen = inputStr.Length();
>    int32_t outMaxLen = inLen - kACEPrefixLen + 1;
> +  UChar outputBuffer[outMaxLen];

Is it OK to use a variable-size array here? I thought that was a non-standard extension. AIUI, it's part of C99, but became optional in C11; not sure of the status in C++ but at least https://gcc.gnu.org/onlinedocs/gcc/Variable-Length.html describes it as an extension.

ISTM we should probably either use kMaxDNSNodeLen + 1 as outMaxLen (if truncating excessively-long input is OK here -- Simon?), or use a standard C++ method to allocate the variable-sized buffer.

Comment 12

[Andrea Marchesini [:baku]]

Attachment: crash.patch

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8719471 [details] [diff] [review]
crash.patch

(I'm really not familiar with this code, so I don't know what is a good max length. Is there some testcase leading to executing this code and also the other method using uidna_labelToUnicode)

Comment 14

[Jonathan Kew [:jfkthame]]

Attachment: Testcase

Here's an example that will hit both those codepaths. It will crash with current trunk due to the bug in IDNA2008ToUnicode; the patch makes that function safely return an error code instead. Obviously, we can't land the crashtest until the bug is fixed, as it points directly at the badness.

Comment 15

[Jonathan Kew [:jfkthame]]

Comment on attachment 8719471 [details] [diff] [review]
crash.patch

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

Yes, AFAICS using the fixed kMaxDNSNodeLen+1 limit is fine here.

Comment 16

[Jonathan Kew [:jfkthame]]

Attachment: Testcase

Just adding <meta charset=utf-8> to the testcase; sorry for the churn.

Comment 17

[Jonathan Kew [:jfkthame]]

Attachment: Testcase

Just adding <meta charset=utf-8> to the testcase; sorry for the churn.

Comment 18

[Jonathan Kew [:jfkthame]]

Comment on attachment 8719471 [details] [diff] [review]
crash.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
The patch makes it pretty obvious which buffer is being overrun; and then it's not hard to guess how one might hit this codepath from content.

So a DoS exploit, at least, is trivial; and given that it's a stack buffer being overflowed, I imagine overwriting a return address and executing arbitrary code may not be much more difficult.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Comments aren't needed, the patch itself is a bullseye! As is the crashtest (in separate patch).

Which older supported branches are affected by this flaw?
Affects mozilla-44 and later; esr-38 should be unaffected.

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

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

How likely is this patch to cause regressions; how much testing does it need?
Minimal regression risk, this just ensures we pass the correct buffer size to prevent an overflow.

Comment 19

[Jonathan Kew [:jfkthame]]

Comment on attachment 8719546 [details] [diff] [review]
Testcase

See above. The crashtest provides a trivial demonstration of how to hit the bug; I assume we should wait until the bug is fixed on all channels before we land the test anywhere.

Comment 20

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

Comment on attachment 8719471 [details] [diff] [review]
crash.patch

sec-approval+ for the patch. The crashtest should wait until we ship a release with a fix so I'm clearing sec-approval? on that.

Please nominate for aurora and beta so we can land it there as well.

Comment 21

[Jonathan Kew [:jfkthame]]

Comment on attachment 8719471 [details] [diff] [review]
crash.patch

Approval Request Comment
[Feature/regressing bug #]: bug 479520

[User impact if declined]: buffer overflow triggered by content, leading to crash or potentially exploitable vulnerability

[Describe test coverage new/current, TreeHerder]: crashtest will be landed after we ship the fix

[Risks and why]: minimal risk -- matches outbuffer size and parameter

[String/UUID change made/needed]: none

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b09f22c69916

Comment 23

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

Approved (and discussed with Release Management).

Comment 24

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/0add2d397bc6
https://hg.mozilla.org/releases/mozilla-beta/rev/64374309d490

Comment 25

[Jonathan Kew [:jfkthame]]

What's the status of esr45 at this point? Does landing this in beta45 mean it's automatically going to be fixed for esr45 as well, or do we need to track/fix that separately?

Comment 26

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

As long as it makes it onto 45 (which it did), it is ok for ESR45. People are only marking ESR45 affected on things to make sure, if we fixed it later, that ESR45 doesn't get lost.

Comment 27

[Petruta Horea [:phorea], Desktop Test Engineering]

Reproduced the crash with "Nightly has stopped working" message using 47.0a1 Nightly debug builds under Win 7 64-bit and Ubuntu 14.04 64-bit.

The crash no longer occurs with Firefox 45.0 2016-03-01 and Nightly 47.0a1 2016-03-02.

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

Oops, the needinfo should have been cleared.