From acc403e77774308cfd7ba7cab3d65a7799d2ff47 Mon Sep 17 00:00:00 2001 From: Stef Walter Date: Tue, 4 Oct 2011 18:15:37 +0200 Subject: [PATCH] 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. --- egg/egg-asn1x.c | 125 +++++++++++++++++++++++++++++++++++------ egg/egg-asn1x.h | 11 +++- egg/tests/test-asn1.c | 53 +++++++++++++++++ gcr/gcr-certificate-renderer.c | 8 +-- gcr/gcr-certificate.c | 4 +- gcr/gcr-fingerprint.c | 14 ++--- gcr/gcr-parser.c | 43 +++++++------- 7 files changed, 206 insertions(+), 52 deletions(-) diff --git a/egg/egg-asn1x.c b/egg/egg-asn1x.c index db64fd7..f2d4b42 100644 --- 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 --- 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 --- 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 --- 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 --- 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 --- 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 --- 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); -- 2.7.4