1015973 - (CVE-2014-1558) ###!!! 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 422

Status: VERIFIED FIXED

(Core :: Security: PSM, defect)

Description

[Christian Holler (:decoder)]

Attachment: mutant582.pem

The attached certificate causes the following failure (mozilla-central bf98b86fbea2):


Running test with PKIX
System JS : ERROR (null):0 - uncaught exception: 2147500037
[11681] ###!!! 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 422
[11681] ###!!! 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 422
Hit MOZ_CRASH() at memory/mozalloc/mozalloc_abort.cpp:30
ASAN:SIGSEGV
=================================================================
==11681==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f3447e4a73a sp 0x7fff6bc6af00 bp 0x7fff6bc6af10 T0)
    #0 0x7f3447e4a739 in mozalloc_abort(char const*) memory/mozalloc/mozalloc_abort.cpp:30
    #1 0x7f343b4f26ad in Abort(char const*) xpcom/base/nsDebugImpl.cpp:435
    #2 0x7f343b4f237a in NS_DebugBreak xpcom/base/nsDebugImpl.cpp:422
    #3 0x7f343b4f099f in CalculateUTF8Length::write(char const*, unsigned int) objdir-ff-asan64dbg/xpcom/string/src/../../../dist/include/nsUTF8Utils.h:422
    #4 0x7f343b4d4a4a in nsCharSinkTraits<CalculateUTF8Length>::write(CalculateUTF8Length&, char const*, unsigned int) objdir-ff-asan64dbg/xpcom/string/src/../../../dist/include/nsCharTraits.h:587
    #5 0x7f343b4d4a4a in CalculateUTF8Length& copy_string<nsReadingIterator<char>, CalculateUTF8Length>(nsReadingIterator<char> const&, nsReadingIterator<char> const&, CalculateUTF8Length&) objdir-ff-asan64dbg/xpcom/string/src/../../../dist/include/nsAlgorithm.h:73
    #6 0x7f343b4d4a4a in AppendUTF8toUTF16(nsACString_internal const&, nsAString_internal&, mozilla::fallible_t const&) xpcom/string/src/nsReadableUtils.cpp:196
    #7 0x7f343b4d3bb7 in AppendUTF8toUTF16(nsACString_internal const&, nsAString_internal&) xpcom/string/src/nsReadableUtils.cpp:185
    #8 0x7f343b4d3e8b in AppendUTF8toUTF16(char const*, nsAString_internal&) xpcom/string/src/nsReadableUtils.cpp:243
    #9 0x7f3440e1e2d8 in NS_ConvertUTF8toUTF16 objdir-ff-asan64dbg/security/manager/ssl/src/../../../../dist/include/nsString.h:158
    #10 0x7f3440e1e2d8 in mozilla::RefPtr<nsIX509Cert>::operator nsIX509Cert*() const security/manager/ssl/src/TransportSecurityInfo.cpp:795
    #11 0x7f3440e1e2d8 in mozilla::psm::formatOverridableCertErrorMessage(nsISSLStatus&, int, nsXPIDLCString const&, int, bool, bool, nsString&) security/manager/ssl/src/TransportSecurityInfo.cpp:1010
    #12 0x7f3440e1e2d8 in mozilla::psm::TransportSecurityInfo::formatErrorMessage(mozilla::BaseAutoLock<mozilla::Mutex> const&, int, mozilla::psm::SSLErrorMessageType, bool, bool, nsString&) security/manager/ssl/src/TransportSecurityInfo.cpp:249
    #13 0x7f3440e20611 in mozilla::psm::TransportSecurityInfo::GetErrorLogMessage(int, mozilla::psm::SSLErrorMessageType, nsString&) security/manager/ssl/src/TransportSecurityInfo.cpp:209
    #14 0x7f3440e5931b in mozilla::RefPtr<mozilla::psm::TransportSecurityInfo>::operator mozilla::psm::TransportSecurityInfo*() const security/manager/ssl/src/SSLServerCertVerification.cpp:223
    #15 0x7f3440e5931b in mozilla::psm::(anonymous namespace)::CertErrorRunnable::CheckCertOverrides() security/manager/ssl/src/SSLServerCertVerification.cpp:536
    #16 0x7f3440e5931b in mozilla::psm::(anonymous namespace)::CertErrorRunnable::RunOnTargetThread() security/manager/ssl/src/SSLServerCertVerification.cpp:548
    #17 0x7f3440e13f93 in mozilla::psm::SyncRunnableBase::Run() security/manager/ssl/src/PSMRunnable.cpp:35


When running without aborting, I also hit this assertion afterwards:


