390209 - pkix AIA manager tries to get certs using AIA url with OCSP access method

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Alexei Volkov]

If libpkix can not find CA certs in the local cert store, it tries to obtain them online if AIA extension is listed on the subject cert.

Current algorithm does not check AIA access method. Thus, if AIA location with
OCSP access method is listed before AIA with caIssuers method, then libpkix takes this AIA in order to get CA certs.

Here is how the output of the vfyserv looks when it fails to validate a cert because of such error:
    volkov@goa1$ vfyserv -d db -c rvdealer.bankofamerica.com
Connecting to host rvdealer.bankofamerica.com (addr 171.161.161.148) on port 443
Cert file cert.000 was created.
1185832050834019:
EB68:  47 45 54 20 2F 20 48 54   54 50 2F 31 2E 31 0D 0A  GET / HTTP/1.1..
EB78:  48 6F 73 74 3A 20 6F 63   73 70 2E 76 65 72 69 73  Host: ocsp.veris
EB88:  69 67 6E 2E 63 6F 6D 3A   38 30 0D 0A 0D 0A        ign.com:80....
1185832050841520:
0FC0:  48 54 54 50 2F 31 2E 30   20 32 30 30 20 4F 6B 0D  HTTP/1.0 200 Ok.
0FD0:  0A 63 6F 6E 74 65 6E 74   2D 74 79 70 65 3A 20 61  .content-type: a
0FE0:  70 70 6C 69 63 61 74 69   6F 6E 2F 6F 63 73 70 2D  pplication/ocsp-
0FF0:  72 65 73 70 6F 6E 73 65   0D 0A 63 6F 6E 74 65 6E  response..conten
1000:  74 2D 6C 65 6E 67 74 68   3A 20 35 0D 0A 63 6F 6E  t-length: 5..con
1010:  74 65 6E 74 2D 74 72 61   6E 73 66 65 72 2D 65 6E  tent-transfer-en
1020:  63 6F 64 69 6E 67 3A 20   62 69 6E 61 72 79 0D 0A  coding: binary..
1030:  63 6F 6E 6E 65 63 74 69   6F 6E 3A 20 63 6C 6F 73  connection: clos
1040:  65 0D 0A 0D 0A 30 03 0A   01 01                    e....0....
VALIDATION ERRORS:
*** FATAL Error- PKIX ALLOC ERROR

GET BUILD RES ERRORS:
*** FATAL Error- PKIX_PL_Object_DecRef: Attempt to DecRef Alloc Error
PROBLEM WITH THE CERT CHAIN:
CERT 0. CN=rvdealer.bankofamerica.com,OU=ETIS,O=Bank of America Corporation,L=Charlotte,ST=North Carolina,C=US :
  ERROR -8179: Peer's Certificate issuer is not recognized.
    CN=VeriSign Class 3 Secure Server CA,OU=Terms of use at https://www.verisign.com/rpa (c)05,OU=VeriSign Trust Network,O="VeriSign, Inc.",C=US
Error in function PR_Write: -8010
 - Attempt to DecRef Alloc Error

Comment 1

[Alexei Volkov]

Attachment: allow only AIA with caIssuers and SIA caRepository methods to be used when retrieving certs

rvdealer.bankofamerica.com certs points to CA site that provides intermediate CA in cert der format instead of pkcs7.

This patch allows libpkix to try to decode incoming buffer even if context-type was not pkcs7. If context-type was anything else but pkcs7, then try to decode the context as a simple cert DER.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

I hope we're not reimplementing the wheel here.
NSS already has a function that attempts to decode input as any of:
- binary DER cert
- binary DER PKCS#7 cert package
- Base64 DER cert
- Base64 DER PKCS#7 cert package. 

See CERT_DecodeCertPackage and CERT_DecodeCertFromPackage

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 274556 [details] [diff] [review]
allow only AIA with caIssuers and SIA caRepository methods to be used when retrieving certs

I'm asking Julien to review this.  
I'm sure he can do so in MUCH less time than I can.  
I have a lot to learn about libPKIX before I can efficiently 
and quickly review patches like this.

Comment 4

[Alexei Volkov]

yes, the function pkix_pl_HttpCertStore_DecodeCertPackage is identical to CERT_DecodeCertPackage except few changes of setting up pkix error code and removing support for handling SEC_OID_NS_TYPE_CERT_SEQUENCE and BASE64 cert encoding. The functions also have the signature with the exception of one extra argument in pkix_pl_HttpCertStore_DecodeCertPackage: plContext that has not been used.

