487487 - CERT_NameToAscii reports "!Invalid AVA!" whenever value exceeds 384 bytes

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

This is probably a regression in 3.12, but it might be an old bug.
In NSS 3.12, the name-to-ASCII functions were given several choices 
for behavior:

fully invertable - even if not very readable
very readable - even if some data is discarded (truncated) to make it so
strict RFC compliance - even if neither invertable nor readable.

Old CERT_NameToAscii performs the "readable" behavior.  New function 
CERT_NameToAsciiInvertable takes an argument telling it which mode to use.

In readable mode, Each entire Attribute Value Assertion (AVA, also known as
an Attribute Type And Value, ATAV, which is a NAME=VALUE string) is supposed 
to be truncated (if necessary) to fit in 384 bytes, including the = and the 
trailing comma (or plus).  (No, I don't know where that magic number comes 
from.)

But experimentation shows that whenever the value string itself exceeds that
length, the entire string prints as "!Invalid AVA!".  

The problem is in function AppendAVA in file alg1485.c.  The truncation 
logic fails to consider the length of the attribute name, so it truncates 
the value to 384 bytes, but then adds the attribute name, and the result is
longer than 384 bytes.  

This is major because it makes real world certs undisplayable.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

A typical failure stack is:

nss3.dll!AppendAVA(... CERT_N2A_READABLE)                   Line 1006
nss3.dll!CERT_NameToAsciiInvertible(... CERT_N2A_READABLE)  Line 1084
nss3.dll!CERT_NameToAscii()                                 Line 1100 
pp.exe!SECU_PrintName()                                     Line 2365
pp.exe!SECU_PrintCertificate()                              Line 2592
pp.exe!SECU_PrintSignedData()                               Line 3276
pp.exe!main()                                               Line 154

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Patch v1 (checked in)

With this patch, the subject and issuer names in bug 487381 and bug 486304 
are displayed as something more intelligent than "!Invalid AVA!".

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 372191 [details] [diff] [review]
Patch v1 (checked in)

Kaspar, Please review this.  I've only gotten it working well enough to work on the other bugs of which you're aware.  But perhaps it's good to go.

Comment 4

[Robert Relyea]

Comment on attachment 372191 [details] [diff] [review]
Patch v1 (checked in)

r+ I particularly noted that the UTF8 processing was correctly preserved where it was needed (and where it wasn't appropriate comments explained why).

bob

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Checking in certdb/alg1485.c; new revision: 1.35; previous revision: 1.34

I'm taking a risk here, checking in while the tree is orange, but my own 
all.sh passes (when I remove the chains.sh script)

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 372191 [details] [diff] [review]
Patch v1 (checked in)

Any further reviews are always welcome but are no longer required.

Comment 7

[Kaspar]

Comment on attachment 372191 [details] [diff] [review]
Patch v1 (checked in)

>+    } else {
>+	/* must truncate the escaped and quoted value */
>+	char bigTmpBuf[TMPBUF_LEN * 3 + 3];
>+	rv = escapeAndQuote(bigTmpBuf, sizeof bigTmpBuf,
>+			    (char *)avaValue->data, valueLen, &mode);
>+
>+	bigTmpBuf[valueLen--] = 0; /* hard stop here */
>+	while (((bigTmpBuf[valueLen] & 0xc0) == 0x80) && valueLen > 0) {
>+	    bigTmpBuf[valueLen--] = 0;
>+	}
>+	/* add elipsis to signify truncation. */
>+	bigTmpBuf[++valueLen] = '.';
>+	bigTmpBuf[++valueLen] = '.';
>+	bigTmpBuf[++valueLen] = '.';
>+	if (bigTmpBuf[0] == '"')
>+	    bigTmpBuf[++valueLen] = '"';
>+	bigTmpBuf[++valueLen] = 0;
>+	PORT_Assert(nameLen + valueLen <= sizeof tmpBuf);
>+	memcpy(encodedAVA + nameLen, bigTmpBuf, valueLen);

The memcpy call does not zero-terminate encodedAVA, so the string might end up with a garbage character (depending on what tmpBuf was originally initialized to). Using valueLen+1 does the trick, but I think there's another issue lurking here: if you add the trailing quote (in case bigTmpBuf had to be quoted), then you run into an off-by-one error regarding valueLen. r- for these reasons, whatever significance that may have now, though (after the commit).

Nit: elipsis should have another "l", and I found the comment referring to the UTF-8 character boundary quite helpful - "((bigTmpBuf[valueLen] & 0xc0) == 0x80)" isn't really self-explaining, IMO.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Kaspar, thanks very much for your review comments.  They were just what 
I hoped they would be.  I stopped working on this bug when the code was
working well enough that I could see at least SOME of the long name.
That enabled me to get on with the really pressing bugs with the cert8 DB.

Would you like to write a supplementary patch to address the issues you found?  
I won't be able to get to this for at least another day or two, I'm afraid, 
and so it it waits for me, it will probably not be done in time for the 
3.12.4 RC0 cutoff (Monday).

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

This bug is not fully fixed, but it IS fixed _enough_ that it no longer 
blocks bug 487381.  Nonetheless, I'm marking as blocking bug 487381 merely
as an aid to keeping track of related bugs.

Comment 10

[Kaspar]

Attachment: Supplementary patch, as suggested in comment 8

Something like this should do the trick. If the value isn't quoted (and doesn't need a trailing " therefore), we're "wasting" an extra character - but I don't think that's a real problem, since we're always showing at least 188 characters when truncating the value. For the sake of consistency, I have replaced 0 by '\0'.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 372393 [details] [diff] [review]
Supplementary patch, as suggested in comment 8

r=nelson
Thanks, Kaspar

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

I committed Kaspar's patch.  Thanks, Kaspar.
Checking in alg1485.c; new revision: 1.37; previous revision: 1.36

Comment 13

[Robert Relyea]

Comment on attachment 372393 [details] [diff] [review]
Supplementary patch, as suggested in comment 8

r+ though only one review was needed.