###!!! ASSERTION: Input wasn't UTF8 or incorrect length was calculated: 'Error', file xpcom/string/src/nsReadableUtils.cpp, line 222


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

Comment 1

[Christian Holler (:decoder)]

I assume this is a broken UTF8String:

  240:d=2  hl=3 l= 128 cons: SEQUENCE          
  243:d=3  hl=2 l=  56 cons: SET               
  245:d=4  hl=2 l=  54 cons: SEQUENCE          
  247:d=5  hl=2 l=   3 prim: OBJECT            :commonName
  252:d=5  hl=2 l=  47 prim: UTF8STRING        :EBG Elektronik Sertifika Hizmet Sa��layıcısı


I also get the second assertion for another certificate that has broken symbols in the OSCP URI. I assume this is the same bug, if that's not the case, please let me know and I'll file separately.

Comment 2

[Daniel Veditz [:dveditz]]

We should not expect strings in a certificate to be UTF-8. There's no standard and I would guess that it's probably the Windows character set for whatever locale the requestor and/or CA used.

Domain names we should require to be in punycode form. I'm not sure all CA's do that, but since multiple IDNA names can map to the same punycode DNS host we should stick to that one form and not try to guess which character set was intended. If we guess wrong an attacker could take advantage of the difference in interpretation between the CA and Firefox to get a fraudulent cert.

Comment 3

[Camilo Viecco (:cviecco)]

This was introduced on Bug 997795 where this same issue was improved (we previusly barfed on valid utf8) but not fixed completely. After talking to dveditz (in real life) we decided to just present a simpler error when the string is not even ascii.

Comment 4

[Camilo Viecco (:cviecco)]

Decoder, the ocsp invalid goes via another code path. Can you file a new bug for that? thanks.

Comment 5

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

