Want exported function that outputs all host names for DNS name matching

RESOLVED FIXED in 3.12

Status

NSS
Libraries
P2
enhancement
RESOLVED FIXED
10 years ago
7 years ago

People

(Reporter: kaie, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

9.59 KB, patch
Nelson Bolyard (seldom reads bugmail)
: review+
Details | Diff | Splinter Review
8.35 KB, text/plain
Details
9.58 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

10 years ago
I'd like PSM to extract the list of valid DNS names from a cert.

Bob R pointed out, this requires the use of
  CERT_FindNSStringExtension(nssCert, SEC_OID_NS_CERT_EXT_SSL_SERVER_NAME);
which can override the name found in a cert's CN.

Unfortunately CERT_FindNSStringExtension is not being exported.

Can we do that for 3.12?
Because CERT_VerifyCertName uses it, right?
Would it be OK if we changed CERT_VerifyCertName to not call it any more,
and retire function CERT_VerifyCertName instead?

If not, then go ahead and write the one-line patch for whatever .def 
file should export it, and ask Julien to review it.  :)
er, sorry, copy-n-paste error there.  I meant
... and retire function CERT_FindNSStringExtension ...
(Yes, I know it also has one other caller)
(Reporter)

Comment 3

10 years ago
(In reply to comment #1)
> Because CERT_VerifyCertName uses it, right?

Because Bob recommended me to do so :-)


> Would it be OK if we changed CERT_VerifyCertName to not call it any more,
> and retire function CERT_VerifyCertName instead?

Even after reading comment 2 I'm not sure what you intended to ask, sorry :-/

Are you proposing to retire function CERT_FindNSStringExtension?
I don't understand why you would propose that and how that would be related to this bug.
There seem to be multiple callers from NSS to function CERT_FindNSStringExtension.

Why would retiring CERT_FindNSStringExtension and using CERT_VerifyCertName help me? I think it would not?

I don't want to verify names, I want to extract names.
(Reporter)

Comment 4

10 years ago
Created attachment 286038 [details] [diff] [review]
Patch v1
Attachment #286038 - Flags: review?(julien.pierre.boogz)
Sorry for not making my counter-proposal clear.  Let me try again.

I believe that the motivation for extracting DNS names from certs is to 
display the names in various dialogs and/or error pages, right?  
Isn't bug 238142 really the motivation for this RFE?

The DNS names that want to be displayed are the names that may be used to 
satisfy the matching of server names from URLs, right?

A DNS name that appears somewhere in the cert (such as in the OU) where 
it cannot be used to match against the hostname in a URL is not of interest, 
right?

So, presently NSS's name matching code (CERT_VerifyCertName) includes names 
from an old Netscape cert extension that was never standardized, and (AFAIK) 
is long since forgotten by any CAs that ever may have supported if (if there 
ever were any).  And because CERT_VerifyCertName uses that as a potential 
source of matching DNS names, you want to get access to that name so it can
be displayed with the other potentially matching DNS names in dialogs, right?

My counter-proposal is that we remove the code that calls CERT_FindNSStringExtension from CERT_VerifyCertName, so that that obsolete
Netscape extension ceases to be one of the places from which a DNS host name
may be taken for matching.  Doing so would mean that there is no need to 
display that name in dialogs to satisfy bug 238142.  
Severity: normal → enhancement
Target Milestone: --- → 3.12
(Reporter)

Comment 6

10 years ago
Nelson, thanks a lot for your more detailed comment.


> I believe that the motivation for extracting DNS names from certs is to 
> display the names in various dialogs and/or error pages, right?  
> Isn't bug 238142 really the motivation for this RFE?

Right, although it was done as part of bug 398718.


> The DNS names that want to be displayed are the names that may be used to 
> satisfy the matching of server names from URLs, right?

right


> A DNS name that appears somewhere in the cert (such as in the OU) where 
> it cannot be used to match against the hostname in a URL is not of interest, 
> right?

right


> So, presently NSS's name matching code (CERT_VerifyCertName) includes names 
> from an old Netscape cert extension that was never standardized, and (AFAIK) 
> is long since forgotten by any CAs that ever may have supported if (if there 
> ever were any).  And because CERT_VerifyCertName uses that as a potential 
> source of matching DNS names, you want to get access to that name so it can
> be displayed with the other potentially matching DNS names in dialogs, right?

Right, or more specifially, if there are no sub alt names, the Netscape cert extension would have precendence over the common name (same as NSS code does).


