292285 - crlutil incorrectly encoding issuerAltNames extension

Status: NEW

(NSS :: Tools, defect, P2)

Description

[Alexei Volkov]

Example that was taken from nss/tests/cerl.sh:


crlutil -d /export/SC/ws/nss-3.10-docs/mozilla/tests_results/security/goa1.2/CA 
-G -n TestCA -f ../tests.pw.30857 -o ../server/root.crl_40-42_or<<EOF_CRLINI
addext issuerAltNames 0
"rfc822Name:[e-mail]|dnsName:ca.com|x400Address:x400Address|directoryName:CN=NSS
Test CA,O=BOGUS NSS,L=Mountain
View,ST=California,C=US|URI:http://ca.com|ipAddress:192.168.0.1|registerID=reg CA"
EOF_CRLINI
CRL Info:
:
    Version: 2 (0x1)
    Signature Algorithm: PKCS #1 MD5 With RSA Encryption
    Issuer: "CN=NSS Test CA,O=BOGUS NSS,L=Mountain View,ST=California,C=US"
    This Update: Thu Apr 28 16:52:46 2005
    Entry (1):
        Serial Number: 41 (0x29)
        Revocation Date: Thu Apr 28 16:52:15 2005
        Entry Extensions:
            Name: CRL reason code

    CRL Extensions:
        Name: Certificate Issuer Alt Name
        Error: Parsing extension: security library: improperly formatted DER-enc
oded message.
        Data: Sequence {
            [1]
                [e-mail]
            [2]
                ca.com

Comment 1

[Alexei Volkov]

Same warning is issued when creating crl with authKeyId extension

Comment 2

[Alexei Volkov]

Attachment: bug fix patch

The problem is in incorrect encoding technique for the extension. The following

member of the template was not properly encoded:
certEDIPartyName,
certOtherName,
certX400Address.

X400 Address has the following template:
static const SEC_ASN1Template CERT_X400AddressTemplate[] = {
    { SEC_ASN1_CONTEXT_SPECIFIC | SEC_ASN1_CONSTRUCTED | 3,
	  offsetof(CERTGeneralName, name.other), SEC_AnyTemplate,
	  sizeof (CERTGeneralName)}
};

and should be encoded with bits
SEC_ASN1_CONTEXT_SPECIFIC | SEC_ASN1_CONSTRUCTED
e.t.c (0x80 | 0x20), not only with 0x80. See patch for details.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 182618 [details] [diff] [review]
bug fix patch

Please submit ALL patches using the output of diff -u, not diff -c.

Comment 4

[Alexei Volkov]

Attachment: bug fix patch (diff -u)

Same patch created by running diff with -u option.

Also added a fix for memory corruption problem in crlgen.c

Comment 5

[Julien Pierre]

Comment on attachment 182689 [details] [diff] [review]
bug fix patch (diff -u)

The patch looks OK, except you should use the macros from secasn1t.h rather the
hardcoded values .

Eg. 0x1f should be SEC_ASN1_TAGNUM_MASK, 0x80 should be
SEC_ASN1_CONTEXT_SPECIFIC,  0x20 should be SEC_ASN1_CONSTRUCTED .

This will make the code much clearer.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 182689 [details] [diff] [review]
bug fix patch (diff -u)

This patch changes code that modifies the ASN.1 encoding of data in 
an input buffer.  I'd like to see a block comment explaining this.  
I'd like it to explain how it is that the value in the buffer is 
already encoded, why/how the encoded value can possibly be incorrect, 
and why it is changed here.  

Isn't it unusual for an encoding function or a decoding/printing function to
modify its input?  Is that really a good idea?

I realize that the code was going this before the patch, and that 
the patch doesn't alter the fact that we're modifying the input, 
but only alters the specific change being made to the input.  
But I think the fact that we need to change this raises some 
questions about this design, and we should answer them before 
proceeding.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Please ignore my comment 6 above.   It clearly doesn't apply to this patch.
Perhaps I added the comment to the wrong bug.

So, the problem is that crlutil attempted to construct an extension
containing an X400address, and encoded the X400 Address without the 
CONSTRUCTED bit in the tag.  The proposed fix will add the missing 
CONSTRUCTED bit to the tag.  

But I think that creates (or rather, uncovers) another problem, which is 
that the data that we're trying to encode as the body of the X400Address 
is just a simple printable string, and is not another DER construct. 
In other words, the data inside the X400Address is not really constructed
at all.  So, when we encode it as constructed, I think it will cause a 
different error, namely an invalid construction in a constructed type.

The constructed bit in the tag implies that another tag will immediately
follow this tag.  But in this case, the data that follows the X400Address
tag is merely the string "x400Address".  I expect DER decoding errors to
result from the attempt to interpret "x" as a tag and "4" as a length.

I'll bet that if you take the existing CRL produced by the current crlutil
(the output that gets that error about invalid encoding), and run it through
a recursive descent parser, such as dumpasn1, that it will PASS (no error).
I'll further bet that if you add the constructed bit, crlutil may stop 
complaining, but dumpasn1 will start to complain.  Please test that theory.

Maybe we simply should not encode any X400Address, certOtherName, or certEDIPartyNames until we have a way to properly construct them as 
constructed types.  

Julien, what do you think of that suggestion?

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

I'm raising the priority of this to P1 for 3.11.1.
This build problem was introduced 9 months ago, and STILL isn't fixed.
Alexei, Some solution needs to be put in place very soon.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Adding Neil to CC list.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Attachment: workaround, don't encode x400 addresses, don't print CRL contents (checked in)

This patch does two things:
a) it causes crlutil to no longer attempt to encode x400 names as alt names,
   thereby avoiding the error about incorrectly encoded CRLs,
b) it quiets crlutil, so that it no longer pretty prints a complete CRL into 
   the QA output log each time it creates or modifies a CRL.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Work around checked in on trunk.
Checking in cert.sh; new revision: 1.31; previous revision: 1.30

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Workaround Checked in on NSS 3.11 branch.
Checking in cert.sh;  new revision: 1.28.2.1; previous revision: 1.28

Bug should remain open until a real fix is committed.
Lowering priority to P2 now that workaround is in place.

Comment 13

[Alexei Volkov]

real fix should be made for certutil and crlutil as both of them utilizes the same code. The fix will be made as a part of changes for bug 324744. Reassign to 3.12 release.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Unsetting target milestone in unresolved bugs whose targets have passed.

Comment 15

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, and this bug has priority 'P2'.
:beurdouche, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 16

[Marco Castelluccio [:marco]]

We have modified the bot to only consider P1 as high priority, so I'm cancelling the needinfo here.