1026022 - (CVE-2014-1559) ###!!! ASSERTION: Not a UTF-8 string. This code should only be used for converting from known UTF-8 strings.: 'Error', file dist/include/nsUTF8Utils.h, line 427

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Christian Holler (:decoder)]

The attached certificate causes the following failure (mozilla-central c482c28b35b6 + patches from bug 1015973):


Running test with PKIX
[3539] WARNING: NS_ENSURE_TRUE(IsUTF8(input)) failed: file netwerk/dns/nsIDNService.cpp, line 293
[3539] ###!!! ASSERTION: Not a UTF-8 string. This code should only be used for converting from known UTF-8 strings.: 'Error', file ../../../dist/include/nsUTF8Utils.h, line 427
[3539] ###!!! ASSERTION: Not a UTF-8 string. This code should only be used for converting from known UTF-8 strings.: 'Error', file ../../../dist/include/nsUTF8Utils.h, line 427
Hit MOZ_CRASH() at memory/mozalloc/mozalloc_abort.cpp:30
ASAN:SIGSEGV
=================================================================
==3539==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f0b9f2a373a sp 0x7fff7c0f56e0 bp 0x7fff7c0f56f0 T0)
    #0 0x7f0b9f2a3739 in mozalloc_abort(char const*) memory/mozalloc/mozalloc_abort.cpp:30
    #1 0x7f0b92641cad in Abort(char const*) xpcom/base/nsDebugImpl.cpp:472
    #2 0x7f0b9264197a in NS_DebugBreak xpcom/base/nsDebugImpl.cpp:459
    #3 0x7f0b9263ff9f in CalculateUTF8Length::write(char const*, unsigned int) dist/include/nsUTF8Utils.h:427
    #4 0x7f0b92623eba in nsCharSinkTraits<CalculateUTF8Length>::write(CalculateUTF8Length&, char const*, unsigned int) dist/include/nsCharTraits.h:572
    #5 0x7f0b92623eba in CalculateUTF8Length& copy_string<nsReadingIterator<char>, CalculateUTF8Length>(nsReadingIterator<char> const&, nsReadingIterator<char> const&, CalculateUTF8Length&) dist/include/nsAlgorithm.h:70
    #6 0x7f0b92623eba in AppendUTF8toUTF16(nsACString_internal const&, nsAString_internal&, mozilla::fallible_t const&) xpcom/string/src/nsReadableUtils.cpp:195
    #7 0x7f0b92623027 in AppendUTF8toUTF16(nsACString_internal const&, nsAString_internal&) xpcom/string/src/nsReadableUtils.cpp:184
    #8 0x7f0b92a0b895 in NS_ConvertUTF8toUTF16 objdir-ff-asan64dbg/netwerk/dns/../../dist/include/nsString.h:169
    #9 0x7f0b92a0b895 in nsIDNService::UTF8toACE(nsACString_internal const&, nsACString_internal&, bool, bool) netwerk/dns/nsIDNService.cpp:170
    #10 0x7f0b92a0790b in nsDNSService::AsyncResolve(nsACString_internal const&, unsigned int, nsIDNSListener*, nsIEventTarget*, nsICancelable**) netwerk/dns/nsDNSService2.cpp:685
    #11 0x7f0b928ff113 in nsDNSPrefetch::Prefetch(unsigned short) netwerk/base/src/nsDNSPrefetch.cpp:61
    #12 0x7f0b92caaf25 in mozilla::net::nsHttpChannel::BeginConnect() netwerk/protocol/http/nsHttpChannel.cpp:4600
    #13 0x7f0b92cabef1 in mozilla::net::nsHttpChannel::OnProxyAvailable(nsICancelable*, nsIURI*, nsIProxyInfo*, tag_nsresult) netwerk/protocol/http/nsHttpChannel.cpp:4710
    #14 0x7f0b929930ae in nsAsyncResolveRequest::DoCallback() netwerk/base/src/nsProtocolProxyService.cpp:247
    #15 0x7f0b929522b8 in nsRefPtr<nsAsyncResolveRequest>::operator->() const netwerk/base/src/nsProtocolProxyService.cpp:139
    #16 0x7f0b929522b8 in nsProtocolProxyService::AsyncResolveInternal(nsIURI*, unsigned int, nsIProtocolProxyCallback*, nsICancelable**, bool) netwerk/base/src/nsProtocolProxyService.cpp:1143
    #17 0x7f0b92c9c276 in mozilla::net::nsHttpChannel::ResolveProxy() netwerk/protocol/http/nsHttpChannel.cpp:1826
    #18 0x7f0b92ca9feb in mozilla::net::nsHttpChannel::AsyncOpen(nsIStreamListener*, nsISupports*) netwerk/protocol/http/nsHttpChannel.cpp:4500
    #19 0x7f0b982054a0 in nsHTTPDownloadEvent::Run() security/manager/ssl/src/nsNSSCallbacks.cpp:157