(In reply to Camilo Viecco (:cviecco) from comment #3)
> This was introduced on Bug 997795 where this same issue was improved (we
> previusly barfed on valid utf8) but not fixed completely. After talking to
> dveditz (in real life) we decided to just present a simpler error when the
> string is not even ascii.

Unless you can fix this right away by doing the "simpler error" thing, please back out the patch for bug 997795 from Gecko 31 and Gecko 32. It looks to me like the new code in that patch is potentially more dangerous than the code that replaced. Bug 997795 is really not a big deal anyway.

Comment 6

[Camilo Viecco (:cviecco)]

Attachment: fix-not-decode-invalid

Comment 7

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

Comment on attachment 8430852 [details] [diff] [review]
fix-not-decode-invalid

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

::: security/manager/ssl/src/TransportSecurityInfo.cpp
@@ +799,5 @@
> +      if (IsASCII(commonName)) {
> +        ++nameCount;
> +        //AppendASCIItoUTF16(commonName, allNames);
> +        allNames.Assign(NS_ConvertASCIItoUTF16(commonName));
> +      }

I looked at the code for CERT_GetCommonName and AFAICT it tries to convert all non-UTF-8 encodings to UTF-8. But, it also seems like if the input data *claims* to be UTF-8 encoded, then it just returns the input data without verifying it really is UTF-8 encoded. Thus, it seems to me that rather than checking IsASCII() we should really be doing some IsUTF8() check, right? In other words, it seems like your initial assumption that the result will never be anything but UTF-8 encoded was correct (and my assumption about this in the previous bug was wrong), but we still need to check that the UTF-8 encoding is actually a valid UTF-8 encoding. That is, instead of using a function that *assumes* valid UTF-8, we need to use a function that *verifies* valid UTF-8.

The next problem is that IsASCII(commonName) may be true even if commonName is not a syntactically-correct hostname or IP address. If we interpret this bug narrowly to be "get rid of the invalid UTF-8 assertion" then IsASCII seems like the wrong solution because it is overly strict. If we interpret this bug more broadly to be about us trying to show names that would never be matched in a call to CERT_VerifyCertName then IsASCII is not strict enough. Either way, it seems like IsASCII isn't the best test to be using here.

It isn't clear to me from the patch why we would consider it OK to just skip a unicode-encoded hostname that appears in the certificate. Is it because RFC 5280 requires international hostnames to be punycode-encoeded? If so, let's reference the relevent section of the RFC in a comment in this code.

The next problem is that the same, or worse, sort of encoding issues also seem to affect GetSubjectAltNames, right? Why are we trying to carefully check the encoding of the common name without also carefully checking the encoding of the SubjectAltName? (Note that it isn't clear whether SubjectAltName is dealing with supposedly-UTF-8-encoded strings or not.)

The next problem is that, if I am reading CERT_GetCommonName correctly, it will *never* return an embedded null character, because it escapes them. Thus, the comment doesn't make sense. (Note that it isn't clear to me whether SubjectAltName is dealing with escaped strings or not, because I didn't look into how CERT_GeneralNames are constructed--in particular, whether they are normalized to UTF-8 or not.)

The next problem I see has to do with the way we are presenting non-ASCII names, whether they are punycode-encoded within the cert or whether they are not in punycode in the certificate. We have rules about when we show punycode to users and when we show the decoded Unicode, based on what languages the user claims to understand. Our error messages that include hostnames and/or URIs should be following those rules. If you want to consider that out of scope of this bug, that's find, but let's file a follow-up bug for fixing this along with a comment in the code referencing that bug.

Finally, note that NSS defines a function CERT_GetValidDNSPatternsFromCert which is very much like this code and which seems to suffer from similar issues--e.g. not checking whether dNSNames or IP Addresses are syntactically valid or not, especially in the case of the common name. Whatever bugs we've found in this code should be filed against CERT_GetValidDNSPatternsFromCert too.

The main thing I'm looking for in an updated patch is some comment justifying the logic used to decide whether to include a hostname in the output (and/or improved logic), and consistency between how the encoding issues are dealt with respect to the subject common name and subjectAltName, along with some tests. The rest of the issues I mentioned can all be deferred to follow-up bugs.

Comment 8

[Camilo Viecco (:cviecco)]

Attachment: fix-not-decode-invalid

Comment 9

[Camilo Viecco (:cviecco)]

Comment on attachment 8437282 [details] [diff] [review]
fix-not-decode-invalid

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

Reducing the scope of this bug to be:
Only add the common name if it not only claims to be utf8 but if it is actually uft8.
not caring about the user viewable things.

Comment 10

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

Comment on attachment 8437282 [details] [diff] [review]
fix-not-decode-invalid

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

Looks good. However, are we doing to have a similar problem at https://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/TransportSecurityInfo.cpp#627 ? (And, indeed, anywhere we get data from a CERTCertificate and interpret it as UTF-8 or ASCII?)

Comment 11

[Camilo Viecco (:cviecco)]

Comment on attachment 8437282 [details] [diff] [review]
fix-not-decode-invalid

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

This is an enhancement and affects FF31 (tomorrow in beta). Any issues?

Comment 12

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

You need to fill out the template that comes up when you set sec-approval?. This also needs a security rating.

Comment 13

[Camilo Viecco (:cviecco)]

Attachment: fix-not-decode-invalid (v2)

Since we are fixing this and pushing upstream lets fix all similar issues in this file.

Comment 14

[Camilo Viecco (:cviecco)]

Comment on attachment 8439518 [details] [diff] [review]
fix-not-decode-invalid (v2)

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

When writing the uplift comment, I noticed a similar issue here. Lets squash these two together.

Comment 15

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

Comment on attachment 8439518 [details] [diff] [review]
fix-not-decode-invalid (v2)

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

Please file the follow-up bug about verifying that the common name is actually an IPv4/IPv6 address or a valid hostname when deciding to output it.

::: security/manager/ssl/src/TransportSecurityInfo.cpp
@@ +628,3 @@
>      switch (current->type) {
>        case certDNSName:
> +        if (IsASCII(nameFromCert)) {

IsUTF8?

Comment 16

[Camilo Viecco (:cviecco)]

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #15)
> Comment on attachment 8439518 [details] [diff] [review]
> fix-not-decode-invalid (v2)
> 
> Review of attachment 8439518 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please file the follow-up bug about verifying that the common name is
> actually an IPv4/IPv6 address or a valid hostname when deciding to output it.
> 
> ::: security/manager/ssl/src/TransportSecurityInfo.cpp
> @@ +628,3 @@
> >      switch (current->type) {
> >        case certDNSName:
> > +        if (IsASCII(nameFromCert)) {
> 
> IsUTF8?

From RFC 5280 section 7.2 ( Internationalized Domain Names in GeneralName)
   Internationalized Domain Names (IDNs) may be included in certificates
   and CRLs in the subjectAltName and issuerAltName extensions, name
   constraints extension, authority information access extension,
   subject information access extension, CRL distribution points
   extension, and issuing distribution point extension.  Each of these
   extensions uses the GeneralName type; one choice in GeneralName is
   the dNSName field, which is defined as type IA5String.

An IA5String limites to ASCII characters, that is why IsASCII should be the
correct thing to do.

Comment 17

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

Comment on attachment 8439518 [details] [diff] [review]
fix-not-decode-invalid (v2)

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

Looks good. r=me with comments addressed.

::: security/manager/ssl/src/TransportSecurityInfo.cpp
@@ +622,5 @@
>  
>    CERTGeneralName *current = sanNameList;
>    do {
>      nsAutoString name;
> +    nsDependentCSubstring nameFromCert((char*)current->name.other.data,

nit: reinterpret_cast<char*>(current->name.other.data)
Also, since this is only used in the certDNSName branch of the switch, let's put it there (I think you'll have to add extra scope {})

@@ +628,3 @@
>      switch (current->type) {
>        case certDNSName:
> +        if (IsASCII(nameFromCert)) {

Let's add a comment here about this.

@@ +716,5 @@
>      if (certName) {
> +      nsDependentCSubstring commonName(certName, strlen(certName));
> +      if (IsUTF8(commonName)) {
> +        // Currently we dont care if this is an actual common name or
> +        // a valid Host identifier.

This comment isn't clear to me - by "host identifier" do you mean domain name?
Also, please be sure to reference the bug filed about fixing this here.

Comment 18

[Camilo Viecco (:cviecco)]

> 
> nit: reinterpret_cast<char*>(current->name.other.data)
> Also, since this is only used in the certDNSName branch of the switch, let's
> put it there (I think you'll have to add extra scope {})

That is how I initially implemented but you cannot create a new identifier inside
a switch statement. So I will address all your comments but this one.

Comment 19

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

(In reply to Camilo Viecco (:cviecco) from comment #18)
> > Also, since this is only used in the certDNSName branch of the switch, let's
> > put it there (I think you'll have to add extra scope {})
> 
> That is how I initially implemented but you cannot create a new identifier
> inside
> a switch statement. So I will address all your comments but this one.

http://stackoverflow.com/questions/92396/why-cant-variables-be-declared-in-a-switch-statement

Comment 20

[Camilo Viecco (:cviecco)]

Attachment: fix-not-decode-invalid (v2.1)

keeping r+ from keeler

Comment 21

[Camilo Viecco (:cviecco)]

Comment on attachment 8440106 [details] [diff] [review]
fix-not-decode-invalid (v2.1)

[Security approval request comment]
How easily could an exploit be constructed based on the patch? It is difficult, as it only show where we are not decoding things properly.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Yes, we are introducing checks for data types that were not before.

Which older supported branches are affected by this flaw?
All of the current branches. The bug was initially introduced on bug 401755 landing on the tree 2007-11-12.

If not all supported branches, which bug introduced the flaw?

Do you have back-ports for the affected branches? If not, how different, hard to create, and risky will they be? 
Not yet, there will be a one line difference between (32-33) and the rest of the branches.


How likely is this patch to cause regressions; how much testing does it need?
Not likely, this patch happens on the error path during name mismatch. For regression, just testing that visiting to a site with an invalid name (name mistmatch) the error message contains the names of the certificate.

Comment 22

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

This needs to have a security rating before it can be approved. What is the severity or implication of this issue?

Comment 23

[Camilo Viecco (:cviecco)]

Error rating: Sec moderate: we are not handling an error condition correctly as we are not validating inputs to match the specified type for converting into a string that is later used to display an error message.

Comment 24

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

Comment on attachment 8440106 [details] [diff] [review]
fix-not-decode-invalid (v2.1)

As a sec-moderate, it doesn't really need sec-approval now but I'm giving it anyway.

Comment 25

[Camilo Viecco (:cviecco)]

try push with noise
 https://tbpl.mozilla.org/?tree=Try&rev=8cb492a6f87e

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1e86f2c32f56

Comment 27

[Camilo Viecco (:cviecco)]

Comment on attachment 8440106 [details] [diff] [review]
fix-not-decode-invalid (v2.1)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 401755 and 997795 
User impact if declined: Crash or memory corruption with specially crafted certificates.
Testing completed (on m-c, etc.): Landed on central on 2014-06-19.
Risk to taking this patch (and alternatives if risky): None
String or IDL/UUID changes made by this patch: None

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/85b450997e1f
https://hg.mozilla.org/releases/mozilla-beta/rev/44872d625d21

Please nominate this for b2g28 and b2g30 approval as well.

Comment 29

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

I was never able to reproduce the crash, despite multiple builds and platforms. However, I can see similar asserts in affected builds. Also, I can see the URL string in the SSL error page displayed incorrectly when using those builds.

In Fx31 and Fx32, as of latest builds, the asserts are gone and I see a nicer error: "The certificate is not valid for any server names."

In Fx33, today's build, I see "sec_error_unknown_critical_extension" instead.

I don't understand the change in behavior for Fx33, but I am marking verified on all branches. Someone can feel free to chime in if they'd like to explain the different error message in Fx33.