> My counter-proposal is that we remove the code that calls
> CERT_FindNSStringExtension from CERT_VerifyCertName, so that that obsolete
> Netscape extension ceases to be one of the places from which a DNS host name
> may be taken for matching.  Doing so would mean that there is no need to 
> display that name in dialogs to satisfy bug 238142.  

Well, I'm ok with your proposal if you think it's safe to do.

But isn't there a risk that we will break some sites on the internet?
Is it necessary to break such sites?

I'm ok with either action that achieves consistency between NSS name matching and the application's name display.

In addition to the alternatives

(a) export the function

(b) stop using the extension in NSS 

there is a third one:

(c) Implement a new exported function that makes NSS return
    a list of all allowed names it uses for hostname matching.

Doing (c) would free the application from repeating the logic.
Kai, I agree that your proposal:

> Implement a new exported function that makes NSS return
> a list of all allowed names it uses for hostname matching.

Is really the right thing to do here.  
What do you think the interface for such a function should look like?
Summary: Export CERT_FindNSStringExtension → Want exported function that outputs all host names for DNS name matching
P2 because PSM would use it if it existed.
Priority: -- → P2
Comment on attachment 286038 [details] [diff] [review]
Patch v1

I'm preemptively marking this r-.  Kai's alternative proposal, a function that returns all the DNS names for matching, is the right answer.
Attachment #286038 - Flags: review?(julien.pierre.boogz) → review-
(Reporter)

Comment 10

10 years ago
(In reply to comment #7)
> What do you think the interface for such a function should look like?

We want it to return a list of strings, where each can be either a fully qualified name, or DNS wildcard pattern. I think the consumer does not need to differ between them.

I propose we mirror a scheme already used by NSS, I found 
  CERT_NicknameStringsFromCertList
which uses type
  CERTCertNicknames
which represents a count and a list of string pointers. Seems right to me.

Unless you object, I think we simply reuse the existing type.
If you object, I propose we duplicate the type under a new name.

/*
 * Collect the valid DNS names or patterns from the given cert.
 * Returns NULL on failure.
 * Use CERT_FreeNicknames() to free the result.
 *
 * "cert" - the certificate
 */
CERTCertNicknames *
CERT_GetValidDNSPatternsFromCert(CERTCertificate *cert);


Or if you prefer the most simple solution:

/*
 * 
 */
int
CERT_GetValidDNSPatternsFromCert(CERTCertificate *cert,
                                 char ***patterns)
{
  *patterns = (char**)(malloc((sizeof char*) * count));
  return count;
}

char **patterns = NULL;
int number_of_patterns = CERT_GetValidDNSPatternsFromCert(cert, &patterns);

char **walk = patterns;
for (int i=0; i < number_of_patterns; ++i, ++walk) {
  printf("%s\n", *walk);
  free(*walk);
}
free(patterns);

Phrew, triple *, I bet I made a minor bug in this code, please read it as pseudo code.
Kai, I think I could live with your proposal to usurp CERTCertNicknames.
Would you like to write a patch for this for NSS 3.12?
(Reporter)

Comment 12

10 years ago
Created attachment 290958 [details] [diff] [review]
Patch v2

Sigh. Even if you just want to return a little bit of data, where everything is straightforward, doing it in C is still a lot of detailed hacking. We should convert NSS to C++... ;-)
Attachment #286038 - Attachment is obsolete: true
Attachment #290958 - Flags: review?(nelson)
Comment on attachment 290958 [details] [diff] [review]
Patch v2

I think the design of this new code is basically good, but I have 
a number of comments, mostly about function names, comments, and 
one function argument.  Easy Stuff.

1. This new function, CERT_GetValidDNSPatternsFromCert, is trying 
to match the behavior of another, existing function, namely
CERT_VerifyCertName, except that it outputs the names/patterns to 
be used for comparisons, rather than doing the comparisons.  

So, I think this new function should be implemented in the same source 
file where CERT_VerifyCertName is implemented, namely certdb.c, 
and close to CERT_VerifyCertName in that file.  And I think the 
declaration of this new function should be near the declaration of
CERT_VerifyCertName in cert.h.

2. I think there may potentially be a lot of confusion about what's being 
returned.  As you well know, even though it has type CERTCertNicknames, 
it's actually NOT a set of Nicknames.  But most readers of this code are
not going to know that, so there needs to be a clear comment, immediately
before the declaration in cert.h and before the definition (in certdb.c)
making that point very clear: These "nicknames" are actually the host 
names, host name patterns, and/or IP address strings taken from the cert 
for host name comparison purposes.  

