pam_pkcs11 -- Pull requests and advice for another bug fix

Logan Garbarini <[email protected]> Mon, 3 Aug 2020 00:28:17 -0700
Newsgroups gmane.comp.encryption.opensc.devel
Message-ID <CANVcHvPuiqttppTBCygwd7R6_u7Y4rVfQDm2j7OcwxP5WeW7jw@mail.gmail.com> 
Hi all,

I've been starting to use OpenSC and pam_pkcs11 personally and
professionally and have discovered a couple issues in `pam_pkcs11`. It
looks like the mailing list is the proper avenue for raising some of these
issues so here I go.

1) I've put up a pull request for one bug that has been verified by another
user here, this seems to be primarily an issue with OpenSSL versions and
how they are handled:

https://github.com/OpenSC/pam_pkcs11/pull/45

2) I've found what appears to be a much trickier issue in the ECDSA
signature verification of cert_vfy.c (
https://github.com/OpenSC/pam_pkcs11/issues/44). I'll reproduce the salient
parts of the issue below:

When using signature verification in pam_pkcs11, I immediately run into the
> following verification issue on ECDSA certs/keys in verify_signature():
>
verify_signature() failed: EVP_VerifyFinal() failed:
error:25066067:DSO support routines:dlfcn_load
>
> The primary issues seems to be "Signed big-endian encoding of minimal
> length", from the stackexchange post How can I convert a DER ECDSA
> signature to ASN.1?
> <https://crypto.stackexchange.com/questions/1795/how-can-i-convert-a-der-ecdsa-signature-to-asn-1/1797>.
> The current upstream code doesn't seem to even yield a properly terminated
> ASN1 signatures on modern OpenSSL versions (1.1.0) and even if it did lacks
> handling of BIGNUMs that may be interpreted as signed integers if not
> properly padded (see above stack exchange).
>

I have attached a hacky demo patch file that along with the linked stack
exchange posts clarifies my issue and what appears to be the necessary
solution.

This leads me into my primary question, as someone who hasn't had too much
exposure to OpenSC and is certainly not an expert in the OpenSSL API, what
is the best way to contribute?

It seems like EVP_Verify is a much older API and EVP_DigestVerify is now
recommended, has better documentation, and has better support for ECDSA
signatures (https://wiki.openssl.org/index.php/EVP_Signing_and_Verifying).
However, this would be a much bigger change than trying to reassemble the
ASN1 encoding using BN_bin2bn and ECDSA_SIG_get0_*.

Best,
Logan

_______________________________________________
Opensc-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/opensc-devel
0001-hack-to-test-ECDSA-sigs.patch.txt (text/plain, 2.4 KB) 
From 3557ce935dc78db625e1d74716c4520883de3c61 Mon Sep 17 00:00:00 2001
From: Logan Garbarini <[email protected]>
Date: Sun, 26 Jul 2020 01:53:39 -0700
Subject: [PATCH] hack to test ECDSA sigs

---
 src/common/cert_vfy.c | 59 ++++++++++++++++++++++++++++++++++++-------
 1 file changed, 50 insertions(+), 9 deletions(-)

diff --git a/src/common/cert_vfy.c b/src/common/cert_vfy.c
index ffdb76e..d90e77f 100644
--- a/src/common/cert_vfy.c
+++ b/src/common/cert_vfy.c
@@ -520,15 +520,56 @@ int verify_signature(X509 * x509, unsigned char *data, int data_length,
 
   if (EVP_PKEY_base_id(pubkey) == EVP_PKEY_EC) {
     rs_len = *signature_length / 2;
-    ec_sig = ECDSA_SIG_new();
-    BN_bin2bn(*signature, rs_len, ECDSA_SIG_get0_r(ec_sig));
-    BN_bin2bn(*signature + rs_len, rs_len, ECDSA_SIG_get0_s(ec_sig));
-    *signature_length = i2d_ECDSA_SIG(ec_sig, &p);
-    free(*signature);
-    *signature = malloc(*signature_length);
-    p = *signature;
-    *signature_length = i2d_ECDSA_SIG(ec_sig, &p);
-    ECDSA_SIG_free(ec_sig);
+    unsigned char* old = *signature;
+
+    int extend_r = 0;
+    int extend_s = 0;
+
+    // first byte of (vr) would be read as negative number, remember to pad
+    if ((unsigned char)*old > 127) {
+      extend_r = 1;
+    }
+    // first byte of (vs) would be read as negative number, remember to pad
+    if (((unsigned char)*(old + rs_len)) > 127) {
+      extend_s = 1;
+    }
+
+    // new signature length is 0x30 b1 0x02 b2 (vr) 0x02 b3 (vs), plus padding 
+    *signature_length = (rs_len * 2) + 6 + extend_r + extend_s;
+
+    // use calloc to handle 0 padding
+    *signature = calloc(*signature_length, sizeof(char*));
+    void * ptr = *signature;
+    DBG1("length is: %d",(*signature_length));
+    memset(ptr, 0x30, 1);
+    
+    // single byte length of all fields after this one
+    ptr += 1;
+    memset(ptr, *signature_length - 2, 1);
+
+    // marker
+    ptr += 1;
+    memset(ptr, 0x02, 1);
+
+    // length of (vr), include padding (if required)
+    ptr += 1;
+    memset(ptr, rs_len + extend_r, 1);
+
+    // vr (padding handled)
+    ptr += 1 + extend_r;
+    memcpy(ptr, old, rs_len);
+    
+    // marker
+    ptr += rs_len;
+    memset(ptr, 0x02, 1);
+    
+    // length of (vs), include padding (if required)
+    ptr += 1;
+    memset(ptr, rs_len + extend_s, 1);
+    
+    // vs (padding handled)
+    ptr += 1 + extend_s;
+    memcpy(ptr, old+rs_len, rs_len);
   }
 
   md_ctx = EVP_MD_CTX_new();
-- 
2.17.1