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)
Tracking
()
People
(Reporter: decoder, Assigned: valentin)
Details
(Keywords: sec-moderate, Whiteboard: [adv-main31+])
Attachments
(2 files, 1 obsolete file)
3.55 KB,
application/x-x509-ca-cert
|
Details | |
2.85 KB,
patch
|
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
lmandel
:
approval-mozilla-b2g28-
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•10 years ago
|
||
Forgot the certificate ;)
Comment 2•10 years ago
|
||
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).
Updated•10 years ago
|
status-firefox33:
--- → affected
Comment 3•10 years ago
|
||
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)
Updated•10 years ago
|
Component: Security: PSM → Networking
Comment 4•10 years ago
|
||
valentin has been dominating old url parser bugs lately.. maybe he would like this one too?
Flags: needinfo?(mcmanus)
Assignee | ||
Comment 5•10 years ago
|
||
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... )
Updated•10 years ago
|
Keywords: sec-moderate
Assignee | ||
Comment 6•10 years ago
|
||
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 7•10 years ago
|
||
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+
Assignee | ||
Comment 8•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8449208 -
Attachment is obsolete: true
Assignee | ||
Comment 9•10 years ago
|
||
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 10•10 years ago
|
||
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+
Updated•10 years ago
|
status-firefox30:
--- → wontfix
status-firefox31:
--- → affected
status-firefox32:
--- → affected
status-firefox-esr24:
--- → affected
tracking-firefox33:
--- → +
Comment 12•10 years ago
|
||
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?
Updated•10 years ago
|
Attachment #8449516 -
Flags: approval-mozilla-beta?
Attachment #8449516 -
Flags: approval-mozilla-aurora?
Comment 13•10 years ago
|
||
Was looking at the wrong patch (thanks OCSP), please ignore comment 12.
Comment 14•10 years ago
|
||
status-b2g-v1.3:
--- → affected
status-b2g-v1.3T:
--- → affected
status-b2g-v1.4:
--- → affected
status-b2g-v2.0:
--- → affected
status-b2g-v2.1:
--- → affected
Keywords: checkin-needed
Comment 15•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Comment 16•10 years ago
|
||
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)
Assignee | ||
Comment 17•10 years ago
|
||
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 18•10 years ago
|
||
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?
Updated•10 years ago
|
Attachment #8449208 -
Flags: approval-mozilla-beta?
Attachment #8449208 -
Flags: approval-mozilla-b2g30?
Attachment #8449208 -
Flags: approval-mozilla-b2g28?
Attachment #8449208 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 19•10 years ago
|
||
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•10 years ago
|
||
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+
Comment 21•10 years ago
|
||
(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•10 years ago
|
||
I'm noming for 1.4 as per our acceptance process for this branch.
blocking-b2g: --- → 1.4?
Comment 23•10 years ago
|
||
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-
Updated•10 years ago
|
Updated•10 years ago
|
Whiteboard: [adv-main31+]
Comment 24•10 years ago
|
||
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)
Assignee | ||
Comment 25•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
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
Updated•10 years ago
|
Alias: CVE-2014-1559
Comment 28•10 years ago
|
||
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)
Assignee | ||
Comment 29•10 years ago
|
||
Yeah, that seems pretty much what I did too. Thanks!
Flags: needinfo?(valentin.gosu)
Comment 31•10 years ago
|
||
(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?
Comment 32•10 years ago
|
||
Add more Mozilla people
Comment 33•10 years ago
|
||
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)
Assignee | ||
Comment 34•10 years ago
|
||
AFAICT it seems that attachment 8449208 [details] [diff] [review] can be applied without rebasing.
Flags: needinfo?(valentin.gosu)
Comment 35•10 years ago
|
||
Comment 36•10 years ago
|
||
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-
Updated•10 years ago
|
QA Whiteboard: qe-verify-
Flags: qe-verify-
Reporter | ||
Comment 37•10 years ago
|
||
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)
Assignee | ||
Comment 38•10 years ago
|
||
I'll try to write an xpcshell test for this.
Flags: needinfo?(valentin.gosu)
Updated•9 years ago
|
Group: core-security → core-security-release
Updated•9 years ago
|
Group: core-security-release
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(valentin.gosu)
You need to log in
before you can comment on or make changes to this bug.
Description
•