Closed Bug 585616 Opened 9 years ago Closed 4 years ago

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

Categories

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

2.0 Branch
defect

Tracking

()

RESOLVED DUPLICATE of bug 1245280
mozilla15

People

(Reporter: KaiE, Unassigned)

Details

(Keywords: sec-low, Whiteboard: [sg:low?])

Attachments

(3 files, 2 obsolete files)

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.
Attached patch Patch v1 (obsolete) — Splinter Review
Initial patch.
Attachment #464060 - Flags: review?(rrelyea)
Should we add a section to https://wiki.mozilla.org/CA:Recommended_Practices ?
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.
Why is this bug hidden ?
Group: core-security
Whiteboard: [sg:low?]
Attachment #464060 - Flags: review?(rrelyea) → review?(wtc)
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).
Attachment #464060 - Flags: review?(wtc) → review-
(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.
> >+    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.
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #464060 - Attachment is obsolete: true
Attachment #467188 - Flags: review?(wtc)
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
Attachment #467188 - Flags: review?(wtc)
Attached patch Patch v3Splinter Review
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.
Attachment #467188 - Attachment is obsolete: true
Attachment #467380 - Flags: review?(wtc)
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);
Attachment #467380 - Flags: review?(wtc) → review-
what happened to this?
Too many change requests, too little time.
Attached patch Patch v4Splinter Review
this is patch v3, which was heavily bitrotted, merged to mozilla-central
Attached patch Patch v5Splinter Review
addressed Wan-Teh's change reuqests.

Patch builds, but is untested.
Attachment #620159 - Flags: review?
(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
Target Milestone: mozilla2.0b4 → mozilla15
Version: Trunk → 2.0 Branch
Attachment #620159 - Flags: review? → review?(wtc)
reassign bug owner.
mass-update-kaie-20120918
Assignee: kaie → nobody
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.)
Attachment #620159 - Flags: review?(wtc) → review?(honzab.moz)
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};
Attachment #620159 - Flags: review?(honzab.moz) → review+
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
Attachment #620159 - Flags: review+
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.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1245280
You need to log in before you can comment on or make changes to this bug.