x509_crt::key_usage is too small

[NWilson]

It's an unsigned char, but the X.509 standard defines 9 flags! PolarSSL just doesn't support the "encipher only" and "decipher only" flags.

It's a minor niggle.

Patch below; I'll turn it into a PR if you'd prefer, but I don't which branch to base ABI-breaking changes against (1.4 or development?)

--- a/include/polarssl/x509.h
+++ b/include/polarssl/x509.h
@@ -110,6 +110,8 @@

 #define KU_KEY_AGREEMENT                (0x08)  /* bit 4 */
 #define KU_KEY_CERT_SIGN                (0x04)  /* bit 5 */
 #define KU_CRL_SIGN                     (0x02)  /* bit 6 */
+#define KU_ENCIPHER_ONLY                (0x01)  /* bit 7 */
+#define KU_DECIPHER_ONLY              (0x8000)  /* bit 8 */

 /*
  * Netscape certificate types
--- a/include/polarssl/x509_crt.h
+++ b/include/polarssl/x509_crt.h
@@ -79,7 +79,7 @@ typedef struct _x509_crt
     int ca_istrue;              /**< Optional Basic Constraint extension value: 1 if this cert
     int max_pathlen;            /**< Optional Basic Constraint extension value: The maximum pa

-    unsigned char key_usage;    /**< Optional key usage extension value: See the values in x50
+    unsigned int key_usage;     /**< Optional key usage extension value: See the values in x50

     x509_sequence ext_key_usage; /**< Optional list of extended key usage OIDs. */

--- a/library/x509_crt.c
+++ b/library/x509_crt.c
@@ -247,9 +247,10 @@ static int x509_get_ns_cert_type( unsigned char **p,

 static int x509_get_key_usage( unsigned char **p,
                                const unsigned char *end,
-                               unsigned char *key_usage)
+                               unsigned int *key_usage)
 {
     int ret;
+    size_t i;
     x509_bitstring bs = { 0, 0, NULL };

     if( ( ret = asn1_get_bitstring( p, end, &bs ) ) != 0 )
@@ -260,7 +261,11 @@ static int x509_get_key_usage( unsigned char **p,
                 POLARSSL_ERR_ASN1_INVALID_LENGTH );

     /* Get actual bitstring */
-    *key_usage = *bs.p;
+    *key_usage = 0;
+    for( i = 0; i < bs.len && i < sizeof(unsigned int); ++i )
+    {
+        *key_usage |= bs.p[i] << (8*i);
+    }
     return( 0 );
 }

@@ -1203,7 +1208,7 @@ static int x509_info_cert_type( char **buf, size_t *size,
         PRINT_ITEM( name );

 static int x509_info_key_usage( char **buf, size_t *size,
-                                unsigned char key_usage )
+                                unsigned int key_usage )
 {
     int ret;
     size_t n = *size;
@@ -1437,7 +1442,7 @@ int x509_crt_verify_info( char *buf, size_t size, const char *prefix,
 int x509_crt_check_key_usage( const x509_crt *crt, int usage )
 {
     if( ( crt->ext_types & EXT_KEY_USAGE ) != 0 &&
-        ( crt->key_usage & usage ) != usage )
+        ( crt->key_usage & (unsigned int)usage ) != (unsigned int)usage )
         return( POLARSSL_ERR_X509_BAD_INPUT_DATA );

     return( 0 );

[mpg]

We already fixed this issue in our internal development branch for the upcoming 2.0 branch. Thanks for your patch anyway!

[mpg]

Haha, I'm glad I double-checked, because I actually didn't get to it yet, though it had been on my list for a while so I thought it was done already. I'll review your patch more carefully in the next few days. In the meanwhile, do you have any test certs? (Edit: ah, I only did the API changes, not the actual implementation, that's it.)

[NWilson]

Yes, here's a cert with the "decipherOnly" bit set:

-----BEGIN CERTIFICATE-----
MIICFzCCAYCgAwIBAgIJAJsTzkylb95SMA0GCSqGSIb3DQEBBQUAMD8xCzAJBgNV
BAYTAkdCMRIwEAYDVQQHDAlDYW1icmlkZ2UxHDAaBgNVBAoME0RlZmF1bHQgQ29t
cGFueSBMdGQwHhcNMTUwNTEyMTAzNjU1WhcNMTgwNTExMTAzNjU1WjA/MQswCQYD
VQQGEwJHQjESMBAGA1UEBwwJQ2FtYnJpZGdlMRwwGgYDVQQKDBNEZWZhdWx0IENv
bXBhbnkgTHRkMIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQC9nxYOSbha/Ap4
6rACrOMH7zfDD+0ZEHhbO0bgGRjc5ElvOaNuD321y9TnyAx+JrqPp/lFrAgNiVo1
HPurPHfcJ+tNBUgBHboWGNENNaf9ovwFPawsBzEZraGnDaqVPEFcIsUQPVqO1lrQ
CHLUjtqo1hMZDqe/Web0Mw9cZrqOaQIDAQABoxswGTAJBgNVHRMEAjAAMAwGA1Ud
DwQFAwMH4IAwDQYJKoZIhvcNAQEFBQADgYEAJ0NS2wUbgRelK0qKxrR2Ts6jVYEH
bmykx3GHjFyKpscDIn2vNyyB7ygfFglZPcw+2mn3xuVIwOV/mWxFvKHk+j2WrTQL
tDqSC5BhFoR01veFu07JdEYvz+I+NCL5z0IGWXkUrk235Wl4w4WMZDnXTqncMNEk
fLtpo9y79XD00QY=
-----END CERTIFICATE-----

[mpg]

Ok, I'm importing this in the 2.0 branch. Some nitpicking about your patch:

    *key_usage |= bs.p[i] << (8*i);

Here bs.p[i] is promoted to int (signed), so if it has its most significant bit set and is shifted by 24, you're shifting into the sign bit, which is undefined behaviour in C. Casting to unsigned int avoids that:

    *key_usage |= (unsigned int) bs.p[i] << (8*i);

[mpg]

I also updated x509_info_key_usage() and most importantly x509_check_key_usage() this these new bits have a very different semantic that the other bits: when asserted, they restrict what may be done with the key. I'm also going to update the cert writing code and then I think I'll be done.