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

Status: VERIFIED FIXED

(Core :: Security: PSM, defect)

Description

[Christian Holler (:decoder)]

Attachment: 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.

Comment 1

[Camilo Viecco (:cviecco)]

Attachment: cleanup-names

Subject names are not limited to ASCII, they can be in general UTF8

Comment 2

[Camilo Viecco (:cviecco)]

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

Comment 3

[Christian Holler (:decoder)]

Just tried the patch, and it fixes the problem for me, thanks :)

Comment 4

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

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));'

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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.

Comment 6

[Camilo Viecco (:cviecco)]

Attachment: cleanup-names (v2)

Keeping r+ from keeler

Comment 7

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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".)

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Comment 10

[Camilo Viecco (:cviecco)]

(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.

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 12

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

How did this even go in without a security rating? Was this 31 only?

Comment 13

[Christian Holler (:decoder)]

Deferring to cviecco who fixed this :)

Comment 14

[Camilo Viecco (:cviecco)]

This was further fixed by https://bugzilla.mozilla.org/show_bug.cgi?id=1015973 which was rated sec-moderate

Comment 15

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

My questions weren't really answered. Does the exposure and rating match bug 1015973?

Comment 16

[Camilo Viecco (: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

Comment 17

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

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.

Comment 18

[Christian Holler (:decoder)]

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 :)

Comment 19

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

Super, thank you decoder!