585616 - No longer allow IP addresses in SSL server's common names

Status: RESOLVED DUPLICATE of bug 1245280

(Core :: Security: PSM, defect, P1)

Description

[Kai Engert [:KaiE:]]

RFC 2818 states that IP addresses might be allowed when verifying a server identity, but the address MUST be in the subject alt name, which means, it's not valid for an IP address to be listed in a certificate's common name (CN) field.

However, as of today, the NSS implementation incorrectly allows CN = IP.

We would like to change Mozilla SSL client behaviour to conform to RFC 2818.

This has already been proposed earlier, in bug 553754.

However, we are worried that some intranet environments might depend on this feature.

Because of this, we believe it's not (yet) appropriate to change the policy inside NSS, which would change the behaviour of any products updating to a newer NSS version as part of a security update.

Therefore we'd like to start slowly, by making this change outside of NSS, at the Mozilla client level, in PSM code. This way Firefox 4 can have the new policy, while older Firefox versions keep their old behaviour.

I'll attach a patch.

Comment 1

[Kai Engert [:KaiE:]]

Attachment: Patch v1

Initial patch.

Comment 2

[Kai Engert [:KaiE:]]

Should we add a section to https://wiki.mozilla.org/CA:Recommended_Practices ?

Comment 3

[Kai Engert [:KaiE:]]

FYI, with this patch, when going to an address like
  https://192.168.0.1
the user will get the usual SSL bad certificate error page, 
and a "domain mismatch" will be reported.

I confess I haven't yet enhanced the shown message, it will say "not valid for 192.168.0.1, only valid for 192.168.0.1", which of course doesn't make a lot of sense. We'll need to add new text.

The user will be able to override using the existing exception mechanism.

Comment 4

[Ludovic Hirlimann [:Usul]]

Why is this bug hidden ?

Comment 5

[Wan-Teh Chang]

Comment on attachment 464060 [details] [diff] [review]
Patch v1

In my opinion, the best approach is to duplicate the
SSL_AuthCertificate and CERT_VerifyCertName in PSM, so
that you can customize the CERT_VerifyCertName to
disallow an IP address in subject common name.  The
reason is that the subject common name should be
ignored unless the certificate has no subjectAltName
extension.

Here are the comments on your patch, which probably
adds as much code as duplicating SSL_AuthCertificate
and CERT_VerifyCertName.  I give review- because of
the potential bug below (marked with "BUG").

In security/manager/ssl/src/nsNSSCallbacks.cpp:

>+    PRBool certCommonNameIsIP;
>+    if (nsSSLIOLayerHelpers::getCertCommonNameIsAnIP(serverCert, certCommonNameIsIP) != SECSuccess) {
>+      PORT_SetError(PR_OUT_OF_MEMORY_ERROR);
>+      return SECFailure;
>+    }

Is PR_OUT_OF_MEMORY_ERROR the right error code?  It's
better to let getCertCommonNameIsAnIP set the error
code when it fails.

>+    if (!certCommonNameIsIP) {
>+      // force mismatch
>+      if (!status->mHaveCertErrorBits) {
>+        status->mHaveCertErrorBits = PR_TRUE;
>+        status->mIsNotValidAtThisTime = PR_FALSE;
>+        status->mIsUntrusted = PR_FALSE;
>+      }
>+      status->mIsDomainMismatch = PR_TRUE;
>+      nsSSLIOLayerHelpers::mHostsWithCertErrors->RememberCertHasError(
>+        infoObject, status, SECFailure);
>+      PORT_SetError(SSL_ERROR_BAD_CERT_DOMAIN);
>+    }

BUG: I think this "if" statement should test certCommonNameIsIP
rather than !certCommonNameIsIP.

In security/manager/ssl/src/nsNSSIOLayer.cpp:

>+SECStatus nsSSLIOLayerHelpers::getCertCommonNameIsAnIP(CERTCertificate *peerCert, PRBool &isAnIP)
>+{
>+  if (!peerCert)
>+    return SECFailure;

This should set the SEC_ERROR_INVALID_ARGS error.

In security/manager/ssl/src/nsNSSIOLayer.h:

>   static void setRenegoUnrestrictedSites(const nsCString &str);
>   static PRBool isRenegoUnrestrictedSite(const nsCString &str);
> 
>   static PRFileDesc *mSharedPollableEvent;
>   static nsNSSSocketInfo *mSocketOwningPollableEvent;
>   
>   static PRBool mPollableEventCurrentlySet;
>+
>+  static SECStatus getCertCommonNameIsAnIP(CERTCertificate *peerCert, PRBool &isAnIP);

The new getCertCommonNameIsAnIP method should be moved up
(this header declares methods first, then members).

Comment 6

[Kai Engert [:KaiE:]]

(In reply to comment #5)
> In my opinion, the best approach is to duplicate the
> SSL_AuthCertificate and CERT_VerifyCertName in PSM, so
> that you can customize the CERT_VerifyCertName to
> disallow an IP address in subject common name.

I didn't like the idea to duplicate NSS code in PSM, but now I believe I have a good reason why we can't do it.

The above functions call into the following other NSS functions, which are currently not being exported:
  cert_VerifySubjectAltName
  cert_TestHostName
  ssl_FindSocket

Even if you argued that these additional functions should get duplicated in PSM, too, we can't, because those functions make use of type "sslSocket", but that's a private, internal type used by the SSL implementation.

Simply declaring that type isn't sufficient, because the code accesses the internals of the type, we'd have to include sslimpl.h, and I think we don't want that.

Comment 7

[Kai Engert [:KaiE:]]

> >+    if (nsSSLIOLayerHelpers::getCertCommonNameIsAnIP(serverCert, certCommonNameIsIP) != SECSuccess) {
> >+      PORT_SetError(PR_OUT_OF_MEMORY_ERROR);
> >+      return SECFailure;
> 
> Is PR_OUT_OF_MEMORY_ERROR the right error code?  It's
> better to let getCertCommonNameIsAnIP set the error
> code when it fails.

Ok.


> >+    if (!certCommonNameIsIP) {
> >+      // force mismatch
> >+      if (!status->mHaveCertErrorBits) {
> >+        status->mHaveCertErrorBits = PR_TRUE;
> >+        status->mIsNotValidAtThisTime = PR_FALSE;
> >+        status->mIsUntrusted = PR_FALSE;
> >+      }
> >+      status->mIsDomainMismatch = PR_TRUE;
> >+      nsSSLIOLayerHelpers::mHostsWithCertErrors->RememberCertHasError(
> >+        infoObject, status, SECFailure);
> >+      PORT_SetError(SSL_ERROR_BAD_CERT_DOMAIN);
> >+    }
> 
> BUG: I think this "if" statement should test certCommonNameIsIP
> rather than !certCommonNameIsIP.

You're right, thanks for finding this bug.


> In security/manager/ssl/src/nsNSSIOLayer.cpp:
> 
> >+SECStatus nsSSLIOLayerHelpers::getCertCommonNameIsAnIP(CERTCertificate *peerCert, PRBool &isAnIP)
> >+{
> >+  if (!peerCert)
> >+    return SECFailure;
> 
> This should set the SEC_ERROR_INVALID_ARGS error.

Ok.


> >+  static SECStatus getCertCommonNameIsAnIP(CERTCertificate *peerCert, PRBool &isAnIP);
> 
> The new getCertCommonNameIsAnIP method should be moved up
> (this header declares methods first, then members).

Please look again at this class. It doesn't separate members and methods. Instead, it's grouping members and methods that belong together. This isn't a class that follows the usual style, it's a collection of static helper functions. The only reason for having the class is not polluting the global namespace.

I'd like to keep this addition at the end of the class declaration, because this new helper doesn't belong to any of the other groups above it.

Comment 8

[Kai Engert [:KaiE:]]

Attachment: Patch v2

Comment 9

[Kai Engert [:KaiE:]]

Comment on attachment 467188 [details] [diff] [review]
Patch v2

taking back my review request.

I still need to check whether the CN is being used at all.
I propose to use code similar to what we do in the cert-viewer.
I believe the logic is:
- as soon as there's any subject-alt-name extension, PSM can ignore the contents of the CN
- if there's no subject-alt-name, then we'll do what the patch currently checks

Comment 10

[Kai Engert [:KaiE:]]

Attachment: Patch v3

This patch will ignore CN as soon as a cert has a SAN.

If there's no SAN, and the CN looks like an IP, PSM triggers a domain mismatch error.

Comment 11

[Wan-Teh Chang]

Comment on attachment 467380 [details] [diff] [review]
Patch v3

Kai, thanks for writing this patch.  The logic of this
patch is correct.  I'd like to suggest some changes.

In security/manager/ssl/src/nsNSSCallbacks.cpp:

You seem to be doing more work than necessary in this file.
It is best to write the new code as post-processing for the
SSL_AuthCertificate call.  So, immediately after this line:

999   CERTCertificate *serverCert = SSL_PeerCertificate(fd);
1000   CERTCertificateCleaner serverCertCleaner(serverCert);
1001 
1002   if (serverCert) {

Add

    if (SECSuccess == rv) {
      PRBool rejectCert;
      if (nsSSLIOLayerHelpers::rejectCertBasedOnCNandSAN(serverCert, rejectCert) != SECSuccess) {
        return SECFailure;
      }
      if (rejectCert) {
        PORT_SetError(SSL_ERROR_BAD_CERT_DOMAIN);
        rv = SECFailure; // force mismatch
      }
    }

You don't need the following new code:

+    if (rejectCert) {
+      // force mismatch
+      if (!status->mHaveCertErrorBits) {
+        status->mHaveCertErrorBits = PR_TRUE;
+        status->mIsNotValidAtThisTime = PR_FALSE;
+        status->mIsUntrusted = PR_FALSE;
+      }
+      status->mIsDomainMismatch = PR_TRUE;
+      nsSSLIOLayerHelpers::mHostsWithCertErrors->RememberCertHasError(
+        infoObject, status, SECFailure);
+      PORT_SetError(SSL_ERROR_BAD_CERT_DOMAIN);
+    }

because it'll be done by nsNSSBadCertHandler, which you updated
below.

In security/manager/ssl/src/nsNSSIOLayer.cpp:

>+  // If there is a subj-alt-name, it's good (we ignore the CN)

Nit: subj-alt-name => subjectAltName

>+    char *walk = cn;
>+    // remove chars that aren't accepted by PR_StringToNetAddr
>+    for (; *walk; ++walk) {
>+      if (*walk == '*')
>+        *walk = '0';
>+    }

Remove the 'walk' code.  The asterisk '*' is handled by NSS.
PSM just needs to filter out normal IP addresses.

>+  {
>+    PRBool domainMismatch = PR_FALSE;
>+    PRBool rejectCert;

Just curious: why did you create a {} block for this code?
Is it because you want to limit the scope of these two
variables?

>+    if (rejectCert) {
>+      domainMismatch = PR_TRUE; // force mismatch
>+    }
>+    else {
>+      // Check the name field against the desired hostname.
>+      if (hostname[0] &&
>+          CERT_VerifyCertName(peerCert, hostname) != SECSuccess) {
>+        domainMismatch = PR_TRUE;
>+      }
>+    }

Nit: move the domainMismatch declaration right above the
"if (rejectCert) {" line, so that it's close to the first
use.

>+    if (domainMismatch) {
>+        collected_errors |= nsICertOverrideService::ERROR_MISMATCH;
>+        errorCodeMismatch = SSL_ERROR_BAD_CERT_DOMAIN;
>+    }

Nit: the body of this if statement is over-indented by 2.

In security/manager/ssl/src/nsNSSIOLayer.h:

>+  static SECStatus rejectCertBasedOnCNandSAN(CERTCertificate *peerCert, PRBool &reject);

The name of this function is vague.  I suggest

    // The subject common name is ignored if peerCert has a subjectAltName extension.
    static SECStatus commonNameIsIPAddr(CERTCertificate *peerCert, PRBool &cnIsIPAddr);

Comment 12

[[Baboo]]

what happened to this?

Comment 13

[Kai Engert [:KaiE:]]

Too many change requests, too little time.

Comment 14

[Kai Engert [:KaiE:]]

Attachment: Patch v4

this is patch v3, which was heavily bitrotted, merged to mozilla-central

Comment 15

[Kai Engert [:KaiE:]]

Attachment: Patch v5

addressed Wan-Teh's change reuqests.

Patch builds, but is untested.

Comment 16

[Kai Engert [:KaiE:]]

(In reply to Wan-Teh Chang from comment #11)
> Add
> 
>     if (SECSuccess == rv) {
>       PRBool rejectCert;
>       if (nsSSLIOLayerHelpers::rejectCertBasedOnCNandSAN(serverCert,
> rejectCert) != SECSuccess) {
>         return SECFailure;
>       }
>       if (rejectCert) {
>         PORT_SetError(SSL_ERROR_BAD_CERT_DOMAIN);
>         rv = SECFailure; // force mismatch
>       }
>     }
> 
> You don't need the following new code:
> 
> +    if (rejectCert) {
> +      // force mismatch
> +      if (!status->mHaveCertErrorBits) {
> +        status->mHaveCertErrorBits = PR_TRUE;
> +        status->mIsNotValidAtThisTime = PR_FALSE;
> +        status->mIsUntrusted = PR_FALSE;
> +      }
> +      status->mIsDomainMismatch = PR_TRUE;
> +      nsSSLIOLayerHelpers::mHostsWithCertErrors->RememberCertHasError(
> +        infoObject, status, SECFailure);
> +      PORT_SetError(SSL_ERROR_BAD_CERT_DOMAIN);
> +    }
> 
> because it'll be done by nsNSSBadCertHandler, which you updated
> below.


Your original suggestion no longer applies, because PSM no longer uses a bad cert handler.

I haven't checked if this code needs to be added elsewhere.

I vaguely remember that we needed to perform the check twice, once when verifying, once when assembling the eror report.

Patch v5 calls the check twice, in AuthCert and when creating an error object, so maybe it's sufficient.


> Remove the 'walk' code.  The asterisk '*' is handled by NSS.
> PSM just needs to filter out normal IP addresses.


I was worried that NSS doesn't check for partial IP with *


> Just curious: why did you create a {} block for this code?
> Is it because you want to limit the scope of these two
> variables?


yes

Comment 17

[Kai Engert [:KaiE:]]

reassign bug owner.
mass-update-kaie-20120918

Comment 18

[Kai Engert [:KaiE:]]

Comment on attachment 620159 [details] [diff] [review]
Patch v5

Moving the review request to a PSM person

(I'm just cleaning up, I haven't looked into this in a while, please also feel free to take over this bug.)

Comment 19

[Honza Bambas (:mayhemer)]

Comment on attachment 620159 [details] [diff] [review]
Patch v5

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

r=honzab

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +1756,5 @@
> +  if (!peerCert)
> +    return false;
> +
> +  // If there is a subjectAltName, it's good (we ignore the CN)
> +  SECItem altNameExtension = {siBuffer, NULL, 0 };

0};

Comment 20

[Honza Bambas (:mayhemer)]

Comment on attachment 620159 [details] [diff] [review]
Patch v5

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

Dropping r+ after more careful look.

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +919,5 @@
>    }
>  
> +  if (cert && nsSSLIOLayerHelpers::commonNameIsIPAddr(cert)) {
> +    rv = SECFailure; // force mismatch
> +  }

You have to set an error (PORT_SetError), no?

What error should that be?  According the code at CreateCertErrorRunnable it looks like you treat this as a domain mismatch what is an overridable error.  I'm not sure this should be overridable.

@@ +920,5 @@
>  
> +  if (cert && nsSSLIOLayerHelpers::commonNameIsIPAddr(cert)) {
> +    rv = SECFailure; // force mismatch
> +  }
> +    

white spaces

Comment 21

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

As of bug 1245280, name verification doesn't use the subject common name at all (for new certificates), so I think we can call this fixed.