528743 - Multiple problems in libpkix's AIA cert fetch code exposed by the https://www.unosoft.hu/ cert

Status: NEW

(NSS :: Libraries, defect, P2)

Description

[Wan-Teh Chang]

Attachment: Certificate for https://www.unosoft.hu/

The cert for https://www.unosoft.hu/ has an AIA extension with an
LDAP URL that has no host name:

            Name: Authority Information Access
            Method: PKIX CA issuers access method
            Location: 
                URI: "ldap:///CN=UNO-SOFT%20RootCA,CN=AIA,CN=Public%20Key%20S
                    ervices,CN=Services,CN=Configuration,DC=unosoft,DC=local?
                    cACertificate?base?objectClass=certificationAuthority"
            Method: PKIX CA issuers access method
            Location: 
                URI: "http://unodc.unosoft.local/CertEnroll/UNODC.unosoft.loc
                    al_UNO-SOFT%20RootCA.crt"

This causes libpkix's AIA cert fetch code to malfunction.  I
attached the cert.  This cert is an important test case for
libpkix's AIA cert fetch code.

libpkix first throws a null-argument exception when it hashes
the host name (an empty string) in the LDAP URL.  I filed bug
528741 about this issue.

When I fixed the empty string hash issue, CERT_PKIXVerifyCert
fails with PR_DIRECTORY_LOOKUP_ERROR (i.e., DNS host name
resolution failure).  I presume the LDAP client tried to
resolve the empty string host name and failed.  Anyway, this
shows several problems.

1. Only the bottommost layer set an error code
(PR_DIRECTORY_LOOKUP_ERROR).  No other layer set an error code.
So CERT_PKIXVerifyCert cannot but report PR_DIRECTORY_LOOKUP_ERROR,
which is not meaningful to the caller who's trying to verify a
cert.  An error code like "cert fetch error" or "cert chain building
failure" would be more meaningful in this context.

2. The AIA cert fetch code gives up after the problem with the
LDAP URL, even though the AIA extension also has an HTTP URL.
The AIA cert fetch code should try the next URL.

3. Even if all the URLs in the AIA extension fail, libpkix should
not abort the cert path construction and validation.  I didn't
study this part of libpkix, so I won't speculate if libpkix actually
abort the cert chain construction and validation when AIA cert fetch
fails.

Comment 1

[Wan-Teh Chang]

Here is the chain of PKIX_Error structures when libpkix verifies
the https://www.unosoft.hu/ certificate with AIA cert fetch:

$10 = {errCode = PKIX_BUILDINITIATEBUILDCHAINFAILED, 
  errClass = PKIX_BUILD_ERROR, plErr = 0, cause = 0x1ac3628, info = 0x0}

$11 = {errCode = PKIX_UNABLETOBUILDCHAIN, errClass = PKIX_BUILD_ERROR, 
  plErr = 0, cause = 0x1aa7d78, info = 0x0}

$12 = {errCode = PKIX_BUILDFORWARDDEPTHFIRSTSEARCHFAILED, 
  errClass = PKIX_BUILD_ERROR, plErr = 0, cause = 0x1a93dd8, info = 0x0}

$13 = {errCode = PKIX_AIAMGRGETHTTPCERTSFAILED, errClass = PKIX_AIAMGR_ERROR, 
  plErr = 0, cause = 0x1ac4ac8, info = 0x0}

$14 = {errCode = PKIX_HTTPSERVERERROR, errClass = PKIX_AIAMGR_ERROR, 
  plErr = 4294961323, cause = 0x0, info = 0x0}
(gdb) print (int)4294961323
$15 = -5973

-5973 is PR_DIRECTORY_LOOKUP_ERROR.

(I've modified PKIX_PL_AIAMgr_GetAIACerts so that it went on to try HTTP
after the LDAP failure.  HTTP also failed because we couldn't resolve
the host name unodc.unosoft.local.)

The plErr field of the PKIX_Error structures is 0 except for the very
last (bottom layer) one.  I haven't figured out how I can set plErr at
one of the higher layers.

Comment 2

[Wan-Teh Chang]

Attachment: Set NSS error to SEC_ERROR_UNKNOWN_ISSUER if PKIX chain building fails

This patch takes a safety net approach.  If libpkix certificate chain
building fails and it sets a non-NSS error, we change the error to
SEC_ERROR_UNKNOWN_ISSUER.

This patch still allows us to improve libpkix's reporting of certificate
chain building errors in the future as long as we set SEC_ERROR_xxx error
codes (for which IS_SEC_ERROR returns true).

Comment 3

[Wan-Teh Chang]

