476126 - CERT_AsciiToName fails when AVAs in an RDN are separated by '+'

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Patch v1 for NSS Trunk

When an RDN has multiple AVAs, they are separated in the ASCII string form
by '+' rather than by ',' or ';'.  CERT_NameToAscii generates that form 
of separator correctly, but CERT_AsciiToName fails when it encounters a '+'
separator.  This means that commands like certutil, which take a DN in ASCII
form on the command line, fail if the DN uses '+' to separate any AVAs.
CERT_AsciiToName is presently unable to create RDNs with multiple AVAs.
There's an existing function for adding AVAs to RDNs, and it is unused.
It's amazing that we've not run into this problem before now.

The attached patch seems to fix it.  Julien, please review it.
Sun CR 6799382.

Comment 1

[Julien Pierre]

Comment on attachment 359711 [details] [diff] [review]
Patch v1 for NSS Trunk

I think this patch is OK, but in ParseRFC1485Name, there should probably be a comment about the

if (bp[-1] != '+')

test. It appears to be reading undefined memory. It doesn't really, because bp has been incremented due to the successful call to CERT_ParseRFC1485AVA . 
It would be more elegant to add an argument to that function to signify that the RDN is done when + is encountered, so the caller doesn't have to parse backwards. That's a stylistic matter so I won't block the patch for that, as long as a comment is added.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Checking in alg1485.c; new revision: 1.29; previous revision: 1.28

I will add a comment, as Julien suggested.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Attachment: supplemental patch

This patch adds an explicit definition for the behavior of the AVA parsing
function, and makes the function behave according to that definition.
It also makes the function static.  It is basically a clean up.  
Julien, please review.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Checking in certdb/alg1485.c; new revision: 1.30; previous revision: 1.29