Last Comment Bug 329067 - NSS encodes cert distinguished name attributes with wrong string type
: NSS encodes cert distinguished name attributes with wrong string type
Status: RESOLVED FIXED
: helpwanted
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.10.1
: All All
: P2 normal (vote)
: 3.12
Assigned To: Neil Williams
:
Mentors:
http://lxr.mozilla.org/security/sourc...
Depends on: 54969
Blocks: 355096
  Show dependency treegraph
 
Reported: 2006-03-02 00:23 PST by Nelson Bolyard (seldom reads bugmail)
Modified: 2007-05-01 18:01 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
proposed changes for RFC 3280 requirements (2.46 KB, patch)
2007-02-22 19:17 PST, Neil Williams
no flags Details | Diff | Review
changed attribute type tables to include the OID of the string type (6.35 KB, patch)
2007-04-20 19:44 PDT, Neil Williams
nelson: review-
Details | Diff | Review
incorporates Nelson's suggestions, IANA registered types (6.68 KB, patch)
2007-04-27 19:45 PDT, Neil Williams
nelson: review+
Details | Diff | Review
check printableString values for validity (7.62 KB, patch)
2007-04-30 15:45 PDT, Neil Williams
alvolkov.bgs: review+
nelson: review+
Details | Diff | Review

Description Nelson Bolyard (seldom reads bugmail) 2006-03-02 00:23:37 PST
When NSS encodes a subject/issuer name for a cert (CSR), it has to decide
which of the numerous string types to use for each name attribute. 
RFC 3280 explicitly defines the right type for every known attribute.
That info could be encoded in a table that NSS could use to pick the right one.
But today, NSS uses this heuristic instead (see 
http://lxr.mozilla.org/security/source/security/nss/lib/certdb/alg1485.c#345 )

If the attribute is Country (C=) then use printablestring.
Else if the attribute is one of the email address attributes (E= or MAIL=) 
   then use IA5String.
Else if the string data is all within the "printableString" character set, 
   then use printableString.
Else if the string data is all 7-bit data, use T61 string,
Else use UTF8.

That doesn't come close to following RFC3280 for most attribute types.
Here's one type that it always gets wrong:  Domain Component.
Should always be IA5String.  NSS generally encodes as printableString.

This bug fix/enhancement would be a good task for one of NSS's newer developers.
There's a fair amount of new code to be written, but it doesn't require 
great knowledge of NSS or crypto to complete.  Just willingness to slog 
through rfC 3280.
Comment 1 Neil Williams 2007-02-22 19:17:30 PST
Created attachment 256127 [details] [diff] [review]
proposed changes for RFC 3280 requirements

First attempt at meeting the requirements for RFC 3280 for handling attribute types in issuer and subject names. That RFC says two attribute types that NSS currently does not handle MUST be supported. Attributes distinguished name qualifier and serial number are included in lib/certdb/alg1485.c but are presently ifdefed out.

Nelson, you made some changes here recently. Perhaps you remember why you removed them. And what about backward compatibilty concerns?

In addition to shuffling the name2Kinds table I added checks for DC, dnQualifier and serialNumber to set them to the 3280-required types.
Comment 2 Nelson Bolyard (seldom reads bugmail) 2007-02-23 00:15:46 PST
Neil, dnQualifier and serialNumber are NOT presently ifdeffed out.  
All the symbols (except "pseudonym") in that table are presently enabled, IINM.
We have to use the correct encoding for each and every one of them.  

Rather than adding lots more if-then-else cases to CERT_ParseRFC1485AVA, 
let me suggest that you add another column to the table name2kinds, a 
column that defines the correct DER encoding of that attribute value, 
and use that value in CERT_ParseRFC1485AVA.
Comment 3 Nelson Bolyard (seldom reads bugmail) 2007-02-23 00:46:57 PST
I wrote:
> dnQualifier and serialNumber are NOT presently ifdeffed out.  
I was mistaken.  Hmmm.  :-/  Sorry.

Still, I think the right solution is to define the proper encoding for 
all entries in the table, including the ifdeffed out ones.  
Maybe we need to revisit the decision to ifdef those out.  :-/
Comment 4 Nelson Bolyard (seldom reads bugmail) 2007-03-01 03:15:17 PST
See Bug 372241 for more history about this problem, to understand how it 
got into its present state.  This bug is more urgent than that one.  
I'd like to see this bug done for NSS 3.11.x if possible.  
Bug 372241 can wait until NSS 3.12 or even later.
Comment 5 Neil Williams 2007-03-01 16:22:43 PST
Good idea, Nelson. I'll change the table to add a column for attribute type.

There are a couple of other requirements to think about. One is that RFC 3280 declares that, in general, AttributeValues will be of type DirectoryString which is defined to be any of 5 string types and it goes on to describe how to represent the data depending on the character set of the data. Then it goes to say that all certificates issued after Dec 31, 2003 MUST use UTF-8 encoding for DirectoryStrings with the exception that "rollover" certs may use the old rules for subject. What that means, IMO, is that NSS should always encode issuer as UTF-8 but the subject field may use the old encoding rules.
Comment 6 Neil Williams 2007-03-01 16:33:39 PST
Another consideration that has been discussed in email. RFC 4514 (and 2253 which is obsoleted by it) define OID<dotted-decimal>=#<hex digits> as an alternate form for AVAs. Are we in agreement that CERT_ParseRFC1485AVA() should support this?
Comment 7 Nelson Bolyard (seldom reads bugmail) 2007-03-01 17:38:20 PST
in reply to comment 5,
> RFC 3280 declares that, in general, AttributeValues will be of type 
> DirectoryString

That statement has been hugely controversial, and has caused lots of problems.
It's not exactly true.  Some attributes are declared to be encoded as Directory strings, and others are NOT.  The statement means only that those attributes 
that are Directory Strings should now be encoded as UTF8.  

There is an update to RFC 3280 that has modified that statement significantly.
RFC 4630 says that most of RFC 3280 section 4.1.2.4 is replaced with the following:

      The DirectoryString type is defined as a choice of
      PrintableString, TeletexString, BMPString, UTF8String, and
      UniversalString.  CAs conforming to this profile MUST use either
      the PrintableString or UTF8String encoding of DirectoryString,
      with one exception.  When CAs have previously issued certificates
      with issuer fields with attributes encoded using the
      TeletexString, BMPString, or UniversalString, the CA MAY continue
      to use these encodings of the DirectoryString to preserve the
      backward compatibility.

So, that is one reason why we need the ability to preserve the original
encoding of attributes.
Comment 8 Nelson Bolyard (seldom reads bugmail) 2007-03-01 18:02:55 PST
Expanding on the previous comment, RFC 3280 explicitly lists the proper 
encoding types for all the attributes defined therein.  And there are only
TWO out of the many that it defines to be Directory Strings.  Those two 
are rarely used ones that have to do with EDI names.  

So, does that mean that only those two attributes are to be encodeed in UTF8 now?

No.  Remember that RFC 3280 is a "profile" of some X.50n standards.  The authors of RFC 3280 chose to document the encodings of many attribute types DIFFERENTLY than the X.50n documents.  Many attributes that are defined in X.50n to use Directory String were documented in RFC 3280 with other types.  According to 
the authors of RFC 3280 (with whom I have corresponded), when they wrote that
statement about being "in general" Directory Strings, they were referring to 
the definitions in X.50n.  So to find out exactly which attributes are the ones that now shouldl be encoded with UTF8, it is necessary to go back to the X.50n documents and study their definitions of the encodings of the attribute types.

A good first approximation of that is to look for the RFC 3280 attribute types that are defined in RFC 3280 as a choice of 3 or more of the Directory String choices.  It's a good bet that all such attribute types were Directory Strings in X.50n, and therefore should now be encoded with UTF8.

Some attribute types defined in RFC 3280 have only one permitted encoding.
Examples include (From RFC 3280 Appendix A.1): 
   X520dnQualifier  ::=  PrintableString
   X520countryName  ::=  PrintableString (SIZE (2))
   X520SerialNumber ::=  PrintableString (SIZE (1..ub-serial-number))
   DomainComponent  ::=  IA5String
   EmailAddress     ::=  IA5String (SIZE (1..ub-emailaddress-length))
   OrganizationName ::=  PrintableString (SIZE (1..ub-organization-name-length))
   OrganizationalUnitName ::= PrintableString (SIZE
                                        (1..ub-organizational-unit-name-length))
   CommonName       ::=  PrintableString (SIZE (1..ub-common-name-length))

and others.  These were not, and are not, Directory strings and so must 
continue to be encoded using only those string types defined for them.
Comment 9 Nelson Bolyard (seldom reads bugmail) 2007-03-01 18:21:44 PST
Here's are relevant sections of a fairly recent version of X.520: 
http://www.itu.int/ITU-T/asn1/database/itu-t/x/x520/2001/SelectedAttributeTypes.html
http://www.itu.int/ITU-T/asn1/database/itu-t/x/x520/2001/UpperBounds.html

It disagrees with RFC 3280 about the encodings of some types, e.g. CommonName. 
In the cases where RFC 3280 says PrintableString and X.520 says DirectoryString, 
I think we can use PrintableString safely, because of RFC 4630.  I recommend reading RFC 4630.  It's short, and the changes are significant.
Comment 10 Nelson Bolyard (seldom reads bugmail) 2007-03-01 21:48:43 PST
(In reply to comment #6)
> RFC 4514 (and 2253 which is obsoleted by it) define 
> OID<dotted-decimal>=#<hex digits> as an alternate form for AVAs. 
> Are we in agreement that CERT_ParseRFC1485AVA() should support this?
Yes.

Comment 11 Neil Williams 2007-04-20 19:44:46 PDT
Created attachment 262321 [details] [diff] [review]
changed attribute type tables to include the OID of the string type

This is phase one of this bug fix. Nelson, your comment 8 is incorrect, I believe. O= and OU= types, for example, defined in Appendix A-1 are CHOICEs and look to satisfy the criteria for treatment as a DirectoryString type. I've coded this patch with that in mind.

Phase II of this bug fix will handle attribute type names expressed as OIDs in dotted decimal form.
Comment 12 Nelson Bolyard (seldom reads bugmail) 2007-04-20 20:11:46 PDT
Interesting.  RFC 3280 defines two sets of similarly named types.
On page 95, it defines:

> X520OrganizationName ::= CHOICE {
> X520OrganizationalUnitName ::= CHOICE {

on page 100, it defines:

> OrganizationName ::= PrintableString (SIZE (1..ub-organization-name-length))
> OrganizationalUnitName ::= PrintableString (SIZE
>                                     (1..ub-organizational-unit-name-length))

The difference is the first 4 characters of the type name, one starts with 
X520, one does not.  Until I study it some more, I'm not sure which of those
definitions is relevant to what we're doing.  
Comment 13 Nelson Bolyard (seldom reads bugmail) 2007-04-20 20:26:45 PDT
ok, bottom of page 98 says

> -- X.400 address syntax starts here

so, it appears that the X520 definitions are the right ones.  
But the right way to tell is to match up the OIDs.  
Comment 14 Neil Williams 2007-04-27 13:22:24 PDT
Comment on attachment 262321 [details] [diff] [review]
changed attribute type tables to include the OID of the string type

Changing strategy. Asking for review in order to check in Phase I by itself. Will add Phase II to bug 372241.
Comment 15 Nelson Bolyard (seldom reads bugmail) 2007-04-27 17:00:24 PDT
Comment on attachment 262321 [details] [diff] [review]
changed attribute type tables to include the OID of the string type

Review comments on this patch:
1. Please try to line up the new table entries into nice columns,
   with numeric fields right-justified within its column.  
   Some table entries won't fit nicely, but they should be rare.

2. Please check that the "short names" being used in this table are
   actually defined in RFCs.  If this proves to be a problem, that is,
   if a large portion of these short names are STILL not defined in any
   RFC, then I'll reconsider.  Please add a comment citing the RFCs 
   that you find that define any short names used in the table.

3. I see that this patch is trying to implement the new requirement
   of RFC 4630, which states:

          CAs conforming to this profile MUST use either the 
          PrintableString or UTF8String encoding of DirectoryString,

   The table shows the "value type" for DirectoryString to be 
   SEC_ASN1_UTF8_STRING, and whenever encoding an attribute of type
   SEC_ASN1_UTF8_STRING, the patched function chooses between 
   PrintableString and UTF8String, giving preference to PrintableString.

   My concern with this approach is this.  Someday there may be added to
   this table a new attribute type whose only allowed encoding is UTF8String.
   That is, it will not be a DirectoryString, but rather just a UTF8String.
   Therefore, it will NOT be correct to substitute PrintableString encoding
   for UTF8String encoding for such an attribute.  If such a new attribute 
   type were to be added to this table in the obvious fashion, values for 
   that attribute type might get erroneously encoded. 

   So, I ask that you use another value, other than SEC_ASN1_UTF8_STRING,
   to signify an attribute that is really a DirectoryString.  
   I don't have a strong recommendation for another value to use.
   I would suggest using either the value zero, or a value greater than 0x1f,
   IOW, a value outside of the existing range of universal ASN.1 encodings.
   Maybe SEC_ASN1_CHOICE .
Comment 16 Neil Williams 2007-04-27 17:56:51 PDT
Nelson, in trying to find some RFC which declares how to find the complete list of short names for attribute types I find in RFC4520 a form for registering a LDAP descriptors with the IANA (under usage is "attribute type" as a possibility). At http://www.iana.org/assignments/ldap-parameters the table "Object ID Descriptors" includes a definition for all of the short names in the table except for pseudonym  and E. (The list currently #if 0ed out IS defined in the table.) I propose moving pseudonym back out of the table.
Comment 17 Neil Williams 2007-04-27 19:45:47 PDT
Created attachment 263087 [details] [diff] [review]
incorporates Nelson's suggestions, IANA registered types
Comment 18 Nelson Bolyard (seldom reads bugmail) 2007-04-27 20:44:08 PDT
Comment on attachment 263087 [details] [diff] [review]
incorporates Nelson's suggestions, IANA registered types

r=nelson, with one required change.  One spelling error in new symbol.

>+#define SEC_ANS1_DS SEC_ASN1_HIGH_TAG_NUMBER

>+    { "CN",             64, SEC_OID_AVA_COMMON_NAME,    SEC_ANS1_DS},

:g/_ANS1_/s//_ASN1_/g
Comment 19 Neil Williams 2007-04-30 15:45:40 PDT
Created attachment 263305 [details] [diff] [review]
check printableString values for validity

Addresses Alexei's comments.
Comment 20 Neil Williams 2007-04-30 15:48:51 PDT
Comment on attachment 263305 [details] [diff] [review]
check printableString values for validity

(There's no change to the name2kinds table in this patch--just a test for printability.)
Comment 21 Alexei Volkov 2007-04-30 15:50:34 PDT
Comment on attachment 263305 [details] [diff] [review]
check printableString values for validity

r=alexei.volkov
Comment 22 Nelson Bolyard (seldom reads bugmail) 2007-04-30 15:51:08 PDT
Comment on attachment 263305 [details] [diff] [review]
check printableString values for validity

This patch satisfies my review comments.  r=nelson

If Alexei has sent you review comments, please attach them to this bug or include them as a comment.
Comment 23 Neil Williams 2007-04-30 16:19:31 PDT
Comment on attachment 263305 [details] [diff] [review]
check printableString values for validity

Comments Alexei delivered orally: CERT_ParseRFC1485AVA used to check Countryname for printability; declaration of vt should be moved to code block it is used in.
Comment 24 Neil Williams 2007-04-30 16:31:56 PDT
Checking in lib/certdb/alg1485.c;
/cvsroot/mozilla/security/nss/lib/certdb/alg1485.c,v  <--  alg1485.c
new revision: 1.26; previous revision: 1.25
done

Note You need to log in before you can comment on or make changes to this bug.