Bug 997795 (CVE-2014-1560)

###!!! ASSERTION: Unexpected non-ASCII character: '!(*s2 & ~0x7F)', file ../../../dist/include/nsCharTraits.h, line 167

VERIFIED FIXED in Firefox 31

Status

()

defect
VERIFIED FIXED
5 years ago
3 years ago

People

(Reporter: decoder, Assigned: cviecco)

Tracking

(Blocks 1 bug, {sec-moderate})

Trunk
mozilla31
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox30 wontfix, firefox31 verified, firefox-esr24 wontfix)

Details

(Whiteboard: [adv-main31+])

Attachments

(2 attachments, 1 obsolete attachment)

Posted file test1.pem
The attached certificate causes the following failure (mozilla-central b735e618c2a8):


 0:03.18 [21808] ###!!! ASSERTION: Unexpected non-ASCII character: '!(*s2 & ~0x7F)', file ../../../dist/include/nsCharTraits.h, line 167
 0:03.18 Hit MOZ_CRASH() at memory/mozalloc/mozalloc_abort.cpp:30
 0:03.18 ASAN:SIGSEGV
 0:03.18 =================================================================
 0:03.18 ==21808==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7fe6defea73a sp 0x7ffffa507820 bp 0x7ffffa507830 T0)
 0:03.20     #0 0x7fe6defea739 in mozalloc_abort(char const*) memory/mozalloc/mozalloc_abort.cpp:30
 0:04.74     #1 0x7fe6d2a306cd in Abort(char const*) xpcom/base/nsDebugImpl.cpp:421
 0:04.74     #2 0x7fe6d2a3039a in NS_DebugBreak xpcom/base/nsDebugImpl.cpp:408
 0:04.74     #3 0x7fe6d2a2d224 in nsCharTraits<char16_t>::copyASCII(char16_t*, char const*, unsigned long) objdir-ff-asan64dbg/xpcom/string/src/../../../dist/include/nsCharTraits.h:167
 0:04.74     #4 0x7fe6d2a20260 in nsAString_internal::AssignASCII(char const*, unsigned int, mozilla::fallible_t const&) xpcom/string/src/nsTSubstring.cpp:351
 0:04.74     #5 0x7fe6d2a201da in nsAString_internal::AssignASCII(char const*, unsigned int) xpcom/string/src/nsTSubstring.cpp:332
 0:04.74     #6 0x7fe6d7fc6990 in nsAString_internal::AssignASCII(char const*) objdir-ff-asan64dbg/security/manager/ssl/src/../../../../dist/include/nsTSubstring.h:380
 0:04.74     #7 0x7fe6d7fc6990 in mozilla::RefPtr<nsIX509Cert>::operator nsIX509Cert*() const security/manager/ssl/src/TransportSecurityInfo.cpp:795
 0:04.74     #8 0x7fe6d7fc6990 in mozilla::psm::formatOverridableCertErrorMessage(nsISSLStatus&, int, nsXPIDLCString const&, int, bool, bool, nsString&) security/manager/ssl/src/TransportSecurityInfo.cpp:1010
 0:04.74     #9 0x7fe6d7fc6990 in mozilla::psm::TransportSecurityInfo::formatErrorMessage(mozilla::BaseAutoLock<mozilla::Mutex> const&, int, mozilla::psm::SSLErrorMessageType, bool, bool, nsString&) security/manager/ssl/src/TransportSecurityInfo.cpp:249
 0:04.74     #10 0x7fe6d7fc8d23 in mozilla::psm::TransportSecurityInfo::GetErrorLogMessage(int, mozilla::psm::SSLErrorMessageType, nsString&) security/manager/ssl/src/TransportSecurityInfo.cpp:209
 0:04.74     #11 0x7fe6d80012bf in mozilla::RefPtr<mozilla::psm::TransportSecurityInfo>::operator mozilla::psm::TransportSecurityInfo*() const security/manager/ssl/src/SSLServerCertVerification.cpp:223
 0:04.74     #12 0x7fe6d80012bf in mozilla::psm::(anonymous namespace)::CertErrorRunnable::CheckCertOverrides() security/manager/ssl/src/SSLServerCertVerification.cpp:536
 0:04.74     #13 0x7fe6d80012bf in mozilla::psm::(anonymous namespace)::CertErrorRunnable::RunOnTargetThread() security/manager/ssl/src/SSLServerCertVerification.cpp:548
 0:04.74     #14 0x7fe6d7fbcad1 in mozilla::psm::SyncRunnableBase::Run() security/manager/ssl/src/PSMRunnable.cpp:35
 0:04.74     #15 0x7fe6d2b5dff5 in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:699
 0:04.75     #16 0x7fe6d2b7e2c9 in NS_InvokeByIndex xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:162
 0:04.75     #17 0x7fe6d56d85fe in CallMethodHelper::Invoke() js/xpconnect/src/XPCWrappedNative.cpp:2405
 0:04.75     #18 0x7fe6d56d85fe in CallMethodHelper::Call() js/xpconnect/src/XPCWrappedNative.cpp:1746
 0:04.75     #19 0x7fe6d569d45a in XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) js/xpconnect/src/XPCWrappedNative.cpp:1713:12
 0:04.75     #20 0x7fe6d56a1f74 in XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1285
 0:04.75     #21 0x7fe6c412385b (+0x185b)
 0:04.75
 0:04.75 AddressSanitizer can not provide additional info.
 0:04.75 SUMMARY: AddressSanitizer: SEGV memory/mozalloc/mozalloc_abort.cpp:30 mozalloc_abort(char const*)



