Re: nss -- xmlSecNssX509StoreVerify question

Hi,

On Thu, Dec 08, 2016 at 02:53:37PM -0800, Aleksey Sanin <[email protected]> wrote:
> Can you check the xmlSecDsigCtx to see if the key used for verification
> (it is returned in the context) has a certificate attached to it?

I'm not sure how to check that. After xmlSecDSigCtxVerify() returns, I
see this:

(gdb) print *pDsigCtx->signKey
$12 = {name = 0x0, value = 0x1fb85d0, dataList = 0x200d780, usage = 4294967295, notValidBefore = 1450425499, notValidAfter = 1482825499}

How do I decide if it has a certificate or not?

In the meantime I tried to find out how the code path differs in case
the root CA / interim CA certificates are / are not available in the NSS
store.  (My understanding is that the intention of the
XMLSEC_KEYINFO_FLAGS_X509DATA_DONT_VERIFY_CERTS flag is that in case
it's set, then xmlSecDSigCtxVerify() shouldn't fail when CA certificates
are not available, otherwise it should.)

Here is what I found: in xmlSecNssX509StoreVerify(), we call
CERT_VerifyCertificate() from NSS. This function does actual
verification only in case either a single certificate usage is
specified, or a usage pointer is passed to it. In the later case the
usage will contain the usages which could be validated.

This means that by changing xmlsec-nss' CERT_VerifyCertificate()
invocation to pass in a usage pointer makes NSS actually attempt to
validate something, which is all usages by default. When the CA
certificates are not present, it still returns success, but in my case
the usage bitfield is 0xb80 in the bad case (CA certs are missing) and
0xbb1 in the good case (CA certs are present). The values are OR'ed
together from certificateUsage* defines from nss/lib/certdb/certt.h.

Based on this, I guess one approach would be to not only check the
return vaulue of CERT_VerifyCertificate(), but also assert that the
usage bitfield contains one of these usage defines, which are not always
returned.

I've implemented this idea in the attached patch. I'm not exactly sure
if this whitelist is a correct one, but it works for my test certificate
at least, i.e. now xmlSecDSigCtxVerify() returns an error when the CA
certs are not present.

This is all promising, except that lots of tests in 'make check' now
fail, like this:

----
aleksey-xmldsig-01/enveloping-sha256-rsa-sha256-relationship
    Checking required transforms                            OK
    Checking required key data                              OK
    Verify existing signature                             Fail
    Create new signature                                    OK
    Verify new signature                                  Fail
----

Now I'm not sure: is it possible that at least some of the tests depend
on this misfeature that signature verification works even if the
certificate can't be verified? Or did my patch break something? In the
above case I verified that the returned usage bitfield is 0xb80, i.e. it
returns exactly the usages which I intentionally did not consider in my
patch. So either the test is not correct (bug I guess it is), or I don't
have an idea right now how to detect the difference between the test and
when my manual testing hits this "digest matches but the certificate
can't be verified" case.

One more info: I've checked the mscrypto backend, and there
xmlSecDSigCtxVerify() properly fails when the CA certs are missing, so I
think the root cause is in the NSS backend (assuming NSS itself is not
buggy, but that's probably the case).

Sorry for the long mail, I'm just dumping all the details, in case it
helps. :-)

Thanks,

Miklos

_______________________________________________
xmlsec mailing list
[email protected]
http://www.aleksey.com/mailman/listinfo/xmlsec

diff --git a/src/nss/x509vfy.c b/src/nss/x509vfy.c
index c65ae68..18bef33 100644
--- a/src/nss/x509vfy.c
+++ b/src/nss/x509vfy.c
@@ -81,6 +81,8 @@ static xmlSecByte *     xmlSecNssX509NameRead           (xmlSecByte *str,
 
 static int              xmlSecNssNumToItem              (SECItem *it, 
                                                          PRUint64 num);
+static int              xmlSecNssX509VerifyStatusAndUsage(SECStatus status,
+                                                          SECCertificateUsage usage);
 
 
 static xmlSecKeyDataStoreKlass xmlSecNssX509StoreKlass = {
@@ -169,6 +171,7 @@ xmlSecNssX509StoreVerify(xmlSecKeyDataStorePtr store, CERTCertList* certs,
     int64 timeboundary;
     int64 tmp1, tmp2;
     PRErrorCode err;
+    SECCertificateUsage usage;
 
     xmlSecAssert2(xmlSecKeyDataStoreCheckId(store, xmlSecNssX509StoreId), NULL);
     xmlSecAssert2(certs != NULL, NULL);
@@ -215,13 +218,13 @@ xmlSecNssX509StoreVerify(xmlSecKeyDataStorePtr store, CERTCertList* certs,
         status = CERT_VerifyCertificate(CERT_GetDefaultCertDB(),
                                         cert, PR_FALSE,
                                         (SECCertificateUsage)0,
-                                        timeboundary , NULL, NULL, NULL);
-        if (status == SECSuccess) {
+                                        timeboundary , NULL, NULL, &usage);
+        if (xmlSecNssX509VerifyStatusAndUsage(status, usage)) {
             break;
         }
     }
 
-    if (status == SECSuccess) {
+    if (xmlSecNssX509VerifyStatusAndUsage(status, usage)) {
         return (cert);
     }
 
@@ -680,6 +683,23 @@ xmlSecNssX509NameStringRead(xmlSecByte **str, int *strLen,
     return((ingoreTrailingSpaces) ? nonSpace - res + 1 : q - res);
 }
 
+static int
+xmlSecNssX509VerifyStatusAndUsage(SECStatus status, SECCertificateUsage usage) {
+    /* require that one of these are present */
+    SECCertificateUsage requiredUsages = certificateUsageSSLClient |
+        certificateUsageEmailSigner | certificateUsageEmailRecipient;
+
+    if(status != SECSuccess) {
+        return(0);
+    }
+
+    if((usage & requiredUsages) == 0) {
+        return(0);
+    }
+
+    return(1);
+}
+
 /* code lifted from NSS */
 static int
 xmlSecNssNumToItem(SECItem *it, PRUint64 ui)

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iEYEARECAAYFAlhKgogACgkQe81tAgORUJZ81ACfQR/ae7IJQRY2VAtO/sI2xkWg
N0EAnAr0vMTK+opEQcH8EaFp5uXAZ4Bx
=tc01
-----END PGP SIGNATURE-----