Attachment: Patch for PKIX_PL_AIAMgr_GetAIACerts (work in progress)

I haven't figured out how to do all those PKIX_DECREF calls in
PKIX_PL_AIAMgr_GetAIACerts correctly for both success and failure
cases, so this patch is not ready for checkin.  But it illustrates
what needs to be done to make PKIX_PL_AIAMgr_GetAIACerts try the
next CA issuers access location if one location fails.

Comment 4

[Wan-Teh Chang]

Attachment: Root CA certificate for https://www.unosoft.hu/

This is the certificate of the root CA that issued the server
certificate for https://www.unosoft.hu/.

If I add this root CA certificate in the NSS database and trust
it, CERT_PKIXVerifyCert returns successfully.  I guess this is
because the AIA cert fetch code doesn't need to be used.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Alexei, we need to give this bug attention very soon.

Comment 6

[Wan-Teh Chang]

This kind of certificate turns out to be quite common, as these
Chromium bugs show:
http://crbug.com/27497
http://crbug.com/27970
http://crbug.com/28033

One of the users told me his cert came from a Microsoft Active Directory.
In Chromium I'm working around this bug by retrying CERT_PKIXVerifyCert
without cert_pi_useAIACertFetch if CERT_PKIXVerifyCert with
cert_pi_useAIACertFetch fails with a "bad" error code:
http://codereview.chromium.org/418001

Comment 7

[Wan-Teh Chang]

Attachment: Ignore unknown AIA location types (checked in)

To review this patch, note how we ignore the info access methods
that we're not interested in.  This patch ignores unknown AIA location
types in the same way.

The SEC_ERROR_UNKNOWN_AIA_LOCATION_TYPE error code is no longer
used, so the patch adds a comment to secerr.h about that.

Comment 8

[Wan-Teh Chang]

Comment on attachment 413480 [details] [diff] [review]
Set NSS error to SEC_ERROR_UNKNOWN_ISSUER if PKIX chain building fails

This patch is wrong.  I now know the right way to fix the error reporting
bug.  (The last component of the relevant entry in
mozilla/security/nss/lib/libpkix/include/pkix_errorstrings.h should be an
appropriate NSS error code instead of 0.)

Comment 9

[Alexei Volkov]

Comment on attachment 476454 [details] [diff] [review]
Ignore unknown AIA location types (checked in)

This patch patch is correct in the sense that allows to continue through the list of AIAs sorting out the ones that libpkix does not understand. But to make patch complete you would need to discard failures that can occur when libpkix fetches certs through GetLDAPCerts or GetHTTPCerts functions. That would be something similar that you are doing in patch in attachment 413484 [details] [diff] [review], but only make sure that pkixError used in macro PKIX_CHECK_GOTO is decrefed.

Comment 10

[Wan-Teh Chang]

Comment on attachment 476454 [details] [diff] [review]
Ignore unknown AIA location types (checked in)

Checked in on the NSS trunk (NSS 3.13) and
NSS_3_12_BRANCH (NSS 3.12.9).

Checking in libpkix/include/pkix_errorstrings.h;
/cvsroot/mozilla/security/nss/lib/libpkix/include/pkix_errorstrings.h,v  <--  pkix_errorstrings.h
new revision: 1.37; previous revision: 1.36
done
Checking in libpkix/pkix_pl_nss/module/pkix_pl_aiamgr.c;
/cvsroot/mozilla/security/nss/lib/libpkix/pkix_pl_nss/module/pkix_pl_aiamgr.c,v  <--  pkix_pl_aiamgr.c
new revision: 1.13; previous revision: 1.12
done
Checking in util/secerr.h;
/cvsroot/mozilla/security/nss/lib/util/secerr.h,v  <--  secerr.h
new revision: 1.29; previous revision: 1.28
done

Checking in libpkix/include/pkix_errorstrings.h;
/cvsroot/mozilla/security/nss/lib/libpkix/include/pkix_errorstrings.h,v  <--  pkix_errorstrings.h
new revision: 1.35.2.2; previous revision: 1.35.2.1
done
Checking in libpkix/pkix_pl_nss/module/pkix_pl_aiamgr.c;
/cvsroot/mozilla/security/nss/lib/libpkix/pkix_pl_nss/module/pkix_pl_aiamgr.c,v  <--  pkix_pl_aiamgr.c
new revision: 1.12.2.1; previous revision: 1.12
done
Checking in util/secerr.h;
/cvsroot/mozilla/security/nss/lib/util/secerr.h,v  <--  secerr.h
new revision: 1.26.2.2; previous revision: 1.26.2.1
done

Comment 11

[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 12

[Marco Castelluccio [:marco]]

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