egg: Fix parsing of unsigned integers in DER

 * When the unsigned integer had a high bit set, we would store/parse
   it incorrectly. We have to force these numbers to be unsigned so
   we prefix/strip an extra zero byte on the front.
 * Also make accessing raw and usg numbers in DER not have to copy
   the value, since these are often sensitive.

diff --git a/egg/egg-asn1x.c b/egg/egg-asn1x.c
index db64fd7..f2d4b42 100644 (file)
--- a/egg/egg-asn1x.c
+++ b/egg/egg-asn1x.c
@@ -1492,6 +1492,33 @@ anode_encoder_simple (gpointer user_data, guchar *data, gsize n_data)
 }
 
 static gboolean
+anode_encoder_unsigned (gpointer user_data,
+                        guchar *data,
+                        gsize n_data)
+{
+       gboolean sign;
+       gchar *p;
+
+       /*
+        * If top bit is set, the result would be negative in two's complement
+        * but since we want an unsigned integer, add a zero byte. That zero
+        * byte is already calculated into n_data, see egg_asn1x_set_integer_as_usg
+        */
+
+       p = user_data;
+       sign = !!(p[0] & 0x80);
+       if (sign) {
+               g_assert (n_data > 1);
+               data[0] = 0;
+               data++;
+               n_data--;
+       }
+
+       memcpy (data, p, n_data);
+       return TRUE;
+}
+
+static gboolean
 anode_encoder_structured (gpointer user_data, guchar *data, gsize n_data)
 {
        GNode *node = user_data;
@@ -2050,6 +2077,8 @@ anode_write_integer_ulong (gulong value, guchar *data, gsize *n_data)
 {
        guchar buf[sizeof (gulong)];
        gint bytes, i, off;
+       guchar *at;
+       gboolean sign;
 
        for (i = 0; i < sizeof (gulong); ++i) {
                off = sizeof (gulong) - (i + 1);
@@ -2064,11 +2093,20 @@ anode_write_integer_ulong (gulong value, guchar *data, gsize *n_data)
        if (bytes == 0)
                bytes = 1;
 
+       /* If the first byte would make this negative, then add a zero */
+       at = buf + (sizeof (gulong) - bytes);
+       sign = !!(at[0] & 0x80);
+
        if (data) {
-               g_assert (*n_data >= bytes);
-               memcpy (data, buf + (sizeof (gulong) - bytes), bytes);
+               g_assert (*n_data >= bytes + 1);
+               if (sign) {
+                       data[0] = 0;
+                       data++;
+               }
+               memcpy (data, at, bytes);
        }
-       *n_data = bytes;
+
+       *n_data = bytes + (sign ? 1 : 0);
        return TRUE;
 }
 
@@ -2510,7 +2548,7 @@ egg_asn1x_set_enumerated (GNode *node, GQuark value)
        val = anode_def_value_as_ulong (opt);
        g_return_val_if_fail (val != G_MAXULONG, FALSE);
 
-       n_data = sizeof (gulong);
+       n_data = sizeof (gulong) + 1;
        data = g_malloc0 (n_data);
        if (!anode_write_integer_ulong (val, data, &n_data))
                return FALSE;
@@ -2569,7 +2607,7 @@ egg_asn1x_set_integer_as_ulong (GNode *node, gulong value)
 
        /* TODO: Handle default values */
 
-       n_data = sizeof (gulong);
+       n_data = sizeof (gulong) + 1;
        data = g_malloc0 (n_data);
        if (!anode_write_integer_ulong (value, data, &n_data))
                return FALSE;
@@ -2577,30 +2615,54 @@ egg_asn1x_set_integer_as_ulong (GNode *node, gulong value)
        return TRUE;
 }
 
-gpointer
-egg_asn1x_get_integer_as_raw (GNode *node, EggAllocator allocator, gsize *n_data)
+gconstpointer
+egg_asn1x_get_integer_as_raw (GNode *node, gsize *n_data)
 {
        Atlv *tlv;
-       gpointer data;
 
        g_return_val_if_fail (node, FALSE);
        g_return_val_if_fail (n_data, FALSE);
        g_return_val_if_fail (anode_def_type (node) == TYPE_INTEGER, FALSE);
 
-       if (!allocator)
-               allocator = g_realloc;
-
        tlv = anode_get_tlv_data (node);
        if (tlv == NULL || tlv->buf == NULL)
                return NULL;
 
-       data = (allocator) (NULL, tlv->len);
-       if (data == NULL)
+       *n_data = tlv->len;
+       return tlv->buf + tlv->off;
+}
+
+gconstpointer
+egg_asn1x_get_integer_as_usg (GNode *node,
+                              gsize *n_data)
+{
+       const guchar *p;
+       gboolean sign;
+       gsize len;
+
+       g_return_val_if_fail (node, FALSE);
+       g_return_val_if_fail (n_data, FALSE);
+       g_return_val_if_fail (anode_def_type (node) == TYPE_INTEGER, FALSE);
+
+       p = egg_asn1x_get_integer_as_raw (node, &len);
+       sign = !!(p[0] & 0x80);
+       if (sign) {
+               g_warning ("invalid two's complement integer is negative, but expected unsigned");
                return NULL;
+       }
 
-       memcpy (data, tlv->buf + tlv->off, tlv->len);
-       *n_data = tlv->len;
-       return data;
+       *n_data = len;
+
+       /* Strip off the extra zero byte that was preventing it from being negative */
+       if (p[0] == 0 && len > 1) {
+               sign = !!(p[1] & 0x80);
+               if (sign) {
+                       p++;
+                       *n_data = len - 1;
+               }
+       }
+
+       return p;
 }
 
 gboolean
@@ -2626,6 +2688,37 @@ egg_asn1x_set_integer_as_raw (GNode *node, gconstpointer data, gsize n_data, GDe
        return TRUE;
 }
 
+gboolean
+egg_asn1x_set_integer_as_usg (GNode *node,
+                              gconstpointer data,
+                              gsize n_data,
+                              GDestroyNotify destroy)
+{
+       gboolean sign;
+       guchar *p;
+
+       g_return_val_if_fail (node, FALSE);
+       g_return_val_if_fail (data, FALSE);
+       g_return_val_if_fail (n_data > 0, FALSE);
+       g_return_val_if_fail (anode_def_type (node) == TYPE_INTEGER, FALSE);
+
+       /* Make sure the integer is properly encoded in twos complement*/
+       p = (guchar*)data;
+       sign = !!(p[0] & 0x80);
+
+       /*
+        * If in two's complement this would be negative, add a zero byte so
+        * that it isn't. Here we just note that the result will be one byte
+        * longer. In anode_encoder_unsigned we actually add the zero byte.
+        */
+       if (sign)
+               n_data += 1;
+
+       anode_encode_tlv_and_enc (node, n_data, anode_encoder_unsigned,
+                                 (gpointer)data, destroy);
+       return TRUE;
+}
+
 gconstpointer
 egg_asn1x_get_raw_element (GNode *node, gsize *n_element)
 {

diff --git a/egg/egg-asn1x.h b/egg/egg-asn1x.h
index b6c8427..3f74655 100644 (file)
--- a/egg/egg-asn1x.h
+++ b/egg/egg-asn1x.h
@@ -102,8 +102,7 @@ gboolean            egg_asn1x_get_integer_as_ulong   (GNode *node,
 gboolean            egg_asn1x_set_integer_as_ulong   (GNode *node,
                                                       gulong value);
 
-gpointer            egg_asn1x_get_integer_as_raw     (GNode *node,
-                                                      EggAllocator allocator,
+gconstpointer       egg_asn1x_get_integer_as_raw     (GNode *node,
                                                       gsize *n_data);
 
 gboolean            egg_asn1x_set_integer_as_raw     (GNode *node,
@@ -111,6 +110,14 @@ gboolean            egg_asn1x_set_integer_as_raw     (GNode *node,
                                                       gsize n_data,
                                                       GDestroyNotify destroy);
 
+gconstpointer       egg_asn1x_get_integer_as_usg     (GNode *node,
+                                                      gsize *n_data);
+
+gboolean            egg_asn1x_set_integer_as_usg     (GNode *node,
+                                                      gconstpointer data,
+                                                      gsize n_data,
+                                                      GDestroyNotify destroy);
+
 gconstpointer       egg_asn1x_get_raw_value          (GNode *node,
                                                       gsize *n_content);
 

diff --git a/egg/tests/test-asn1.c b/egg/tests/test-asn1.c
index c155b42..9403ebb 100644 (file)
--- a/egg/tests/test-asn1.c
+++ b/egg/tests/test-asn1.c
@@ -37,6 +37,7 @@
 extern const ASN1_ARRAY_TYPE test_asn1_tab[];
 
 const gchar I33[] =           "\x02\x01\x2A";
+const gchar I253[] =           "\x02\x02\x00\xFD";
 const gchar BFALSE[] =        "\x01\x01\x00";
 const gchar BTRUE[] =         "\x01\x01\xFF";
 const gchar SFARNSWORTH[] =   "\x04\x0A""farnsworth";
@@ -147,6 +148,57 @@ test_integer (void)
 }
 
 static void
+test_unsigned (void)
+{
+       GNode *asn;
+       gulong value;
+       guchar *check;
+       gsize n_check;
+       guchar val;
+       gconstpointer usg;
+       gsize n_usg;
+
+       asn = egg_asn1x_create (test_asn1_tab, "TestInteger");
+       g_assert (asn);
+
+       /* Check with ulong */
+       if (!egg_asn1x_decode (asn, I253, XL (I253)))
+               g_assert_not_reached ();
+       if (!egg_asn1x_get_integer_as_ulong (asn, &value))
+               g_assert_not_reached ();
+       g_assert (value == 253);
+
+       egg_asn1x_clear (asn);
+
+       if (!egg_asn1x_set_integer_as_ulong (asn, 253))
+               g_assert_not_reached ();
+
+       check = egg_asn1x_encode (asn, NULL, &n_check);
+       egg_assert_cmpmem (check, n_check, ==, I253, XL (I253));
+
+       /* Now check with usg */
+       if (!egg_asn1x_decode (asn, I253, XL (I253)))
+               g_assert_not_reached ();
+       g_free (check);
+
+       val = 0xFD; /* == 253 */
+       usg = egg_asn1x_get_integer_as_usg (asn, &n_usg);
+       egg_assert_cmpmem (&val, 1, ==, usg, n_usg);
+
+       egg_asn1x_clear (asn);
+
+       if (!egg_asn1x_set_integer_as_usg (asn, &val, 1, NULL))
+               g_assert_not_reached ();
+
+       check = egg_asn1x_encode (asn, NULL, &n_check);
+       egg_assert_cmpsize (n_check, ==, XL (I253));
+       egg_assert_cmpmem (check, n_check, ==, I253, XL (I253));
+
+       egg_asn1x_destroy (asn);
+       g_free (check);
+}
+
+static void
 test_octet_string (void)
 {
        GNode *asn;
@@ -1156,6 +1208,7 @@ main (int argc, char **argv)
        g_test_add_func ("/asn1/boolean", test_boolean);
        g_test_add_func ("/asn1/null", test_null);
        g_test_add_func ("/asn1/integer", test_integer);
+       g_test_add_func ("/asn1/unsigned", test_unsigned);
        g_test_add_func ("/asn1/octet_string", test_octet_string);
        g_test_add_func ("/asn1/generalized_time", test_generalized_time);
        g_test_add_func ("/asn1/implicit", test_implicit);

diff --git a/gcr/gcr-certificate-renderer.c b/gcr/gcr-certificate-renderer.c
index 484a5e5..e6d5df7 100644 (file)
--- a/gcr/gcr-certificate-renderer.c
+++ b/gcr/gcr-certificate-renderer.c
@@ -569,6 +569,7 @@ gcr_certificate_renderer_render (GcrRenderer *renderer, GcrViewer *viewer)
        const gchar *text;
        GcrCertificate *cert;
        gpointer raw;
+       gconstpointer number;
        gulong version;
        guint bits, index;
        gchar *display;
@@ -646,10 +647,9 @@ gcr_certificate_renderer_render (GcrRenderer *renderer, GcrViewer *viewer)
        _gcr_display_view_append_value (view, renderer, _("Version"), display, FALSE);
        g_free (display);
 
-       raw = egg_asn1x_get_integer_as_raw (egg_asn1x_node (asn, "tbsCertificate", "serialNumber", NULL), NULL, &n_raw);
-       g_return_if_fail (raw);
-       _gcr_display_view_append_hex (view, renderer, _("Serial Number"), raw, n_raw);
-       g_free (raw);
+       number = egg_asn1x_get_integer_as_raw (egg_asn1x_node (asn, "tbsCertificate", "serialNumber", NULL), &n_raw);
+       g_return_if_fail (number != NULL);
+       _gcr_display_view_append_hex (view, renderer, _("Serial Number"), number, n_raw);
 
        display = g_malloc0 (128);
        if (egg_asn1x_get_time_as_date (egg_asn1x_node (asn, "tbsCertificate", "validity", "notBefore", NULL), &date)) {

diff --git a/gcr/gcr-certificate.c b/gcr/gcr-certificate.c
index a7d50ed..945d198 100644 (file)
--- a/gcr/gcr-certificate.c
+++ b/gcr/gcr-certificate.c
@@ -886,6 +886,7 @@ guchar *
 gcr_certificate_get_serial_number (GcrCertificate *self, gsize *n_length)
 {
        GcrCertificateInfo *info;
+       const guchar *serial;
 
        g_return_val_if_fail (GCR_IS_CERTIFICATE (self), NULL);
        g_return_val_if_fail (n_length != NULL, NULL);
@@ -893,7 +894,8 @@ gcr_certificate_get_serial_number (GcrCertificate *self, gsize *n_length)
        info = certificate_info_load (self);
        g_return_val_if_fail (info, NULL);
 
-       return egg_asn1x_get_integer_as_raw (egg_asn1x_node (info->asn1, "tbsCertificate", "serialNumber", NULL), NULL, n_length);
+       serial = egg_asn1x_get_integer_as_raw (egg_asn1x_node (info->asn1, "tbsCertificate", "serialNumber", NULL), n_length);
+       return g_memdup (serial, *n_length);
 }
 
 /**

diff --git a/gcr/gcr-fingerprint.c b/gcr/gcr-fingerprint.c
index 3fd16cb..37a1df6 100644 (file)
--- a/gcr/gcr-fingerprint.c
+++ b/gcr/gcr-fingerprint.c
@@ -76,12 +76,12 @@ rsa_subject_public_key_from_attributes (GckAttributes *attrs, GNode *info_asn)
 
        attr = gck_attributes_find (attrs, CKA_MODULUS);
        g_return_val_if_fail (attr, FALSE);
-       egg_asn1x_set_integer_as_raw (egg_asn1x_node (key_asn, "modulus", NULL),
+       egg_asn1x_set_integer_as_usg (egg_asn1x_node (key_asn, "modulus", NULL),
                                      attr->value, attr->length, NULL);
 
        attr = gck_attributes_find (attrs, CKA_PUBLIC_EXPONENT);
        g_return_val_if_fail (attr, FALSE);
-       egg_asn1x_set_integer_as_raw (egg_asn1x_node (key_asn, "publicExponent", NULL),
+       egg_asn1x_set_integer_as_usg (egg_asn1x_node (key_asn, "publicExponent", NULL),
                                      attr->value, attr->length, NULL);
 
        key = egg_asn1x_encode (key_asn, g_realloc, &n_key);
@@ -128,7 +128,7 @@ dsa_subject_public_key_from_private (GNode *key_asn, GckAttribute *ap,
        g_return_val_if_fail (my, FALSE);
        gcry_mpi_powm (my, mg, mx, mp);
 
-       gcry = gcry_mpi_aprint (GCRYMPI_FMT_USG, &buffer, &n_buffer, my);
+       gcry = gcry_mpi_aprint (GCRYMPI_FMT_STD, &buffer, &n_buffer, my);
        g_return_val_if_fail (gcry == 0, FALSE);
        egg_asn1x_set_integer_as_raw (key_asn, buffer, n_buffer, gcry_free);
 
@@ -163,15 +163,15 @@ dsa_subject_public_key_from_attributes (GckAttributes *attrs, GNode *info_asn)
 
        p = gck_attributes_find (attrs, CKA_PRIME);
        g_return_val_if_fail (p, FALSE);
-       egg_asn1x_set_integer_as_raw (egg_asn1x_node (params_asn, "p", NULL), p->value, p->length, NULL);
+       egg_asn1x_set_integer_as_usg (egg_asn1x_node (params_asn, "p", NULL), p->value, p->length, NULL);
 
        q = gck_attributes_find (attrs, CKA_SUBPRIME);
        g_return_val_if_fail (q, FALSE);
-       egg_asn1x_set_integer_as_raw (egg_asn1x_node (params_asn, "q", NULL), q->value, q->length, NULL);
+       egg_asn1x_set_integer_as_usg (egg_asn1x_node (params_asn, "q", NULL), q->value, q->length, NULL);
 
        g = gck_attributes_find (attrs, CKA_BASE);
        g_return_val_if_fail (g, FALSE);
-       egg_asn1x_set_integer_as_raw (egg_asn1x_node (params_asn, "g", NULL), g->value, g->length, NULL);
+       egg_asn1x_set_integer_as_usg (egg_asn1x_node (params_asn, "g", NULL), g->value, g->length, NULL);
 
        value = gck_attributes_find (attrs, CKA_VALUE);
        g_return_val_if_fail (value, FALSE);
@@ -184,7 +184,7 @@ dsa_subject_public_key_from_attributes (GckAttributes *attrs, GNode *info_asn)
                        g_return_val_if_reached (FALSE);
 
        } else if (klass == CKO_PUBLIC_KEY) {
-               egg_asn1x_set_integer_as_raw (key_asn, value->value, value->length, NULL);
+               egg_asn1x_set_integer_as_usg (key_asn, value->value, value->length, NULL);
        }
 
        key = egg_asn1x_encode (key_asn, g_realloc, &n_key);

diff --git a/gcr/gcr-parser.c b/gcr/gcr-parser.c
index fcb6cc8..16df804 100644 (file)
--- a/gcr/gcr-parser.c
+++ b/gcr/gcr-parser.c
@@ -225,12 +225,12 @@ parsed_attribute (GcrParsed *parsed,
 }
 
 static gboolean
-parsed_asn1_attribute (GcrParsed *parsed,
-                       GNode *asn,
-                       const guchar *data,
-                       gsize n_data,
-                       const gchar *part,
-                       CK_ATTRIBUTE_TYPE type)
+parsed_asn1_number (GcrParsed *parsed,
+                    GNode *asn,
+                    const guchar *data,
+                    gsize n_data,
+                    const gchar *part,
+                    CK_ATTRIBUTE_TYPE type)
 {
        const guchar *value;
        gsize n_value;
@@ -239,11 +239,10 @@ parsed_asn1_attribute (GcrParsed *parsed,
        g_assert (data);
        g_assert (parsed);
 
-       value = egg_asn1x_get_raw_value (egg_asn1x_node (asn, part, NULL), &n_value);
+       value = egg_asn1x_get_integer_as_usg (egg_asn1x_node (asn, part, NULL), &n_value);
        if (value == NULL)
                return FALSE;
 
-       /* TODO: Convert to USG FROM STD */
        parsed_attribute (parsed, type, value, n_value);
        return TRUE;
 }
@@ -470,12 +469,12 @@ parse_der_private_key_rsa (GcrParser *self, const guchar *data, gsize n_data)
                goto done;
        }
 
-       if (!parsed_asn1_attribute (parsed, asn, data, n_data, "modulus", CKA_MODULUS) ||
-           !parsed_asn1_attribute (parsed, asn, data, n_data, "publicExponent", CKA_PUBLIC_EXPONENT) ||
-           !parsed_asn1_attribute (parsed, asn, data, n_data, "privateExponent", CKA_PRIVATE_EXPONENT) ||
-           !parsed_asn1_attribute (parsed, asn, data, n_data, "prime1", CKA_PRIME_1) ||
-           !parsed_asn1_attribute (parsed, asn, data, n_data, "prime2", CKA_PRIME_2) ||
-           !parsed_asn1_attribute (parsed, asn, data, n_data, "coefficient", CKA_COEFFICIENT))
+       if (!parsed_asn1_number (parsed, asn, data, n_data, "modulus", CKA_MODULUS) ||
+           !parsed_asn1_number (parsed, asn, data, n_data, "publicExponent", CKA_PUBLIC_EXPONENT) ||
+           !parsed_asn1_number (parsed, asn, data, n_data, "privateExponent", CKA_PRIVATE_EXPONENT) ||
+           !parsed_asn1_number (parsed, asn, data, n_data, "prime1", CKA_PRIME_1) ||
+           !parsed_asn1_number (parsed, asn, data, n_data, "prime2", CKA_PRIME_2) ||
+           !parsed_asn1_number (parsed, asn, data, n_data, "coefficient", CKA_COEFFICIENT))
                goto done;
 
        parsed_fire (self, parsed);
@@ -513,10 +512,10 @@ parse_der_private_key_dsa (GcrParser *self, const guchar *data, gsize n_data)
        parsed_boolean_attribute (parsed, CKA_PRIVATE, CK_TRUE);
        ret = GCR_ERROR_FAILURE;
 
-       if (!parsed_asn1_attribute (parsed, asn, data, n_data, "p", CKA_PRIME) ||
-           !parsed_asn1_attribute (parsed, asn, data, n_data, "q", CKA_SUBPRIME) ||
-           !parsed_asn1_attribute (parsed, asn, data, n_data, "g", CKA_BASE) ||
-           !parsed_asn1_attribute (parsed, asn, data, n_data, "priv", CKA_VALUE))
+       if (!parsed_asn1_number (parsed, asn, data, n_data, "p", CKA_PRIME) ||
+           !parsed_asn1_number (parsed, asn, data, n_data, "q", CKA_SUBPRIME) ||
+           !parsed_asn1_number (parsed, asn, data, n_data, "g", CKA_BASE) ||
+           !parsed_asn1_number (parsed, asn, data, n_data, "priv", CKA_VALUE))
                goto done;
 
        parsed_fire (self, parsed);
@@ -552,10 +551,10 @@ parse_der_private_key_dsa_parts (GcrParser *self, const guchar *keydata, gsize n
        parsed_boolean_attribute (parsed, CKA_PRIVATE, CK_TRUE);
        ret = GCR_ERROR_FAILURE;
 
-       if (!parsed_asn1_attribute (parsed, asn_params, params, n_params, "p", CKA_PRIME) ||
-           !parsed_asn1_attribute (parsed, asn_params, params, n_params, "q", CKA_SUBPRIME) ||
-           !parsed_asn1_attribute (parsed, asn_params, params, n_params, "g", CKA_BASE) ||
-           !parsed_asn1_attribute (parsed, asn_key, keydata, n_keydata, NULL, CKA_VALUE))
+       if (!parsed_asn1_number (parsed, asn_params, params, n_params, "p", CKA_PRIME) ||
+           !parsed_asn1_number (parsed, asn_params, params, n_params, "q", CKA_SUBPRIME) ||
+           !parsed_asn1_number (parsed, asn_params, params, n_params, "g", CKA_BASE) ||
+           !parsed_asn1_number (parsed, asn_key, keydata, n_keydata, NULL, CKA_VALUE))
                goto done;
 
        parsed_fire (self, parsed);