This issue is similar to bug 1015973 but has a different cause according to comment 4 in that bug. It also reproduces with the patch from that bug.

Marked s-s until investigated and confirmed to be harmless.

Comment 1

[Christian Holler (:decoder)]

Attachment: mutant1542.pem

Forgot the certificate ;)

Comment 2

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Well, it looks like a couple of things are wrong here:

1. nsStdURLParser::ParseURL shouldn't be happy parsing a url like 'http:\257/ocsp.verisign.com' (it ends up saying the host is '\257'
2. nsDNSService::AsyncResolve does this:

683         nsAutoCString hostACE;
684         if (idn && !IsASCII(*hostPtr)) {
685             if (NS_SUCCEEDED(idn->ConvertUTF8toACE(*hostPtr, hostACE)))
686                 hostPtr = &hostACE;
687         }

It probably shouldn't assume that a string is automatically UTF8 if it isn't ASCII.

Also, I discovered plenty of places in nsNSSCertHelper.cpp that make similar mistakes (i.e. assuming something is ASCII or UTF8 without actually checking).

Comment 3

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Patrick - is there someone on the networking team that can work on this? Basically what's happening is PSM is opening a channel for an OCSP request to a url specified in a certificate which is causing problems (see comment 2). (The issues in nsNSSCertHelper.cpp can be dealt with separately.)

Comment 4

[Patrick McManus [:mcmanus]]

valentin has been dominating old url parser bugs lately.. maybe he would like this one too?

Comment 5

[Valentin Gosu [:valentin] (he/him)]

I'm happy to look into the issue.
It's also interesting that clicking on the link  'http:\257/ocsp.verisign.com' and copying it in another tab leads to different behaviours (first goes to \257 the second to www..com/ocsp... )

Comment 6

[Valentin Gosu [:valentin] (he/him)]

Attachment: utf8.patch

This patch fixes the assertion, which is largely due to the fact that GetAsciiHost returns non-ascii/non-utf8 characters.
Is it ok to return an error code if it's not utf8?

Comment 7

[Patrick McManus [:mcmanus]]

Comment on attachment 8449208 [details] [diff] [review]
utf8.patch

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

that seems right. We certainly don't handle any other cases now (I don't believe there are other legitimate cases, but I'm not positive). so fixing the precondition checks on the existing code is the right thing.

Comment 8

[Valentin Gosu [:valentin] (he/him)]

Attachment: Check string is UTF8

Comment 9

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8449516 [details] [diff] [review]
Check string is UTF8

[Security approval request comment]

How easily could an exploit be constructed based on the patch?
Not easily, considering javascript strings are utf16, and the code path isn't obvious.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No. While it's expected that the string should be UTF8, the method of triggering the assertion isn't obvious. 

Which older supported branches are affected by this flaw?
All.

If not all supported branches, which bug introduced the flaw?
In all supported branches. Blame lists Bug 542222 but the code is a lot older than that.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Attachment 8449208 [details] [diff] can be applied to beta and aurora, and 8449516 is rebased on m-c.

How likely is this patch to cause regressions; how much testing does it need?
Unlikely to cause regressions, as there is no relevant change in functionality.
All green on try: https://tbpl.mozilla.org/?tree=Try&rev=10b7b8e82470

Comment 10

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

Comment on attachment 8449516 [details] [diff] [review]
Check string is UTF8

Giving this sec-approval+ but, per guidelines at https://wiki.mozilla.org/Security/Bug_Approval_Process, a sec-moderate doesn't need approval to be checked in. FYI.

Comment 11

[Valentin Gosu [:valentin] (he/him)]

Thanks, Al!

Comment 12

[Camilo Viecco (:cviecco)]

Comment on attachment 8449516 [details] [diff] [review]
Check string is UTF8

