CERT_NameToAscii fails when cert name attribute too long

RESOLVED FIXED in 3.9.1

Status

NSS
Libraries
P3
normal
RESOLVED FIXED
14 years ago
14 years ago

People

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

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

The End Entity cert for NIST test 4.3.10, which is in a file named
ValidRolloverfromPrintableStringtoUTF8StringTest10EE.crt, has a
commonName attribute in the Subject Name that is 71 characters long. 
The commonName in question is:

Valid Rollover from PrintableString to UTF8String EE Certificate Test10 

NSS fails this test in two different ways.  This bug report is about one
of those failures.

NSS is unable to print the subject name of that cert because of the length
of the commonName attribute.  

RFC 3280 defines upper bounds for the lengths of the various attribute 
types in a Name.  The bound for a commonName attribute is 64 characters.  

NSS enforces that limit in CERT_CreateAVA and in CERT_NameToAscii.  
Both operations simply fail if any attribute's length exceeds its respective
upper bound.  If any attribute is too long, CERT_NameToAscii returns NULL.
It doesn't return a string that contains any useful information about the
other attributes, or about the attribute that was too long.  I think that's
bad.  

Some alternatives for CERT_NameToAscii are:

1. when an attribute is too long, simply truncate it to the maximum length,
and go on.

2. when an attribute is too long, display "Value Too Long" instead of the 
actual value.

3. remove the length checks entirely

4. Raise the length limits so that no limit is less than, say, 128 or 256.
(Assignee)

Comment 1

14 years ago
*** Bug 216119 has been marked as a duplicate of this bug. ***

Comment 2

14 years ago
Nelson, now that NIST acknowledged that that test was
incorrect, I think this bug can be resolved INVALID,
right?  Do we want to be more lenient about the length
of cert name attributes?
(Assignee)

Comment 3

14 years ago
I propose to implement alternative #1 above in time for NSS 3.9.
It should only be a few lines of code, and it will make the programs that
depend on this function MUCH more useful in the presence of certs that are
slightly out of spec.
Status: NEW → ASSIGNED
Priority: -- → P3
Summary: NIST test failure when cert name attribute too long → CERT_NameToAscii fails when cert name attribute too long
Target Milestone: --- → 3.9
(Assignee)

Comment 4

14 years ago
Retargetting to 3.9.1.  Patch forthcoming.
Target Milestone: 3.9 → 3.9.1
(Assignee)

Comment 5

14 years ago
Created attachment 139538 [details] [diff] [review]
patch v1

truncate too-long strings, and change last character to "!" to signify
truncation.
(Assignee)

Comment 6

14 years ago
Created attachment 139540 [details]
cert with too-long OU in subject name (base64)

This cert demonstrates the problem and shows the fix.
(Assignee)

Comment 7

14 years ago
Comment on attachment 139540 [details]
cert with too-long OU in subject name (base64)

Wan-Teh, please review.
Attachment #139540 - Flags: review?(wchang0222)
(Assignee)

Comment 8

14 years ago
Comment on attachment 139540 [details]
cert with too-long OU in subject name (base64)

sorry, wrong attachment.
Attachment #139540 - Flags: review?(wchang0222)
(Assignee)

Comment 9

14 years ago
Comment on attachment 139538 [details] [diff] [review]
patch v1

Wan-Teh, please review.
Attachment #139538 - Flags: review?(wchang0222)

Comment 10

14 years ago
Comment on attachment 139538 [details] [diff] [review]
patch v1

Adding "!" is not a strong enough indication that
the string has been truncated.	First, some company
names, for example, "Yahoo!", contain "!".  Second,
this cannot be detected programmatically.  Why don't
we add an error code to report this condition?

>+	** XXX Perhaps we should see if we're in the middle of a 
>+	** multi-byte UTF8 character.

This is an issue.
(Assignee)

Comment 11

14 years ago
Wan-Teh, previouslt this code did detect the condition and report it with 
an error.  This patch changes it to not do that, and display something 
useful to a user instead.  

This function is used to generate strings that are displayed, e.g. by 
certutil or pp.  The present code (which outputs an error and no string)
makes it impossible for a user of those programs to see the name in a 
cert that contains an invalid (too long) name attribute.  

