Closed Bug 1026022 (CVE-2014-1559) Opened 10 years ago Closed 10 years ago

###!!! 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

Categories

(Core :: Networking, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33
blocking-b2g 1.4+
Tracking Status
firefox30 --- wontfix
firefox31 --- fixed
firefox32 --- fixed
firefox33 + fixed
firefox-esr24 --- wontfix
b2g-v1.3 --- wontfix
b2g-v1.3T --- wontfix
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: decoder, Assigned: valentin)

Details

(Keywords: sec-moderate, Whiteboard: [adv-main31+])

Attachments

(2 files, 1 obsolete file)

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.
Attached file mutant1542.pem —
Forgot the certificate ;)
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).
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.)
Flags: needinfo?(mcmanus)
Component: Security: PSM → Networking
valentin has been dominating old url parser bugs lately.. maybe he would like this one too?
Flags: needinfo?(mcmanus)
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... )
Attached patch utf8.patch (obsolete) — — Splinter Review
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?
Assignee: nobody → valentin.gosu
Attachment #8449208 - Flags: review?(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.
Attachment #8449208 - Flags: review?(mcmanus) → review+
Attached patch Check string is UTF8 — — Splinter Review
Attachment #8449208 - Attachment is obsolete: true
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
Attachment #8449516 - Flags: sec-approval?
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.
Attachment #8449516 - Flags: sec-approval? → sec-approval+
Thanks, Al!
Keywords: checkin-needed
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
Attachment #8449516 - Flags: approval-mozilla-beta?
Attachment #8449516 - Flags: approval-mozilla-aurora?
Attachment #8449516 - Flags: approval-mozilla-beta?
Attachment #8449516 - Flags: approval-mozilla-aurora?
Was looking at the wrong patch (thanks OCSP), please ignore comment 12.
https://hg.mozilla.org/mozilla-central/rev/9c65f4d62c92
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
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!
Flags: needinfo?(valentin.gosu)
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.
Attachment #8449208 - Flags: approval-mozilla-beta?
Attachment #8449208 - Flags: approval-mozilla-b2g30?
Attachment #8449208 - Flags: approval-mozilla-b2g28?
Attachment #8449208 - Flags: approval-mozilla-aurora?
Flags: needinfo?(valentin.gosu)
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.
Attachment #8449516 - Flags: approval-mozilla-beta?
Attachment #8449516 - Flags: approval-mozilla-b2g30?
Attachment #8449516 - Flags: approval-mozilla-b2g28?
Attachment #8449516 - Flags: approval-mozilla-aurora?
Attachment #8449208 - Flags: approval-mozilla-beta?
Attachment #8449208 - Flags: approval-mozilla-b2g30?
Attachment #8449208 - Flags: approval-mozilla-b2g28?
Attachment #8449208 - Flags: approval-mozilla-aurora?
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 on attachment 8449516 [details] [diff] [review]
Check string is UTF8

31 is an ESR. Taking it right now.
Attachment #8449516 - Flags: approval-mozilla-beta?
Attachment #8449516 - Flags: approval-mozilla-beta+
Attachment #8449516 - Flags: approval-mozilla-aurora?
Attachment #8449516 - Flags: approval-mozilla-aurora+
(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
I'm noming for 1.4 as per our acceptance process for this branch.
blocking-b2g: --- → 1.4?
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.
Attachment #8449516 - Flags: approval-mozilla-b2g30?
Attachment #8449516 - Flags: approval-mozilla-b2g28?
Attachment #8449516 - Flags: approval-mozilla-b2g28-
Whiteboard: [adv-main31+]
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.
Flags: needinfo?(choller)
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.
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.
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
Alias: CVE-2014-1559
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/
Flags: needinfo?(valentin.gosu)
Yeah, that seems pretty much what I did too. Thanks!
Flags: needinfo?(valentin.gosu)
Moving to backlog per comment 23
blocking-b2g: 1.4? → backlog
(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.
blocking-b2g: backlog → 1.4?
Add more Mozilla people
Let's take this one in v1.4.

Hi Valentin,

Is it necessary to rebase the patch for 1.4 landing?
blocking-b2g: 1.4? → 1.4+
Flags: needinfo?(valentin.gosu)
AFAICT it seems that attachment 8449208 [details] [diff] [review] can be applied without rebasing.
Flags: needinfo?(valentin.gosu)
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.
QA Whiteboard: qe-verify-
QA Whiteboard: qe-verify-
Flags: qe-verify-
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.
Flags: needinfo?(choller)
I'll try to write an xpcshell test for this.
Flags: needinfo?(valentin.gosu)
Group: core-security → core-security-release
Group: core-security-release
Flags: needinfo?(valentin.gosu)
You need to log in before you can comment on or make changes to this bug.