39494 - DN to ascii conversion doesn't deal with unknown attribute types

Status: VERIFIED FIXED

(NSS :: Libraries, defect, P2)

Description

[Fred Roeber]

This is a replacement bug for Netscape bug
http://scopus/bugsplat/show_bug.cgi?id=56284

RFC1779 specifies a method for representing unknown attribute types when
representing a DN as a string.  I've quoted the relevant section below.
Our current implementation just fails with an error when it finds an unknown
attribute type.

   There is an escape mechanism from the normal user oriented form, so
   that this syntax may be used to print any valid distinguished name.
   This is ugly.  It is expected to be used only in pathological cases.
   There are two parts to this mechanism:

   1.  Attributes types are represented in a (big-endian) dotted
       notation.  (e.g., OID.2.6.53).

   2.  Attribute values are represented in hexadecimal (e.g.  #0A56CF).
       Each pair of hex digits defines an octet, which is the ASN.1 Basic
       Encoding Rules value of the Attribute Value.



------- Additional Comments From gangolli  04/09/97 08:47 ------- 


If you plan to fix this I have some code in my tree for converting
decimal dot-separated oid component lists to DER encoded OIDs.

We use it in the Certificate Server when specifying OIDs in string
form, but yes we don't use it within string distinguished name
translations.




------- Additional Comments From relyea  Jun-11-1999 14:09 ------- 

Here's a bug report for an enhancement I know fred has already planned on 
making...

------- Additional Comments From relyea  Jun-11-1999 14:09 ------- 

Here's a bug report for an enhancement I know fred has already planned on 
making...

Comment 1

[Wan-Teh Chang]

Set Target Milestone 4.0.

Comment 2

[Wan-Teh Chang]

Changed the QA contact to Bishakha.

Comment 3

[Aleksander Adamowski]

Can this bug be the cause for x.509 Certificate Viewer in Mozilla to show values
as hex and names as OIDs for unsupported extensions?

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

RFC 1779 has been superseded by RFC 2253.  
IMO we should target compliance with RFC 2253 instead of 1779.

Comment 5

[Ian McGreer]

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

Comment 6

[Ian McGreer]

Attachment: handle conversion to ASCII for unknown attribute types

This patch mixes in some code from Stan to convert OIDs to dotted-decimal form.

Comment 7

[Ian McGreer]

Attachment: sample cert with serialNumber in the DN

Here is the cert I tested with.

Comment 8

[Wan-Teh Chang]

Ian, please have Nelson review your patch.

Nelson and I discussed this bug before and
he made a suggestion that I think is a good
idea.  Here is what Nelson suggested:

  [RFC] 2253 says that the #<hex> form must
  be used when the attributeValue is not a
  string type, and SHOULD be used when the
  AttributeType is shown in dotted decimal
  form.

  [But] I'd modify [that SHOULD] as follows:

  When the AttributeValue is known to be a
  string type, then it should be shown as a
  string (not in hex), even if the AttributeType
  is shown as an OID.

Comment 9

[Ian McGreer]

That suggestion fits with the cert attached to this bug, as serialNumber is a
PrintableString.

However, the OID for serialNumber is not registered with NSS, so there would be
additional work in registering it, then having it intrepreted accordingly.  I
would prefer to get this patch in now, then work on a case-by-case basis later.

Comment 10

[Ian McGreer]

Comment on attachment 105606 [details] [diff] [review]
handle conversion to ASCII for unknown attribute types


>+      if( (char *)NULL == a ) {
>+        /* This is the first number.. decompose it */
>+        PRUint32 one = (n/40), two = (n%40);
>+        
>+        a = PR_smprintf("%lu.%lu", one, two);

I think this is not correct.  My ASN.1 book says,

 "The first two arcs of the registration tree are encoded on
  a single integer using the formula 'first_arc * 40 +
  second_arc', whcih assumes that there are no more than
  39 arcs just below the iso and itu-t arcs of the root.
  No limitation, however, is imposed on the number of
  arcs under joint-iso-itu-t."

Thus, if the first octet n > 80, the second arc should equal n - 80, not n %
40.  The code above would produce the wrong result whenever the second arc of
joint-iso-itu-t is greater than 119.

Comment 11

[Ian McGreer]

I meant to say n > 119, or the second arc is > 39, of course.

Comment 12

[Ian McGreer]

Attachment: rev 2 of patch

Based on Nelson's review:

- implement comment #8 by ignoring comment #9
- fix described in comment #10

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Ian,  In comment 10, you proposed to change the computation of the variable
named "two".  You changed it from 

+        PRUint32 one = (n/40), two = (n%40);
to
+        PRUint32 one = (n/40);
+        PRUint32 two = n - one * 40;

IINM, those two definitions of "two" are completely equivalent. 
I expected that your new definition of two would be something like this:

         PRUint32 two = (n > 80) ? (n - 80) : (n % 40)

Comment 14

[Ian McGreer]

AFAIK, the value of "one" must be 0, 1, or 2.  The table from my ASN.1 book says
that whenever n >= 80, the first arc is joint-iso-itu-t (2) and the second arc
is n-80.

Thus, the code I used is not the same as n % 80, and should be correct.

Comment 15

[Ian McGreer]

Ah, spoke before I saw the problem.  Really, it should be:

one = (n < 40) : 0 ? (n < 80) : 1 : 2;
two = n - one * 40;

Comment 16

[Ian McGreer]

Attachment: rev 3

implement last suggestion with '?'s and ':'s in the right places.

Comment 17

[Ian McGreer]

Attachment: rev 4

fix indentation, more efficient char-hex conversion

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

r=nelsonb

Comment 19

[Ian McGreer]

marking fixed

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

I verified this in a trunk build today.  Big improvement!

Comment 21

[Robert Relyea]

Comment on attachment 106220 [details] [diff] [review]
rev 4

There's a stack overflow problem here. There is no bounds on the size of the
string returned from get_oid_string, so it is possible to have it return a
string larger than 384. This would cause an overflow when we copy the string
into tmpbuf.

bob

Comment 22

[Wan-Teh Chang]

Attachment: Patch to detect buffer overflow

This patch should be applied on top of patch rev 4
(attachment 106220 [details] [diff] [review]).

Two open issues.

1. I set the error code to SEC_ERROR_OUTPUT_LEN if
tagName is too long.  This may not be the most
appropriate error code.  We might want to add an
error code, such as SEC_ERROR_INVALID_TAG.

2. I am freeing unknownTag as soon as it is not
needed.  I think it is easier to understand this
way.  This change has nothing to do with the
buffer overflow.

Comment 23

[Ian McGreer]

Comment on attachment 108149 [details] [diff] [review]
Patch to detect buffer overflow

This looks good to me.	Nelson, Wan-Teh asked if you could sr= it.

Comment 24

[Nelson Bolyard (seldom reads bugmail)]

This code looks correct.  However, it seems odd that this function uses 
both fixed length buffers AND PR_smprintf.  PR_smprintf is used when you 
want the code to be correct (no overflows) and have no fixed length limits.  
So to use PR_smprintf, and then constrain the output to a fixed length limit
seems odd.  

So, while I believe this patch is correct, I'd prefer to see this code
use PR_smprintf instead of the fixed length buffer, and eliminate the 
384 byte length limit entirely.  But that's up to you.  

sr=nelsonb (second review, not super review)

Comment 25

[Asa Dotzler [:asa]]

"blocking1.3a ?" is the approprite nominatio mechanism to bring a _bug_ to the
attention of drivers. The flag, "approval1.3a ?" (in the attachment manager) is
how to make an approval request of [e-mail] for a specific patch. "+"
on either of those flags is reserved for [e-mail]. 

Comment 26

[Asa Dotzler [:asa]]

Comment on attachment 108149 [details] [diff] [review]
Patch to detect buffer overflow

a=asa for checkin to 1.3a

Comment 27

[Wan-Teh Chang]

Comment on attachment 108149 [details] [diff] [review]
Patch to detect buffer overflow

This patch has been checked into the trunk, NSS_3_7_BRANCH,
and NSS_CLIENT_TAG of NSS.

It has also been checked into the NSS_3_3_BRANCH.  It has
not yet been checked into the NSS_3_6_BRANCH.