The file also has duplication of function that are used for pkcs7 envelope decoding.

I'll clean up the code and resubmit the patch. Julien, please review changes related only to PKIX_PL_AIAMgr_GetAIACerts function.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Alexei,  You just told me about a big source of bloat in libPKIX:  
Duplication of lots of code that is also in libsmime.  
Let's get rid of all the duplication.  Of course, we don't want to make 
libnss dependent on libSMIME.  But duplication is not the answer.

I see no reason to abandon base64 encoding, or the old Netscape cert sequence.
(Is it an OID?  what code uses an OID for those sequences?)

Comment 6

[Alexei Volkov]

By code clean up I meant to get rid of duplications. I also see not reason to abandon code that handles these two decodings.

SEC_OID_NS_TYPE_CERT_SEQUENCE is the OID.  CERT_DecodeCertPackage is the only place there nss uses this OID. SEC_ReadCertSequence function (http://lxr.mozilla.org/security/source/security/nss/lib/pkcs7/certread.c#97) is used to decode such sequence.

Comment 7

[Alexei Volkov]

Attachment: Additional cleanup of the pkix_pl_httpcertstore.c code

Code that handles decoding of pkcs7 envelope is removed. Instead, it's copy from nss/lib/pkcs7 is used. Added pkcs7 code to be linked into nss .so library since the code was not part of the library. Also the order of static linking is changed in platlib.mk.

Comment 8

[Alexei Volkov]

(In reply to comment #7)
> Created an attachment (id=274808) [details]
> Additional cleanup of the pkix_pl_httpcertstore.c code
> 
> Code that handles decoding of pkcs7 envelope is removed. Instead, it's copy
> from nss/lib/pkcs7 is used. Added pkcs7 code to be linked into nss .so library
> since the code was not part of the library. Also the order of static linking is
> changed in platlib.mk.
> 

Additional change is needed for nss/lib/manifest.mn to compile pkcs7 sources before we link libnss3.so.

RCS file: /cvsroot/mozilla/security/nss/lib/manifest.mn,v
--- lib/manifest.mn     25 May 2007 07:28:31 -0000      1.17
+++ lib/manifest.mn     1 Aug 2007 21:32:06 -0000
 DIRS =  util freebl softoken \
        base asn1 dev pki pki1 \
        libpkix \
-       certdb certhigh pk11wrap cryptohi nss \
+       certdb certhigh pk11wrap cryptohi pkcs7 nss \
        ssl \
-       pkcs12 pkcs7 smime \
+       pkcs12 smime \

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 274808 [details] [diff] [review]
Additional cleanup of the pkix_pl_httpcertstore.c code

Linking a second copy of the old PKCS7 library into libNSS is BLOAT,
and bloat is perceived to be a big problem for NSS right now, so it
would be better to not link two copies of the old PKCS7 library into 
NSS.  

IINM, SHARED_LIBRARY_LIBS is for the names of other *shared* libraries 
that are to be linked into this shared library being built.  So I 
don't think it's the right place to put pkcs7, which is not a shared
lib.

Comment 10

[Julien Pierre]

Nelson,

Linking the same copy twice but having only one copy of the source to maintain is still progress. I agree with you it would be better to have only one copy of pkcs7. However, that directory is currently exported from libsmime3. We can't have libnss3 depend on libsmime3 because that would introduce a circular dependency.
Maybe we could move the pkcs7 exports to libnss3 and have smime3 be wrappers calling into them ... Any other ideas ?

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Julien, I agree with your analysis in comment 10, except that I think two 
copies of the object (binary) code will be unacceptable to Mozilla, and
SHOULD be unacceptable to us.  

At the moment, I see only these options:

a) move the code to libNSS3.  Whether this necessitates wrappers, or not, 
seems like it should have the same answer as the used for the functions 
moved from libNSS to libUTIL.

b) move the code to a new pkcs7 shared lib that both libNSS3 and libSMIME 
link with.  The necessity of wrappers should be the same for either option.

I also observe that Christian Kaiser's new NSSCMS_ API was originally 
intended to REPLACE the older pkcs7 API.  The older PKCS7 API really 
SHOULD be deprecated, IMO, since we have the new NSSCMS code in and 
working now.  We'd reduce bloat quite a bit if we finally got rid of the 
PKCS7 functions, and made all the code that uses it switch to using the 
NSSCMS_ functions instead.  