The point of this patch is to give the user a readable useful string, 
while still calling attention to the error in a humanly readable way.

As for being programmatically detectable, I submit that there are other
APIs that should be used for that purpose.  

I will look into the UTF8 issue.

Comment 12

14 years ago
Thanks for the explanation.

I think "..." is a more common indication of truncation
than "!".

Is the string in question always UTF-8?
(Assignee)

Comment 13

14 years ago
taking bug. 
Assignee: wchang0222 → MisterSSL
Status: ASSIGNED → NEW
(Assignee)

Comment 14

14 years ago
Created attachment 139638 [details] [diff] [review]
patch v2

Deal with multi-byte UTF9 characters.  
Add elipsis (...) to show string too long.
Attachment #139538 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #139538 - Flags: review?(wchang0222)
(Assignee)

Comment 15

14 years ago
Comment on attachment 139638 [details] [diff] [review]
patch v2

Wan-Teh, please review.
This patch addresses your review comments to the previous patch.
Attachment #139638 - Flags: review?(wchang0222)
(Assignee)

Comment 16

14 years ago
Before and after pictures:
Before:
        Subject:
            "!Invalid AVA!"

After:  (slightly edited here to preserve some anonymity of cert holder)
        Subject:
            "CN=www.br****way.tv,OU=Domain Control Validated,OU=See www.geo**
            ***.com/qu****sl/cps (c)03,OU=Business Registration: https://serv
            ices.****c****nt.net/get.jsp?...,O=www.br****way.tv,C=US"
See the ellipsis ------------------------^^^
The results are much more useful.

Comment 17

14 years ago
Comment on attachment 139638 [details] [diff] [review]
patch v2

r=wtc.

1. I find the test (avaValue->len > maxLen + 3) a little
hard to understand. I would write the test like this:

    if (avaValue->len > maxLen) {
	maxLen -= 3; /* must be room for "..." */

2. I don't know how to detect whether we are in the middle
of a multi-byte UTF-8 character, so I'll trust that your
test ((xxx & 0xc0) == 0x80) is correct.

3. Does maxLen mean the number of bytes or the number of
UTF-8 characters?
Attachment #139638 - Flags: review?(wchang0222) → review+

Comment 18

14 years ago
Comment on attachment 139638 [details] [diff] [review]
patch v2

If maxLen may be < 3, my proposed rewrite may reduce maxLen
below 0.  To address that will make the code complicated and
Nelson's version will turn out to be simpler.
(Assignee)

Comment 19

14 years ago
Answers to comment 17

1. The new code wants to display up to maxLen characters from the original 
string, PLUS 3 dots, so that is what the test tests.  To avoid reallocating, 
we ensure that the existing string has room for the 3 dots.  Yes, this means 
that if a string exceeds maxLen by only 1 or 2, we let it slide.

2. The test I used was taken from John Meyer's new UTF8 parser code.  That
code expects the first byte to encode the length of the rest, and the 
remaining bytes will match the test given.

3. I have wondered about this myself.  I have not found an authoritative 
answer to this question.  As implemented, maxLen means bytes.  

This could be changed.  I have thought about implementing special NSS UTF8
functions that return the character count in a UTF8 string (UTF8_strlen), 
or copy a UTF8 string up to some character limit (UTF8_strncpy).  

A related question is: how should command line tools display UTF8 strings? 
e.g. What (if anything) can pp or certutil do to cause the charaacters to 
be displayed properly ?

For Comment 18:

That's why I want to display the elipsis _after_ the allowed length of 
characters.
(Assignee)

Comment 20

14 years ago
/cvsroot/mozilla/security/nss/lib/certdb/alg1485.c,v  <--  alg1485.c
new revision: 1.19; previous revision: 1.18
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Comment 21

14 years ago
(In reply to comment #19)
> A related question is: how should command line tools display UTF8 strings? 
> e.g. What (if anything) can pp or certutil do to cause the charaacters to 
> be displayed properly ?

If you don't want to assume UTF-8, you could use something like
iconv_open("char", "UTF-8") to convert to the locale's charset.
You need to log in before you can comment on or make changes to this bug.