Skip to content

Commit

Permalink
Workaround for incorrect CKA_EC_POINT format
Browse files Browse the repository at this point in the history 
Workaround for broken PKCS#11 modules not returning CKA_EC_POINT
in the ASN1_OCTET_STRING format.  Closes #79.
  • Loading branch information
@mtrojnar
mtrojnar committed Apr 20, 2016
1 parent 491c6b1 commit 874154b
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 2 deletions.
3 changes: 3 additions & 0 deletions NEWS
View file
@@ -1,6 +1,9 @@
NEWS for Libp11 -- History of user visible changes

New in 0.4.1; unreleased
* Workaround for broken PKCS#11 modules not returning CKA_EC_POINT
in the ASN1_OCTET_STRING format (Michał Trojnara)
* Improved building against OpenSSL 1.1.0-dev (Michał Trojnara)

New in 0.4.0; 2016-03-28; Michał Trojnara
* Merged engine_pkcs11 (Michał Trojnara)
Expand Down
9 changes: 7 additions & 2 deletions src/p11_ec.c
View file
Expand Up @@ -119,15 +119,20 @@ static EC_KEY *pkcs11_get_ec(PKCS11_KEY *key)
if (!key_getattr_alloc(pubkey, CKA_EC_POINT, &point, &point_len)) {
const unsigned char *a;
ASN1_OCTET_STRING *os;
EC_KEY *success = NULL;

/* PKCS#11 returns ASN1_OCTET_STRING */
/* PKCS#11-compliant modules should return ASN1_OCTET_STRING */
a = point;
os = d2i_ASN1_OCTET_STRING(NULL, &a, (long)point_len);
if (os) {
a = os->data;
o2i_ECPublicKey(&ec, &a, os->length);
success = o2i_ECPublicKey(&ec, &a, os->length);
ASN1_STRING_free(os);
}
if (success == NULL) { /* Workaround for broken PKCS#11 modules */
a = point;
o2i_ECPublicKey(&ec, &a, point_len);
}
OPENSSL_free(point);
}

Expand Down

10 comments on commit 874154b

@DDvO
Copy link

@DDvO DDvO commented on 874154b Apr 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In answer to your comment:

Currently, pkcs11_get_ec() does best-effort retrieval and it does not fail if it was unable to retrieve some data. Maybe we should enforce a more strict error checking. I appreciate your comments on this topic.

Pleased to hear.

How about returning NULL if pubkeyis non-NULL but the point data could not be read and parsed successfully? In any such cases I suggest:

{
              EC_KEY_free(ec);
              ec = NULL;
}

Otherwise, when an EC public key is present and gets referenced in an application, this will crash because the assumed point data is not present.

@mtrojnar
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will crash because the assumed point data is not present.

Were you able to demonstrate the crash? Were exactly does it crash?

@DDvO
Copy link

@DDvO DDvO commented on 874154b Apr 21, 2016 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An EC public key without a point would be pointless ;-)
Indeed, I just reproduced this for two cases:

When registering the public key for TLS client authentication:

Thread [1] 0 (Suspended : Signal : SIGSEGV:Segmentation fault)  
cygcrypto-1.0.0!EC_POINT_cmp() at 0x689521fc    
ec_GF2m_have_precompute_mult() at 0x68961042    
cygcrypto-1.0.0!X509_check_private_key() at 0x689ec24c  
cygssl-1.0.0!SSL_CTX_use_PrivateKey() at 0x58d1876b 
est_client_set_cert_and_key() at est_client.c:54 0x4058da   
est_client_set_auth() at est_client.c:3.678 0x40c1fc    
do_operation() at estclient-engine.c:677 0x402011   
main() at estclient-engine.c:1.234 0x403436

When inserting the public key in a CSR:

Thread [1] 0 (Suspended : Signal : SIGSEGV:Segmentation fault)  
cygcrypto-1.0.0!EC_POINT_point2oct() at 0x689659f5  
i2o_ECPublicKey() at 0x6895bf0a 
ec_GF2m_have_precompute_mult() at 0x6896171c    
cygcrypto-1.0.0!X509_PUBKEY_set() at 0x689c0a44 
populate_x509_request() at est_client.c:156 0x405bdf    
est_generate_pkcs10() at est_client.c:191 0x405cc8  
est_client_enroll_cn() at est_client.c:1.869 0x4093c8   
est_client_enroll() at est_client.c:2.848 0x40ad8c  
simple_enroll_attempt() at estclient-engine.c:341 0x40176f  
do_operation() at estclient-engine.c:768 0x4022be   
<...more frames...>

@DDvO
Copy link

@DDvO DDvO commented on 874154b Apr 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thus propose the following revision.

    if (!key_getattr_alloc(pubkey, CKA_EC_POINT, &point, &point_len)) {
        const unsigned char *a;
        ASN1_OCTET_STRING *os;
        EC_KEY *success = NULL;

        /* PKCS#11-compliant modules should return ASN1_OCTET_STRING */
        a = point;
        os = d2i_ASN1_OCTET_STRING(NULL, &a, (long)point_len);
        if (os) {
            a = os->data;
            success = o2i_ECPublicKey(&ec, &a, os->length);
            ASN1_STRING_free(os);
        }
        if (!success) { /* Workaround for broken PKCS#11 modules */
            a = point;
            success = o2i_ECPublicKey(&ec, &a, point_len);
        }
        OPENSSL_free(point);
        if (!success) {
            EC_KEY_free(ec);
            ec = NULL;
        }
    } else {
        EC_KEY_free(ec);
        ec = NULL;
    }

@mtrojnar
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think 80c5a94 implements the proper solution: it always attempts to retrieve the params and the point, but it only requires them for public keys.

@DDvO
Copy link

@DDvO DDvO commented on 874154b Apr 23, 2016 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice that the new code also lincludes strictness on the EC params of public keys.

For curiosity, I tested the above mentioned use cases on EC pubkeys having a point but no params:
The first one also crashes, while the latter returns the following neat OpenSSL error:

2675996:error:100DF07C:elliptic curve routines:ECKEY_PARAM2TYPE:missing parameters:ec_ameth.c:78:
2675996:error:100D8010:elliptic curve routines:ECKEY_PUB_ENCODE:EC lib:ec_ameth.c:114:
2675996:error:0B07807E:x509 certificate routines:X509_PUBKEY_set:public key encode error:x_pubkey.c:103:

I just came across a related problem where points and params are expected also for EC private keys, but that's worth a new issue. (Update: see #83)

@dengert
Copy link
Member

@dengert dengert commented on 874154b Apr 23, 2016 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DDvO
Copy link

@DDvO DDvO commented on 874154b Apr 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dengert, as mentioned in my other recent posts, I use the CardOS module (cardos11.dll) version 5.3 (build 7 and 8) which is crucial for accessing CardOS 5.0 cards. Yet what we've been discussing in this thread is independent of the module and card, and therefore I see no need to include any lower-level debug trace here.

The issue here was which information (namely, the EC parameters and the curve point) an application can expect to be present in the EC variant of the EVP_PKEY data structure when it asked engine_pkcs11.dll (und thus libp11.dll) for an EC public key, and thus whether libp11 should flag the non-existence of such data items as a error to avoid application crashes like the ones I provided above when simulating (regardless of the module and card) their non-existence. This error-handling issue has been solved with the last commit mentioned above.

@dengert
Copy link
Member

@dengert dengert commented on 874154b Apr 25, 2016 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DDvO
Copy link

@DDvO DDvO commented on 874154b Apr 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dengert, your last two responses must be due to a misunderstanding. This thread (unlike its parent #79) is not about observed misbehavior of the CardOS module, but about strictness of libp11 in case any module did not return useful EC parameters or points.

In order to demonstrate to @mtrojnar that missing error checking can lead to application crashes, as mentioned I simulated that such data was missing from the module. Thus I cannot provide spy output showing this. Indeed, the CardOS module does give correct EC parameter responses for public key objects.

Please sign in to comment.