Marked s-s until investigated and confirmed to be harmless.
Assignee: nobody → cviecco
Posted patch cleanup-names (obsolete) — Splinter Review
Subject names are not limited to ASCII, they can be in general UTF8
decoder, here is a try build. I am unable to easily replicate. Does this try build or patch work for you?

https://tbpl.mozilla.org/?tree=Try&rev=e4f97bfcb61b
Just tried the patch, and it fixes the problem for me, thanks :)
Attachment #8408376 - Flags: review?(dkeeler)
Comment on attachment 8408376 [details] [diff] [review]
cleanup-names

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

r=me with comment addressed.

::: security/manager/ssl/src/TransportSecurityInfo.cpp
@@ +791,5 @@
>      if (!certName)
>        certName = CERT_GetCommonName(&nssCert->subject);
>      if (certName) {
>        ++nameCount;
> +      allNames = NS_ConvertUTF8toUTF16(certName);

How about 'allNames.Assign(NS_ConvertUTF8toUTF16(certName));'
Attachment #8408376 - Flags: review?(dkeeler) → review+
Comment on attachment 8408376 [details] [diff] [review]
cleanup-names

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

::: security/manager/ssl/src/TransportSecurityInfo.cpp
@@ +791,5 @@
>      if (!certName)
>        certName = CERT_GetCommonName(&nssCert->subject);
>      if (certName) {
>        ++nameCount;
> +      allNames = NS_ConvertUTF8toUTF16(certName);

How do you know the encoding of certName is UTF-8? IIRC, there are several possible encodings and it isn't clear how to know which one it is.

Also, if certName is the result of CERT_GetCommonName then it might not even be a domain name, in which case we shouldn't display it.

Internally, when we match names, aren't we doing the name matching in CERT_on the assumption that the names are ASCII? See CERT_VerifyCertName and cert_TestHostName. If so then it seems wrong to be decoding UTF-8 here.
Keeping r+ from keeler
Attachment #8408376 - Attachment is obsolete: true
Attachment #8408556 - Flags: review+
Comment on attachment 8408322 [details]
test1.pem

Looking at the certificate, there is not subjectAltName and the CN is not a domain name or an IPv4 or IPv6 address; it's "CN=AC Raíz Certicámara S.A.". Thus, it shouldn't be listed as a domain name in this message. (The full subject name is "CN=AC Raíz Certicámara S.A.;O=Sociedad Cameral de Certificación Digital - Certicámara S.A.;C=CO".)
Attachment #8408322 - Attachment mime type: application/x-x509-ca-cert → application/pkix-cert
Comment on attachment 8408556 [details] [diff] [review]
cleanup-names (v2)

AFAICT, this patch is not correct for the reasons I gave above: (1) It isn't any more correct to assume UTF-8 than it was to assume ASCII, AFAICT; (2) We shouldn't be assuming the common name is a domain name or IP address without testing it. Also, (3) in the case of a certificate that is not valid for any domain names, we shouldn't allow the cert error override UI.
Attachment #8408556 - Flags: review-
https://hg.mozilla.org/mozilla-central/rev/ba276673a564
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #8)
> Comment on attachment 8408556 [details] [diff] [review]
> cleanup-names (v2)
> 
> AFAICT, this patch is not correct for the reasons I gave above: (1) It isn't
> any more correct to assume UTF-8 than it was to assume ASCII, AFAICT; (2) We
> shouldn't be assuming the common name is a domain name or IP address without
> testing it. Also, (3) in the case of a certificate that is not valid for any
> domain names, we shouldn't allow the cert error override UI.


Thanks for your time an insight brian. I is always refreshing. Unfortunately
I only say your feedback after I had landed this to central.

Let me reply by parts
1. In this case we are reading the subject's common name, it is more correct to assume
it utf8 than ascii, as utf8 encodings (the rfc states that conforming CAs MUST use
PrintableString or UTF8String encoding) so it it better, Not perfect. I think 
this as an intermediate step is a good change.
2. Agreed
3. Agreed with you

So I propose: let this patch stay on central (and be uplifted to aurora/beta) and
file a new bug to address issues 2 and 3( to improve UI errors on
bad invalid cert domain  (we might need a new i10n string?)). 
We can also reopen this.
There's no sec rating here and it's not obvious how far back this goes - is ESR24 affected and does this need to be uplifted? Keep in mind that FF31 (where this is fixed) is the next ESR version.
How did this even go in without a security rating? Was this 31 only?
Flags: needinfo?(choller)
Deferring to cviecco who fixed this :)
Flags: needinfo?(cviecco)
This was further fixed by https://bugzilla.mozilla.org/show_bug.cgi?id=1015973 which was rated sec-moderate
Flags: needinfo?(cviecco)
My questions weren't really answered. Does the exposure and rating match bug 1015973?
Flags: needinfo?(choller) → needinfo?(cviecco)
How did this even go in without a security rating? 
Lack of process awareness by me mostly.

Was this 31 only?
No it has been in the code since 2008.

Does the exposure and rating match bug 1015973?
Yes
Flags: needinfo?(cviecco)
Keywords: sec-moderate
Whiteboard: [adv-main31+]
decoder, how did you load this cert? I've tried hosting it on a site as well as manually importing it, but neither causes a crash, only errors. 

If you have instructions on how to verify, I'd be happy to do that. Or if you can easily verify the fix yourself, that would be good too. Either way. Thanks.
Flags: needinfo?(choller)
Hm. I think your way to verify should have worked. Nevertheless, I can safely mark this verified because my fuzzer used to hit it all the time and it has been gone since the fix landed :)
Status: RESOLVED → VERIFIED
Flags: needinfo?(choller)
Alias: CVE-2014-1560
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.