221272 - .crt files with mac line-endings not supported

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

If I try to import a .crt file as root CA that uses Macintosh Line-Endings
(0x0D), nothing happens - no error message, no success message, no imported
certificate.

This happens both when loading the file in the browser and when clicking
"Import" in the certificate manager.

Comment 1

[Wan-Teh Chang]

Christian, could you attach this .crt file?

Are you trying to import this .crt file on Macintosh or
Windows?

If you convert the .crt file to have the "correct" line
endings, can you successfully import it?

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

I'm not sure if I have it anymore... I will check

I tried to import it on windows 2000, as the OS field here says.

yes, converting the line-endings made it possible to import it.

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: testcase

while this is not the file I originally tested with, it works just as well

Comment 4

[Kai Engert [:KaiE:]]

PSM simply obtains the raw buffer from the network and passes it to
CERT_KeyFromDERCrl.

I'm reassining the bug to NSS. I'd argue, NSS is the code that analyzes the
content (it could be either binary or ascii), and NSS should make the decision
whether it wants want to handle text files with Mac line endings.

Comment 5

[Wan-Teh Chang]

Bob, could you take a look at this?

Kai, the file in question is a certificate (.crt), not a CRL.
Do you know whether PSM passes the *pathname* or *contents* of
the .crt file to NSS, and to which NSS function?

Comment 6

[Kai Engert [:KaiE:]]

> the file in question is a certificate (.crt), not a CRL

Oops, I misread, thanks for clarifying.

> Do you know whether PSM passes the *pathname* or *contents* of
> the .crt file to NSS, and to which NSS function?

When using the "import from file" in cert manager, we end up in
  nsNSSCertificateDB::ImportCertsFromFile

This function reads raw bytes into a buffer, for a CA cert calls PSM function
nsNSSCertificateDB::ImportCertificates, calling
nsNSSCertificateDB::getCertsFromPackage to decode the buffer, which in turn
makes this call to NSS:

  SECStatus sec_rv = CERT_DecodeCertPackage(NS_REINTERPRET_CAST(char *, data), 
                                            length, collect_certs, 
                                            (void *)collectArgs);

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 136115 [details]
testcase

Changing content type from application/x-x509-ca-cert to
application/octet-stream so that I can download it without the browser
intercepting it, and without any conversion of line endings.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Taking bug.

I reproduced this with the command:
   certutil -d DB -A -i D:/tmp/bug221272.crt -t ",," -n bug221272

Notice the absense of the -a option there.  I found that certutil crashed
when I added the -a option to that command.  

I will shortly attach a patch that addresses both problems.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v1

This patch fixes two separate sets of code in NSS, each of which attempts
to convert a base64-encoded cert into a binary one to be decoded.
Neither set of code worked properly when presented with input containing
only \r and no \n .  The code in certutil actually crashed over this.  

Turns out the -a option to certutil, and certutil's code to convert ascii to 
binary for inputting certs is completely unnecessary, since the function
CERT_DecodeCertPackage will decode either ascii or binary forms.  But the
function is necessary in other paths (e.g. inputtinga cert request), so 
I fixed it and left it alone.

With this patch, the cert can be imported with either of these commands:
     certutil -d DB -A -i D:/tmp/bug221272.crt -t ",," -n bug221272
     certutil -d DB -A -i D:/tmp/bug221272.crt -t ",," -n bug221272 -a
and both work just fine.  the browser should be happy, too.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 141021 [details] [diff] [review]
patch v1

BTW, I meant to add that without this patch, NSS cannot import files with MAC
line endings on ANY platform, including MACos.

Also, this patch moves a bunch of variable declarations, reducing their scope
to the blocks in which they are used.  In the process of doing that, I found
previously unreported unused variables.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Nominating for 3.9.1 since this bug has affected real users, and potentially
affects all Mac users.

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 141021 [details] [diff] [review]
patch v1

Wan-Teh, please review

Comment 13

[Wan-Teh Chang]

Comment on attachment 141021 [details] [diff] [review]
patch v1

r=wtc.

I find the use of 'asc' in SECU_ReadDERFromFile a
little confusing.  It is used for two different
purposes.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Checked in on trunk:

/cvsroot/mozilla/security/nss/cmd/certutil/certutil.c,v  <--  certutil.c
new revision: 1.85; previous revision: 1.84

/cvsroot/mozilla/security/nss/cmd/lib/secutil.c,v  <--  secutil.c
new revision: 1.63; previous revision: 1.62

/cvsroot/mozilla/security/nss/lib/pkcs7/certread.c,v  <--  certread.c
new revision: 1.7; previous revision: 1.6

On NSS 3.9 branch:

/cvsroot/mozilla/security/nss/cmd/certutil/certutil.c,v  <--  certutil.c
new revision: 1.82.16.1; previous revision: 1.82

/cvsroot/mozilla/security/nss/cmd/lib/secutil.c,v  <--  secutil.c
new revision: 1.60.2.2; previous revision: 1.60.2.1

/cvsroot/mozilla/security/nss/lib/pkcs7/certread.c,v  <--  certread.c
new revision: 1.6.78.1; previous revision: 1.6