selfserv fails when server cert contains multiple DNS names in SAN

Status

NSS
Tools
P2
critical
ASSIGNED
8 years ago
4 years ago

People

(Reporter: Nelson Bolyard (seldom reads bugmail), Assigned: Nelson Bolyard (seldom reads bugmail))

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: regression)

Attachments

(2 attachments, 1 obsolete attachment)

I have a cert with a large number of DNS names in the SAN extension.
The subject Common Name is merely "localhost".
selfserv used to be able to serve this cert OK, but now with trunk 
it fails.  selfserv sends an alert saying the requested name from 
client's SNI cert is not supported, even though it is clearly present 
in the cert's SAN extension.  

This is a show stopper!!

I run selfserv with the command
  selfserv -d DB -vv -n localhost -p 443

The cert whose nickname is localhost is as described above. 
I access the server using one of the DNS names in the cert's SAN.
It fails.  The server sends an alert to the client.  

This is double-plus ungood.
This regression is a show stopper for 3.12.7 IMO.
Target Milestone: --- → 3.12.7
Created attachment 449492 [details]
pkcs12 file with server cert & chain.  PW=test
The problem here is that the client requests a connection using one of the 
names in the cert's SAN.  Evidently, selfserv now doesn't recognize any names
from the SNI by default, and so returns an error alert to the client.  
I can see that as an optional behavior to be enabled with some command line 
switches, but IMO, the default behavior should be to serve the configured cert without errors, ESPECIALLY if the requested DNS name is actually in the cert's
SAN!
I have written a patch for this that I've been running for a few weeks.
I think it is OK, but I'm not sure.

There's an SNI callback function registered with the SSL socket by calling
SSL_SNISocketConfigHook, and I'm not sure what the value returned by that 
function is supposed to mean.  It's an index in some space, but what space?

The function documentation could use more explanation in this area, I think.
Assignee: alexei.volkov.bugs → nelson
Status: NEW → ASSIGNED

Updated

8 years ago
Target Milestone: 3.12.7 → 3.13

Comment 5

8 years ago
Nelson, if you still have the patch, could you attach it to
this bug?  Thanks.
Target Milestone: 3.13 → 3.12.9
Oh, I forgot about this bug, and just filed bug 593089, which is essentially 
a duplicate of this one.  My patch is attached to that bug.
(Assignee)

Updated

8 years ago
Duplicate of this bug: 593089
Created attachment 471584 [details] [diff] [review]
patch for trunk

This patch makes selfserv choose from among multiple server certificates by 
looking up the client's SNI name in the host names in the cert(s), and 
picking the first cert that it finds with a matching host name.  It uses the same host name matching function as our clients use.  

With this patch, it is no longer necessary to use a DNS name for a cert's
nickname, because the nickname is not used for matching with SNI strings.
It is also no longer necessary to provide multiple DNS names on the command
line for a single cert.  selfserv will use all the host names found in the cert(s).  

This patch makes the -a and -n options synonymous.  Either one or both may be 
used to specify a nickname for a cert, and up to 10 nicknames may be given.
The first nickname given becomes the "default" cert, the one used if no 
SNI option is present in the client hello.

I have been running this patch at home continuously for 9 weeks.  I use 
selfserv with certs from my own CA to respond to requests sent to https 
ad servers that are redirected to 127.1 via my hosts file.  

This patch also makes one other change, which makes the patch MUCH larger 
than it otherwise needs to be.  (Sorry)  It removes the name "selfserv" from 
all the error messages, and instead displays the name given on the command 
line to invoke the program.  This adds a lot of lines to the patch, but 
they are trivial to review.
Attachment #471584 - Flags: superreview?(wtc)
Attachment #471584 - Flags: review?(alexei.volkov.bugs)

Comment 9

8 years ago
Comment on attachment 471584 [details] [diff] [review]
patch for trunk

Nelson, thank you for the patch.

I'd appreciate it if you could separate this into two
patches.  A big patch with a lot of "noise" in it is
hard to review.

Also, you changed the coding style of previous code
unnecessarily.  Here is an example:

