Closed
Bug 329067
Opened 18 years ago
Closed 17 years ago
NSS encodes cert distinguished name attributes with wrong string type
Categories
(NSS :: Libraries, defect, P2)
Tracking
(Not tracked)
RESOLVED
FIXED
3.12
People
(Reporter: nelson, Assigned: neil.williams)
References
(Depends on 1 open bug, Blocks 1 open bug, )
Details
(Keywords: helpwanted)
Attachments
(1 file, 3 obsolete files)
7.62 KB,
patch
|
alvolkov.bgs
:
review+
nelson
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•18 years ago
|
QA Contact: jason.m.reid → libraries
Reporter | ||
Updated•18 years ago
|
Assignee: nelson → nobody
Priority: -- → P2
Summary: NSS encodes cert domainComponents with wrong string type → NSS encodes cert distinguished name attributes with wrong string type
Target Milestone: --- → 3.12
Reporter | ||
Updated•17 years ago
|
Assignee: nobody → neil.williams
Assignee | ||
Comment 1•17 years ago
|
||
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.
Reporter | ||
Comment 2•17 years ago
|
||
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.
Reporter | ||
Comment 3•17 years ago
|
||
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. :-/
Reporter | ||
Comment 4•17 years ago
|
||
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.
Version: 3.10.1 → 3.9
Assignee | ||
Comment 5•17 years ago
|
||
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.
Status: NEW → ASSIGNED
Version: 3.9 → 3.10.1
Assignee | ||
Comment 6•17 years ago
|
||
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?
Reporter | ||
Comment 7•17 years ago
|
||
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.
Reporter | ||
Comment 8•17 years ago
|
||
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.
Reporter | ||
Comment 9•17 years ago
|
||
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.
Reporter | ||
Comment 10•17 years ago
|
||
(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.
Assignee | ||
Comment 11•17 years ago
|
||
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.
Attachment #256127 -
Attachment is obsolete: true
Reporter | ||
Comment 12•17 years ago
|
||
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.
Reporter | ||
Comment 13•17 years ago
|
||
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.
Assignee | ||
Comment 14•17 years ago
|
||
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.
Attachment #262321 -
Flags: review?(nelson)
Reporter | ||
Comment 15•17 years ago
|
||
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 .
Attachment #262321 -
Flags: review?(nelson) → review-
Assignee | ||
Comment 16•17 years ago
|
||
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.
Assignee | ||
Comment 17•17 years ago
|
||
Attachment #262321 -
Attachment is obsolete: true
Attachment #263087 -
Flags: review?(nelson)
Reporter | ||
Comment 18•17 years ago
|
||
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
Attachment #263087 -
Flags: review?(nelson)
Attachment #263087 -
Flags: review?(alexei.volkov.bugs)
Attachment #263087 -
Flags: review+
Assignee | ||
Comment 19•17 years ago
|
||
Addresses Alexei's comments.
Attachment #263087 -
Attachment is obsolete: true
Attachment #263305 -
Flags: review?(alexei.volkov.bugs)
Attachment #263087 -
Flags: review?(alexei.volkov.bugs)
Assignee | ||
Comment 20•17 years ago
|
||
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•17 years ago
|
||
Comment on attachment 263305 [details] [diff] [review] check printableString values for validity r=alexei.volkov
Attachment #263305 -
Flags: review?(alexei.volkov.bugs) → review+
Reporter | ||
Comment 22•17 years ago
|
||
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.
Attachment #263305 -
Flags: review?(alexei.volkov.bugs)
Updated•17 years ago
|
Attachment #263305 -
Flags: review?(alexei.volkov.bugs) → review+
Assignee | ||
Comment 23•17 years ago
|
||
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.
Assignee | ||
Comment 24•17 years ago
|
||
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
Assignee | ||
Updated•17 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•