208649 - CERT_CompareAVA compares improperly

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Wan-Teh Chang]

CERT_CompareAVA has an if statement with only
comments in its body.  That if statement is
wrong.  I believe that the comparison of the
AVAs' values should be inside the body of the
if statement.

Comment 1

[Wan-Teh Chang]

Attachment: Proposed patch

This is different from what I suggested earlier.
I thought 'rv' has the type SECStatus, but it
has the type SECComparison, so the meaning of
"if (rv)" is different.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

The logic of this function is certainly flawed.  
The flaws go well beyond the fact that it ignores the attribute type.
There are actually 3 important characteristics of an AVA:
1. The attribute type (e.g. O, OU, L, ST, CN, etc.)
2. The encoding of the attribute value (e.g. IA5 string, T61 string, 
   printable string, UTF8 string, universal string, etc.)
3. The encoded value of the attribute.

It is true that if the attribute types do not match exactly, then the 
function should indicate that they do not match.  

If the types match but the encodings do not, then the two strings should
be converted to a common encoding for comparison.  Further, for printable
encoding, RFC 3280 says:

      (c)  attribute values in PrintableString are not case sensitive

      (d)  attribute values in PrintableString are compared after
      removing leading and trailing white space and converting internal
      substrings of one or more consecutive white space characters to a
      single space.

The function in question does not take value encoding into account, and 
does not handle PrintableString any differently than the other encodings.

It's not immediately obvious how this function would determine what encoding
had been used in any particular AVA.  The means by which that is done must
be determined before this can be fixed.

Comment 3

[Wan-Teh Chang]

Nelson, may I check in my patch first and leave the bug open
until the issue with encoding is also resolved?

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

The logic change in the above patch seems fine.  
Please remove the existing XXX comment and add one that reminds us that the 
second SECITEM_CompareItem call neither does case insensitive comparison nor
whitespace canonicalization for the comparison of PrintableStrings.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

It appears that both of NSS's active ASN.1 decoders discard the information 
about the encoding of the items.  When a template specifies a particular 
type of ASN.1 encoding, then the decoder checks that the actual encoding 
matches the template.  But when the template is an "ANY" template, the decoder
discards all information about the decoded object.  The decoder tends to 
set the "type" field of each decoded SECITEM to siBuffer (which is zero).

The template for an AVA is 

static const SEC_ASN1Template cert_AVATemplate[] = {
    { SEC_ASN1_SEQUENCE,  0, NULL, sizeof(CERTAVA) },
    { SEC_ASN1_OBJECT_ID, offsetof(CERTAVA,type),  },
    { SEC_ASN1_ANY,       offsetof(CERTAVA,value), },
    { 0, }
};

Here are some ideas I have for changing the decoder and/or templates to 
not lose the information about the encoding of the AVA values.

1.  Stop using the SEC_ASN1_ANY type in the above template, and change 
the template to use a SEC_ASN1_CHOICE of types that correspond to the 
types expected in all known AVAs.  Requires no change to the decoders,
and several changes to the templates.  May or may not be possible to do
this and remain binary compatible with existing structures that correspond
to the existing templates.  Might be possible if the decoder can store the 
choice value in an unused secitem's type field.

2. Change the decoders to store the tag for a SEC_ASN1_ANY item into the
corresponding SECITEM's type field, which otherwise will contain siBuffer.
This should be fine for any code except code that expects/requires the 
type field to contain siBuffer.  It is doubtful that any existing code 
cares about the value in item->type after the decoder finishes.
This option requires no change to any templates.  or

3. Add another flag to the template, one that means "store the decoded tag
into the item's type field.  Change the decoders to honor this flag.  
The flag could work for types other than SEC_ASN1_ANY.

4. A variant of items 2-3 above.  Change the declaration of SECITEM so 
that the "type" field is just an unsigned short or unsigned char, and 
use the remaining parts to store the decoded tag.  Tag could conceivably
be store for ALL objects, not just ANYs, and without requirign a new flag.

Example:  Change from
struct SECItemStr {
    SECItemType type;
    unsigned char *data;
    unsigned int len;
};
to
struct SECItemStr {
    unsigned short reserved;
    unsigned char  tag;    
    unsigned char  type;
    unsigned char *data;
    unsigned int len;
};

5. another variant of 2-3.  Store the tag in the type field shiften a few bits,
e.g. change the decoder from
   item->type = siBuffer;
to 
   item->type = (SECItemType)(siBuffer | (tag << 8));

Note that GeneralNames have exactly the same problem as directoryName AVAs.
SO, of these choices, the options that store the tag in the item type 
regardless of the template or tag type appeal to me the most because General
Names would benefit from this as well as directoryNames.  

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Here's another point of information that may be relevant.

There's a function named CERT_DecodeAVAValue that takes as its argument the
address of the SECITEM corresponding to the ANY value.  It clearly expects 
that the "value" is still DER encoded, and has not had its DER encoding 
removed.  It expects that item->value[0] will be the tag.  I'm not sure 
that code is correct, but if it is then it will be pretty easy to fix
CERT_CompareAVA to pay proper attention to the encoding type.

But this may also mean that NSS needs different solutions for directoryNames
than for GeneralNames.

Comment 7

[Wan-Teh Chang]

Assigned the bug to Nelson.

I haven't checked in my patch.  Please mark it obsolete
when you attach your patch.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Patch forthcoming

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v2, rewrite

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Checking in cert.h;    new revision: 1.36; previous revision: 1.35
Checking in secname.c; new revision: 1.14; previous revision: 1.13

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

mark dependency

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Attachment: v3, supplemental (incremental) patch

Since patch v1 above was deemed to contain unnecessary changes, when I produced

patch v2 above, I attempted to remove all the unnecessary changes.  In the 
process, I removed several NULL pointer tests that I had added in V1.

I decided to develop a series of smaller patches that could be applied
incrementally, that (together with patch v2) would add up to patch v1.	
In the process of developing the first of these supplemental patches,
I discovered the null pointer tests that had been omitted from patch v2.

So, here is patch v3 which contains 
a) the missing null pointer tests that I found
b) changes all PORT_*Alloc calls that allocate new structs or arrays of same
to use the PORT_*New* macros instead.  
c) ifdefs out some dead functions that are rife with failure to detect 
allocation failures.