220855 - CERT_NameToAscii fails when cert name attribute too long

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P3)

Description

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

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

Comment 2

[Wan-Teh Chang]

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?

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Retargetting to 3.9.1.  Patch forthcoming.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v1

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

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Attachment: cert with too-long OU in subject name (base64)

This cert demonstrates the problem and shows the fix.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

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

Wan-Teh, please review.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

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

sorry, wrong attachment.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 139538 [details] [diff] [review]
patch v1

Wan-Teh, please review.

Comment 10

[Wan-Teh Chang]

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.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

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

[Wan-Teh Chang]

Thanks for the explanation.

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

Is the string in question always UTF-8?

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

taking bug. 

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v2

Deal with multi-byte UTF9 characters.  
Add elipsis (...) to show string too long.

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 139638 [details] [diff] [review]
patch v2

Wan-Teh, please review.
This patch addresses your review comments to the previous patch.

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

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

[Wan-Teh Chang]

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?

Comment 18

[Wan-Teh Chang]

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.

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

/cvsroot/mozilla/security/nss/lib/certdb/alg1485.c,v  <--  alg1485.c
new revision: 1.19; previous revision: 1.18

Comment 21

[John G. Myers]

(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.