It seems to me that the proposed move of the old PKCS7 code from libSMIME 
to another library further entrenches the libPKCS7 code, and is 
inconsistent with that code being deprecated.  :(

Comment 12

[Julien Pierre]

Nelson,

I think we have already determined that we met Mozilla's size goals through other means. This should be a non-issue now. And I don't think it  should be unacceptable to us - certainly it is OK as far as I am concerned as long as there is only one source.

The SEC_PKCS7 functions are still public and exported from libsmime3.so . So I think we have to keep at least one copy of that objects somewhere.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

How much space does the PKCS7 code occupy?  
We met the size goals before doubling that amount, whatever it is.

Also, if you think we've met the bloat goal, then please r+ the patch for it
attached to bug 389343.

Comment 14

[Julien Pierre]

Nelson,

You are right. I forgot that only part of the pkcs7 code had been copied and pasted and not all of it. So, linking all the objects twice would result in an increase. I don't have an optimized tree right now, so I don't know how much that increase will be. I will look into it. One solution would be to move all the pkcs7 functions that PKIX needs into a separate C source file, and then either link that object file twice, or #include it from a libpkix/pkix_pl_nss source.

Re: bug 389343, I will give you r+ when I have actually reviewed the patch itself; at this time, I only evaluated the size output last week but not correctness.

Comment 15

[Julien Pierre]

Comment on attachment 274808 [details] [diff] [review]
Additional cleanup of the pkix_pl_httpcertstore.c code

There are two problems with this.

1) build breakage
mozilla/security/nss/lib/manifest.mn needs to be updated to build pkcs7 before nss

2) the bloat issue that was raised by Nelson.

Unfortunately, by linking the entire pkcs7 library twice, libnss3.so actually grows .

This is the stripped size before the patch 1201396.

After the patch, it is 1232944 .

Since unfortunately we are concerned about code size, I have to give this r- .

To avoid a size increase, I suggest moving all the PKCS#7 code needed for pkix into its own source file in lib/pkcs7, then linking that object file only into libnss3.so to satisfy libpkix dependencies .

Comment 16

[Alexei Volkov]

filed two additional bugs related to the issues: bug 395093, bug 395090

Comment 17

[Alexei Volkov]

Attachment: do not use AIA OCSP access method for cert fetching

bug now is split into three parts. This bug is related to part #1: Current algorithm does not check AIA access method. Thus, if AIA location with
OCSP access method is listed before AIA with caIssuers method, then libpkix
takes this OCSP access method url CA certs fetching.

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 279813 [details] [diff] [review]
do not use AIA OCSP access method for cert fetching

One minor requested change.  Please declare automatic variables to 
minimize their scope, to declare them in their locality of reference.
Move the declaration of "method", from here:

> {
>         PKIX_UInt32 numAias = 0;
>         PKIX_UInt32 aiaIndex = 0;
>+        PKIX_UInt32 method = 0;
>         PKIX_UInt32 iaType = PKIX_INFOACCESS_LOCATION_UNKNOWN;


>@@ -613,20 +614,30 @@ PKIX_PL_AIAMgr_GetAIACerts(
>                 aiaIndex < aiaMgr->numAias;
>                 aiaIndex ++) {

Down to here.

>                 PKIX_CHECK(PKIX_List_GetItem
>                         (aiaMgr->aia,
>                         aiaIndex,
>                         (PKIX_PL_Object **)&ia,
>                         plContext),
>                         PKIX_LISTGETITEMFAILED);
> 
>+                PKIX_CHECK(PKIX_PL_InfoAccess_GetMethod
>+                        (ia, &method, plContext),
>+                        PKIX_INFOACCESSGETMETHODFAILED);
>+
>+                if (method != PKIX_INFOACCESS_CA_ISSUERS &&
>+                    method != PKIX_INFOACCESS_CA_REPOSITORY) {
>+                    PKIX_DECREF(ia);
>+                    continue;
>+                }

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 279813 [details] [diff] [review]
do not use AIA OCSP access method for cert fetching

r=nelson

Comment 20

[Alexei Volkov]

/cvsroot/mozilla/security/nss/lib/libpkix/include/pkix_errorstrings.h,v  <--  pkix_errorstrings.h
new revision: 1.10; previous revision: 1.9
/cvsroot/mozilla/security/nss/lib/libpkix/pkix_pl_nss/module/pkix_pl_aiamgr.c,v  <--  pkix_pl_aiamgr.c
new revision: 1.5; previous revision: 1.4