crlutil incorrectly encoding issuerAltNames extension

NEW
Assigned to

Status

NSS
Tools
P2
normal
13 years ago
9 years ago

People

(Reporter: Alexei Volkov, Assigned: Alexei Volkov)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Assignee)

Description

13 years ago
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:caemail@ca.com|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]
                caemail@ca.com
            [2]
                ca.com

Updated

13 years ago
Assignee: wtchang → alexei.volkov.bugs
(Assignee)

Comment 1

13 years ago
Same warning is issued when creating crl with authKeyId extension
(Assignee)

Comment 2

13 years ago
Created attachment 182618 [details] [diff] [review]
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 on attachment 182618 [details] [diff] [review]
bug fix patch

Please submit ALL patches using the output of diff -u, not diff -c.
Attachment #182618 - Flags: review-
(Assignee)

Comment 4

13 years ago
Created attachment 182689 [details] [diff] [review]
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
(Assignee)

Updated

13 years ago
Attachment #182689 - Flags: review?(julien.pierre.bugs)

Comment 5

13 years ago
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.
Attachment #182689 - Flags: review?(julien.pierre.bugs) → review-

Updated

13 years ago
Priority: -- → P2
Hardware: PC → All
Target Milestone: --- → 3.10.1
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.
Summary: crlutil reports warning when creating crl with issuerAltNames extension → crlutil incorrectly encoding issuerAltNames extension
QA Contact: bishakhabanerjee → jason.m.reid

Updated

12 years ago
Target Milestone: 3.10.1 → 3.11
Blocks: 265003
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?
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.
Priority: P2 → P1
Target Milestone: 3.11 → 3.11.1
Adding Neil to CC list.
Created attachment 209774 [details] [diff] [review]
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.
Attachment #209774 - Flags: review?
Attachment #209774 - Flags: review? → review?(julien.pierre.bugs)

Updated

12 years ago
Attachment #209774 - Flags: review?(julien.pierre.bugs) → review+
Work around checked in on trunk.
Checking in cert.sh; new revision: 1.31; previous revision: 1.30
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.
Priority: P1 → P2
Attachment #209774 - Attachment description: workaround, don't encode x400 addresses, don't print CRL contents → workaround, don't encode x400 addresses, don't print CRL contents (checked in)
QA Contact: jason.m.reid → tools
(Assignee)

Comment 13

11 years ago
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.
Target Milestone: 3.11.1 → 3.12
(Assignee)

Updated

11 years ago
No longer blocks: 265003
Unsetting target milestone in unresolved bugs whose targets have passed.
Target Milestone: 3.12 → ---
You need to log in before you can comment on or make changes to this bug.