Now for some specifics in the code.

3. This code passes the address of a SECItem to cert_GetSubjectAltNameList
as an argument.  cert_GetSubjectAltNameList puts data into that secitem,
and that data is freed by the caller sometime later.  I'm guessing that
this is because it was thought that the content of that secitem had to 
be preserved until after cert_GetNickNamesFromGeneralNames was called and
had finished, for some reason.  

But IINM, it's not necessary to preserve the contents of that SECITEM 
outside of the scope of function cert_GetSubjectAltNameList, because 
the contents of that SECItem is copied into the arena by 
CERT_DecodeAltNameExtension, which is called by cert_GetSubjectAltNameList.
So, it should be enough to define and initialize SECItem subAltName inside
of cert_GetSubjectAltNameList, and unconditionally call SECITEM_FreeItem
on that SECItem immediately after the call to CERT_DecodeAltNameExtension.
No need for the contents of that SECItem to be accessible to any code 
outside of cert_GetSubjectAltNameList, I believe.  (But if you think 
this analysis is wrong, please enlighten me.)

4. New function cert_CountGeneralNames doesn't do what its name suggests.
It only counts general names of two specific types, and ignores the other
types of general names that are allowed to exist in a subjectAltNames 
extension.  So it should be given a name that better explains what it 
does, or else some developer will be tempted to call it for the purpose
of counting ALL general names.

5. Similarly, cert_GetNickNamesFromGeneralNames doesn't do what its name
suggests.  It doesn't get nicknames.  It populates the contents of a 
nicknames structure with values that aren't nicknames.  I think we want
the function name to be completely clear about what it's doing.  As a 
code reviewer, I've seen many developers write code that calls a function, 
guessing that they know what it does based solely on its name.  So, let's
use names that won't tempt them to use this function for the wrong purpose.
Maybe "cert_GetDNSPatternsFromGeneralNames"?
Attachment #290958 - Flags: review?(nelson) → review-
(Reporter)

Comment 14

10 years ago
Created attachment 293051 [details] [diff] [review]
Patch v3
Attachment #290958 - Attachment is obsolete: true
Attachment #293051 - Flags: review?(nelson)
(Reporter)

Comment 15

10 years ago
Nelson, thanks a lot for your review.

I agree with all your comments and think I addressed all your change requests.
(Reporter)

Comment 16

10 years ago
Created attachment 293052 [details]
Diff Diff between v2 and v3
(Reporter)

Comment 17

10 years ago
The diff diff is not very easy to read, so let me summarize my changes between v2 and v3, which I'm building by looking at a graphical side by side diff in tkdiff.

- moved declaration and function to the new desired location
- added a comment in the header
- eliminated the SECItem parameter in cert_GetSubjectAltNameList
- use a temporary SECItem in cert_GetSubjectAltNameList
  and free it asap
- renamed cert_CountGeneralNames to cert_CountDNSPatterns
- rename cert_GetNickNamesFromGeneralNames to cert_GetDNSPatternsFromGeneralNames
Comment on attachment 293051 [details] [diff] [review]
Patch v3

r+=nelson

Nits:

>+ * numberOfGeneralNames should have been obtain from cert_CountDNSPatterns,
   should be ->                            obtained

>+      /* could we allocate both the buffer of pointers and the string? */
should be -> Did we
Attachment #293051 - Flags: review?(nelson) → review+
(Reporter)

Comment 19

10 years ago
Created attachment 295905 [details] [diff] [review]
Patch v3 with nits addressed, for check in
(Reporter)

Comment 20

10 years ago
Thanks for the review Nelson.

Checked in, marking fixed.


Checking in certdb/cert.h;
/cvsroot/mozilla/security/nss/lib/certdb/cert.h,v  <--  cert.h
new revision: 1.63; previous revision: 1.62
done
Checking in certdb/certdb.c;
/cvsroot/mozilla/security/nss/lib/certdb/certdb.c,v  <--  certdb.c
new revision: 1.88; previous revision: 1.87
done
Checking in nss/nss.def;
/cvsroot/mozilla/security/nss/lib/nss/nss.def,v  <--  nss.def
new revision: 1.184; previous revision: 1.183
done
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Reporter)

Updated

10 years ago
Blocks: 411246
You need to log in before you can comment on or make changes to this bug.