Approval Request Comment
[Feature/regressing bug #]:Blame lists Bug 542222 but the code is a lot older than that.
[User impact if declined]: Potential Memory corruption/crash when visiting a site with a specially crafted certificate
[Describe test coverage new/current, TBPL]: Landed In mc for a week
[Risks and why]: This only affects error reporting so the risks are low, even in that case the code has been in M-C for more than a week without any issues.
[String/UUID change made/needed]: None

Comment 13

[Camilo Viecco (:cviecco)]

Was looking at the wrong patch (thanks OCSP), please ignore comment 12.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Marking esr24 as wontfix since sec-moderates in general aren't taken there. Feel free to set it back to affected and request approval if you want to argue the case that it should be. Also, please request Aurora/Beta/b2g30/b2g28 approval on this patch when you get a chance. Thanks!

Comment 17

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8449208 [details] [diff] [review]
utf8.patch

This patch can be applied to the aurora/beta branches.

Approval Request Comment
[Feature/regressing bug #]: pre-existing bug
[User impact if declined]: Potential memory corruption or crash when visiting a site with a specially crafted certificate
[Describe test coverage new/current, TBPL]: On m-c for a few days, passes all unit tests on try. Tested manually with the certificate provided in this bug.
[Risks and why]: Low risk. Only a few lines have been added, checking for the correct encoding of characters.
[String/UUID change made/needed]: None.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8449516 [details] [diff] [review]
Check string is UTF8

Just moving the requests over to the patch that landed rather than the obsolete one.

Comment 19

[Valentin Gosu [:valentin] (he/him)]

Thanks Ryan. I had requested approval for that one because it's the one that can be applied to aurora/beta. Sorry if that's not the correct procedure.

Comment 20

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8449516 [details] [diff] [review]
Check string is UTF8

31 is an ESR. Taking it right now.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

(In reply to Valentin Gosu [:valentin] from comment #19)
> Thanks Ryan. I had requested approval for that one because it's the one that
> can be applied to aurora/beta. Sorry if that's not the correct procedure.

I missed that part, sorry :)

https://hg.mozilla.org/releases/mozilla-aurora/rev/8825a5c848a2
https://hg.mozilla.org/releases/mozilla-beta/rev/a8bd3841c658

Comment 22

[Lawrence Mandel [:lmandel] (use needinfo)]

I'm noming for 1.4 as per our acceptance process for this branch.

Comment 23

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8449516 [details] [diff] [review]
Check string is UTF8

Clearing the b2g30 approval request in favour of the 1.4 nom.

b2g28- as we're past the point of taking changes on 1.3.

Comment 24

[Matt Wobensmith [:mwobensmith][:matt:]]

decoder: How did you cause this to happen? Did you host the cert on a site and point Fx to it? Or do you have another more devious way? We need to verify that this is fixed, and I'm happy to do it if you can tell me how you found it. Thanks.

Comment 25

[Valentin Gosu [:valentin] (he/him)]

The way I did it was to set up a https website using nginx, with the specified certificate, and alias uwl.weblio.jp to localhost, then access it.
I think decoder had a slightly different method, but this one works too.

Comment 26

[Matt Wobensmith [:mwobensmith][:matt:]]

Thanks Valentin.

I was able to add the cert to a local apache instance, set up the hosts file to mimic that cert's domain, and connected successfully to my local site. I had to override an error message, as the cert itself is not trusted, but after that, no crash.

decoder - let me know if there's a better way to reproduce this.

Comment 27

[Kamil Jozwiak [:kjozwiak]]

I've tried going through this using nodejs + express and couldn't reproduce the issue either. Used the cert from comment # 1 and created a localhost using port 8000. Once accessed, I received an error message indicating that the cert wasn't trusted. After that, I couldn't reproduce the crash either.

I'll try using nginx and the STR :valentin posted in comment #25

Comment 28

[Kamil Jozwiak [:kjozwiak]]

I tried the following:

- installed nginx via Ubuntu 14.10
- changed -> ssl_certificate /etc/nginx/ssl/mutant1542.pem; (under /etc/nginx/sites-enabled)
- changed -> ssl_certificate_key /etc/nginx/ssl/mutant1542.pem; (under /etc/nginx/sites-enabled)
- Added "127.0.0.1 uwl.weblio.jp" into /etc/hosts
- Visiting https://uwl.weblio.jp would direct me to an "Untrusted website Exception" page
- Once the certificate was confirmed, https://localhost loaded without any crashes/issues

Valentin, does this seem correct? I've used linux asan builds from around June 12, June 16 and June 25.. Downloaded them from:

http://inbound-archive.pub.build.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan/

Comment 29

[Valentin Gosu [:valentin] (he/him)]

Yeah, that seems pretty much what I did too. Thanks!

Comment 30

[Preeti Raghunath(:Preeti)]

Moving to backlog per comment 23

Comment 31

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to Preeti Raghunath(:Preeti) from comment #30)
> Moving to backlog per comment 23

Preeti, I don't understand this logic. I nomed the bug in comment 22 and cleared the approval request in comment 23. I have renomed so that triage can review.

Comment 32

[Ivan Tsay (:ITsay)]

Add more Mozilla people

Comment 33

[Ivan Tsay (:ITsay)]

Let's take this one in v1.4.

Hi Valentin,

Is it necessary to rebase the patch for 1.4 landing?

Comment 34

[Valentin Gosu [:valentin] (he/him)]

AFAICT it seems that attachment 8449208 [details] [diff] [review] can be applied without rebasing.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/3831045422d6

Comment 36

[Matt Wobensmith [:mwobensmith][:matt:]]

Setting qe-verify- due to lack of ability to reproduce issue. Feel free to ask for more help in verification if you are able to provide us with more information. See comment 26 and comment 28. Thank you.

Comment 37

[Christian Holler (:decoder)]

I just saw this old needinfo for verification. This is a typical example for a bug that should not require manual verification at all. There is a simple testcase attached, which can be used in an xpcshell test to verify that the assertion does not happen anymore. You don't even need to add a complicated full-browser test for this. I suggest the developers add this test as a regression test.

Comment 38

[Valentin Gosu [:valentin] (he/him)]

I'll try to write an xpcshell test for this.