>-    {
>-	int i;
>-	for (i=0; i<kt_kea_size; i++) {
>-	    if (cert[i]) {
>-		CERT_DestroyCertificate(cert[i]);
>-	    }
>-	    if (privKey[i]) {
>-		SECKEY_DestroyPrivateKey(privKey[i]);
>-	    }
>-	}
>-        for (i = 0;virtServerNameArray[i];i++) {
>-            PORT_Free(virtServerNameArray[i]);
>-        }
>+    for (i=0; i<kt_kea_size; i++) {
>+	if (cert[i]) {
>+	    CERT_DestroyCertificate(cert[i]);
>+	}
>+	if (privKey[i]) {
>+	    SECKEY_DestroyPrivateKey(privKey[i]);
>+	}
>+    }
>+    for (nextVirtServerInfo = virtServerInfos; 
>+	 nextVirtServerInfo - virtServerInfos < MAX_NICKNAMES &&
>+         nextVirtServerInfo->privKey;
>+         nextVirtServerInfo++) {
>+	PORT_Free(               nextVirtServerInfo->nickName);
>+	CERT_DestroyCertificate( nextVirtServerInfo->cert);
>+	if (nextVirtServerInfo->privKey) {
>+	    SECKEY_DestroyPrivateKey(nextVirtServerInfo->privKey);
>+    	}
>     }

The new code now has two for loops in close proximity,
but one uses an array index, and the other uses a moving
pointer.  The original code uses array indexes in both
loops consistently.

>-static char  *virtServerNameArray[MAX_VIRT_SERVER_NAME_ARRAY_INDEX];
>+static selfServerCertInfo virtServerInfos[MAX_NICKNAMES];

MAX_NICKNAMES should be renamed MAX_VIRT_SERVER_INFOS or
MAX_VIRTUAL_SERVERS because the array is no longer named
virtServerNameArray.

The type name selfServerCertInfo should be capitalized
SelfServerCertInfo.  That's the naming convention in NSS.
But I know selfserv.c has a lot of violations of our naming
convention.

The sorting of variable declarations by their types at the
beginning of the main() function makes the patch larger than
necessary, but doesn't make the code easier to understand.
For example, the closely related arrays

+    CERTCertificate *    cert   [kt_kea_size] = { NULL };
...
+    SECKEYPrivateKey *   privKey[kt_kea_size] = { NULL };

used to be declared next to each other.  Now they're
separated.

Comment 10

8 years ago
(In reply to comment #4)
> The function documentation could use more explanation in this area, I think.

Nelson, please check ssl.h. This file has description of the error codes expected to be returned by the callback function.

http://mxr.mozilla.org/security/source/security/nss/lib/ssl/ssl.h#295

Comment 11

8 years ago
Comment on attachment 471584 [details] [diff] [review]
patch for trunk

Good patch. I should have used CERT_VerifyCertName instead of just looking for nick match.

Here are my comments:

line 467: pwdata no longer needed since keys are already known.
line 482: indentation problem.

line 1909 through 1913 are not needed. The program already exited or returned success.

line 2255: since this code can be used as example, I would emphasize that allowing privKey to be NULL will work, only if server configured to disallow name change during renegotiation(default). However, if it is allowed and the key is NULL, previously used key pair will be used. 

line 2438: since you set privKey to NULL only when nextVirtServerInfo == virtServerInfos at line 2249, this loop will stop at the first iteration.  
You probably intended to use nickName instead of privKey to stop iterations.
Attachment #471584 - Flags: review?(alexei.volkov.bugs) → review-
(Assignee)

Updated

8 years ago
Priority: P1 → P2
Created attachment 495936 [details] [diff] [review]
Updated patch for trunk

This update addresses all of the review concerns except it still does not separate the large "progname" change from the rest (sorry wtc). 
It fixes a few bugs that I found in further testing on more platforms 
since I  posted the previous patch.
Attachment #471584 - Attachment is obsolete: true
Attachment #495936 - Flags: review?(wtc)
Attachment #471584 - Flags: superreview?(wtc)
Comment on attachment 495936 [details] [diff] [review]
Updated patch for trunk

A note for reviewer(s):
This patch may appear to have indentation problems, if you look at it with 
BMO's patch diff viewer.  But nearly all of these apparent problems are false,
that is, not real problems.  

The issue is that, when the browser renders the text, it does not render the 
same amount of horizontal space for 8 spaces and for a tab character, so 
lines that are indented with spaces appear to have different indentation 
than lines indented with tabs, even when a fixed width font is used.
I hate to say it, but it looks better on IE than FF.  

So, reviewers, please resist your urge to (that I share) to object to apparent
indentation problems.  I'd like to run a simple script over this file that 
would convert it to a uniform use of tabs for indentation, with tabs at the 
default of 8 columns wide.  But for this file, it would be an ENORMOUS change.
So, I have not done that in this patch.
You need to log in before you can comment on or make changes to this bug.