GNOME.org
 

[gnome-keyring] [egg, gcr, pkcs11] Take length of ASN.1 into account, when parsing.

commit fc589f391ac9b3ef9d987736cd9d0522a608ea40
Author: Stefan Walter <Stefan Walter>
Date:   Sun Aug 2 20:30:24 2009 +0000

    [egg, gcr, pkcs11] Take length of ASN.1 into account, when parsing.
    
    When parsing ASN.1, take length of elements into account, in order
    to prevent null character related vulnerabilities.

 egg/egg-asn1.c                        |  104 ++++++++++++++++++++++----------
 egg/egg-asn1.h                        |    4 +-
 egg/tests/unit-test-asn1.c            |    4 +-
 gcr/gcr-certificate-details-widget.c  |   18 ++----
 gcr/gcr-certificate.c                 |    3 +-
 pkcs11/gck/gck-certificate.c          |    7 ++-
 pkcs11/gck/gck-data-der.c             |    6 +-
 pkcs11/gck/tests/unit-test-data-der.c |    5 +-
 8 files changed, 94 insertions(+), 57 deletions(-)
---
diff --git a/egg/egg-asn1.c b/egg/egg-asn1.c
index fc4708f..3ba0bc8 100644
--- a/egg/egg-asn1.c
+++ b/egg/egg-asn1.c
@@ -222,6 +222,7 @@ egg_asn1_read_value (ASN1_TYPE asn, const gchar *part, gsize *len, EggAllocator
 	
 	g_return_val_if_fail (asn != NULL, NULL);
 	g_return_val_if_fail (part != NULL, NULL);
+	g_return_val_if_fail (len != NULL, NULL);
 	
 	if (allocator == NULL)
 		allocator = (EggAllocator)g_realloc;
@@ -241,7 +242,7 @@ egg_asn1_read_value (ASN1_TYPE asn, const gchar *part, gsize *len, EggAllocator
 	if (res != ASN1_SUCCESS) {
 		(allocator) (buf, 0);
 		buf = NULL;
-	} else if (len) {
+	} else {
 		*len = l;
 	}
 	
@@ -262,20 +263,42 @@ egg_asn1_write_value (ASN1_TYPE asn, const gchar *part,
 	return res == ASN1_SUCCESS;
 }
 
+static gboolean
+ascii_length_equals (const gchar *str, gconstpointer data, gsize n_data)
+{
+	g_assert (str);
+	if (!data)
+		return FALSE;
+	if (strlen (str) != n_data)
+		return FALSE;
+	return strncmp (str, data, n_data) == 0;
+}
+
+static gboolean
+ascii_length_case_equals (const gchar *str, gconstpointer data, gsize n_data)
+{
+	g_assert (str);
+	if (!data)
+		return FALSE;
+	if (strlen (str) != n_data)
+		return FALSE;
+	return g_ascii_strncasecmp (str, data, n_data) == 0;
+}
+
 gboolean
 egg_asn1_read_boolean (ASN1_TYPE asn, const gchar *part, gboolean *val)
 {
 	gchar buffer[32];
-	int n_buffer = sizeof (buffer) - 1;
+	int n_buffer = sizeof (buffer);
 	int res;
 	
 	memset (buffer, 0, sizeof (buffer));
 	
 	res = asn1_read_value (asn, part, buffer, &n_buffer);
-	if (res != ASN1_SUCCESS)
+	if (res != ASN1_SUCCESS || !n_buffer)
 		return FALSE;
 		
-	if (g_ascii_strcasecmp (buffer, "TRUE") == 0)
+	if (ascii_length_case_equals ("TRUE", buffer, n_buffer - 1))
 		*val = TRUE;
 	else
 		*val = FALSE;
@@ -332,15 +355,20 @@ egg_asn1_read_oid (ASN1_TYPE asn, const gchar *part)
 {
 	GQuark quark;
 	guchar *buf;
+	gpointer end;
 	gsize n_buf;
 	
 	buf = egg_asn1_read_value (asn, part, &n_buf, NULL);
-	if (!buf)
+	if (!buf || !n_buf)
 		return 0;
-		
+
+	/* Make sure the string is actually that long */
+	end = memchr (buf, 0, n_buf - 1);
+	if (end != NULL)
+		return 0;
+
 	quark = g_quark_from_string ((gchar*)buf);
 	g_free (buf);
-	
 	return quark;
 }
 
@@ -443,9 +471,9 @@ time_t timegm(struct tm *t)
 #endif //NOT_HAVE_TIMEGM
 
 static gboolean
-parse_utc_time (const gchar *time, struct tm* when, gint *offset)
+parse_utc_time (const gchar *time, gsize n_time,
+                struct tm* when, gint *offset)
 {
-	guint n_time;
 	const char *p, *e;
 	int year;
 
@@ -453,7 +481,8 @@ parse_utc_time (const gchar *time, struct tm* when, gint *offset)
 	g_assert (time);
 	g_assert (offset);
 	
-	n_time = strlen (time);
+	if (n_time != strlen (time))
+		return FALSE;
 	
 	/* YYMMDDhhmmss.ffff Z | +0000 */
 	if (n_time < 6 || n_time >= 28) 
@@ -573,30 +602,34 @@ when_to_time (struct tm *when, gint offset)
 }
 
 glong
-egg_asn1_time_parse_utc (const gchar *time)
+egg_asn1_time_parse_utc (const gchar *time, gssize n_time)
 {
 	struct tm when;
 	gint offset;
 	
 	g_return_val_if_fail (time, -1);
+
+	if (n_time == -1)
+		n_time = strlen (time);
 	
-	if (!parse_utc_time (time, &when, &offset))
+	if (!parse_utc_time (time, n_time, &when, &offset))
 		return -1;
 	
 	return when_to_time (&when, offset);
 }
 
 static gboolean
-parse_general_time (const gchar *time, struct tm* when, gint *offset)
+parse_general_time (const gchar *time, gsize n_time,
+                    struct tm* when, gint *offset)
 {
 	const char *p, *e;
-	guint n_time;
 
 	g_assert (time);
 	g_assert (when);
 	g_assert (offset);
-	
-	n_time = strlen (time);
+
+	if (strlen (time) != n_time)
+		return FALSE;
 	
 	/* YYYYMMDDhhmmss.ffff Z | +0000 */
 	if (n_time < 8 || n_time >= 30) 
@@ -691,14 +724,17 @@ parse_general_time (const gchar *time, struct tm* when, gint *offset)
 }
 
 glong
-egg_asn1_time_parse_general (const gchar *time)
+egg_asn1_time_parse_general (const gchar *time, gssize n_time)
 {
 	struct tm when;
 	gint offset;
 	
 	g_return_val_if_fail (time, -1);
 	
-	if (!parse_general_time (time, &when, &offset))
+	if (n_time == -1)
+		n_time = strlen (time);
+
+	if (!parse_general_time (time, n_time, &when, &offset))
 		return -1;
 	
 	return when_to_time (&when, offset);
@@ -716,31 +752,32 @@ read_asn1_time (ASN1_TYPE asn, const gchar *part, struct tm *when, gint *offset)
 	g_assert (when);
 	g_assert (offset);
 
-	len = sizeof (ttime) - 1;
+	len = sizeof (ttime);
 	res = asn1_read_value (asn, part, ttime, &len);
 	if (res != ASN1_SUCCESS)
 		return FALSE;
-		
+	--len; /* libtasn1 returns the null terminator in this count */
+
 	/* CHOICE */
-	if (strcmp (ttime, "generalTime") == 0) {
+	if (ascii_length_equals ("generalTime", ttime, len)) {
 		name = g_strconcat (part, ".generalTime", NULL);
 		len = sizeof (ttime) - 1;
 		res = asn1_read_value (asn, name, ttime, &len);
 		g_free (name);
 		if (res != ASN1_SUCCESS)
 			return FALSE;
-		return parse_general_time (ttime, when, offset);
+		return parse_general_time (ttime, len - 1, when, offset);
 		
 	/* UTCTIME */
-	} else {
+	} else if (ascii_length_equals ("utcTime", ttime, len)) {
 		name = g_strconcat (part, ".utcTime", NULL);
 		len = sizeof (ttime) - 1;
 		res = asn1_read_value (asn, name, ttime, &len);
 		g_free (name);
 		if (res != ASN1_SUCCESS)
 			return FALSE;
-		return parse_utc_time (ttime, when, offset);
-    	}
+		return parse_utc_time (ttime, len - 1, when, offset);
+	}
 
 	return FALSE;	
 }
@@ -807,6 +844,7 @@ dn_print_oid_value_parsed (GQuark oid, guint flags, const guchar *data, gsize le
 	ASN1_TYPE asn1;
 	gchar *part;
 	gchar *value;
+	gsize n_value;
 	
 	g_assert (data);
 	g_assert (len);
@@ -824,19 +862,19 @@ dn_print_oid_value_parsed (GQuark oid, guint flags, const guchar *data, gsize le
 		return NULL;
 	}
 
-	value = (gchar*)egg_asn1_read_value (asn1, "", NULL, NULL);
+	value = (gchar*)egg_asn1_read_value (asn1, "", &n_value, NULL);
 	
 	/*
 	 * If it's a choice element, then we have to read depending
 	 * on what's there.
 	 */
 	if (value && (flags & EGG_OID_IS_CHOICE)) {
-		if (strcmp ("printableString", value) == 0 ||
-		    strcmp ("ia5String", value) == 0 ||
-		    strcmp ("utf8String", value) == 0 ||
-		    strcmp ("teletexString", value) == 0) {
+		if (ascii_length_equals ("printableString", value, n_value - 1) ||
+			ascii_length_equals ("ia5String", value, n_value - 1 ) ||
+			ascii_length_equals ("utf8String", value, n_value - 1) ||
+			ascii_length_equals ("teletexString", value, n_value - 1)) {
 			part = value;
-			value = (gchar*)egg_asn1_read_value (asn1, part, NULL, NULL);
+			value = (gchar*)egg_asn1_read_value (asn1, part, &n_value, NULL);
 			g_free (part);
 		} else {
 			g_free (value);
@@ -852,8 +890,8 @@ dn_print_oid_value_parsed (GQuark oid, guint flags, const guchar *data, gsize le
 	/* 
 	 * Now we make sure it's UTF-8. 
 	 */
-	if (!g_utf8_validate (value, -1, NULL)) {
-		gchar *hex = dn_print_hex_value ((guchar*)value, strlen (value));
+	if (!g_utf8_validate (value, n_value, NULL)) {
+		gchar *hex = dn_print_hex_value ((guchar*)value, n_value);
 		g_free (value);
 		value = hex;
 	}
diff --git a/egg/egg-asn1.h b/egg/egg-asn1.h
index 04e369d..3ee6d43 100644
--- a/egg/egg-asn1.h
+++ b/egg/egg-asn1.h
@@ -75,9 +75,9 @@ gchar*             egg_asn1_read_dn                       (ASN1_TYPE asn, const
 gchar*             egg_asn1_read_dn_part                  (ASN1_TYPE asn, const gchar *part, const gchar *match);
 
 
-glong              egg_asn1_time_parse_utc                (const gchar* value);
+glong              egg_asn1_time_parse_utc                (const gchar* value, gssize n_value);
 
-glong              egg_asn1_time_parse_general            (const gchar* value);
+glong              egg_asn1_time_parse_general            (const gchar* value, gssize n_value);
 
 
 typedef void       (*EggAsn1DnCallback)                   (guint index, GQuark oid, const guchar *value,
diff --git a/egg/tests/unit-test-asn1.c b/egg/tests/unit-test-asn1.c
index 96a542b..bcd44d0 100644
--- a/egg/tests/unit-test-asn1.c
+++ b/egg/tests/unit-test-asn1.c
@@ -332,7 +332,7 @@ DEFINE_TEST(general_time)
 	const TimeTestData *data;
 	
 	for (data = generalized_time_test_data; data->value; ++data) {
-		when = egg_asn1_time_parse_general (data->value);
+		when = egg_asn1_time_parse_general (data->value, -1);
 		if (data->ref != when) {
 			printf ("%s", data->value);
 			printf ("%s != ", ctime (&when));
@@ -350,7 +350,7 @@ DEFINE_TEST(utc_time)
 	const TimeTestData *data;
 	
 	for (data = utc_time_test_data; data->value; ++data) {
-		when = egg_asn1_time_parse_utc (data->value);
+		when = egg_asn1_time_parse_utc (data->value, -1);
 		if (data->ref != when) {
 			printf ("%s", data->value);
 			printf ("%s != ", ctime (&when));
diff --git a/gcr/gcr-certificate-details-widget.c b/gcr/gcr-certificate-details-widget.c
index a66f28c..6621c16 100644
--- a/gcr/gcr-certificate-details-widget.c
+++ b/gcr/gcr-certificate-details-widget.c
@@ -182,10 +182,10 @@ append_extension (GcrCertificateDetailsWidget *self, ASN1_TYPE asn,
 {
 	GQuark oid;
 	gchar *name, *display;
-	gsize n_val, n_value;
+	gsize n_value;
 	const guchar *value;
 	const gchar *text;
-	guchar *val;
+	gboolean critical;
 	int len, res;
 	
 	/* Make sure it is present */
@@ -225,18 +225,10 @@ append_extension (GcrCertificateDetailsWidget *self, ASN1_TYPE asn,
 	
 	/* Critical */
 	name = g_strdup_printf ("tbsCertificate.extensions.?%u.critical", index);
-	val = egg_asn1_read_value (asn, name, &n_val, NULL);
+	if (egg_asn1_read_boolean (asn, name, &critical))
+		append_field_and_value (self, _("Critical"), critical ? _("Yes") : _("No"), FALSE);
 	g_free (name);
 	
-	if (!val || n_val < 1 || val[0] != 'T')
-		text = _("Yes");
-	else
-		text = _("No");
-	g_free (val);
-	
-	append_field_and_value (self, _("Critical"), text, FALSE);
-	
-	
 	return TRUE;
 }
 
@@ -273,7 +265,7 @@ on_parsed_dn_part (guint index, GQuark oid, const guchar *value,
 	
 	display = egg_asn1_dn_print_value (oid, value, n_value);
 	if (display == NULL)
-		display = "";
+		display = g_strdup ("");
 	
 	append_field_and_value (self, field, display, FALSE);
 	g_free (field);
diff --git a/gcr/gcr-certificate.c b/gcr/gcr-certificate.c
index 13990f5..c16c6a1 100644
--- a/gcr/gcr-certificate.c
+++ b/gcr/gcr-certificate.c
@@ -586,7 +586,7 @@ gcr_certificate_get_fingerprint_hex (GcrCertificate *self, GChecksumType type)
 /**
  * gcr_certificate_get_serial_number:
  * @self: a #GcrCertificate
- * @length: the length of the returned data.
+ * @n_length: the length of the returned data.
  * 
  * Get the raw binary serial number of the certificate.
  * 
@@ -601,6 +601,7 @@ gcr_certificate_get_serial_number (GcrCertificate *self, gsize *n_length)
 	GcrCertificateInfo *info;
 	
 	g_return_val_if_fail (GCR_IS_CERTIFICATE (self), NULL);
+	g_return_val_if_fail (n_length, NULL);
 	
 	info = certificate_info_load (self);
 	g_return_val_if_fail (info, NULL);
diff --git a/pkcs11/gck/gck-certificate.c b/pkcs11/gck/gck-certificate.c
index a3b4c41..3aff6d7 100644
--- a/pkcs11/gck/gck-certificate.c
+++ b/pkcs11/gck/gck-certificate.c
@@ -674,7 +674,12 @@ gck_certificate_get_extension (GckCertificate *self, GQuark oid,
 		val = egg_asn1_read_value (self->pv->asn1, name, &n_val, NULL);
 		g_free (name);
 		
-		if (!val || n_val < 1 || val[0] != 'T')
+		/*
+		 * We're pretty liberal in what we accept as critical. The goal
+		 * here is not to accidentally mark as non-critical what some
+		 * other x509 implementation meant to say critical.
+		 */
+		if (!val || n_val < 1 || g_ascii_toupper (val[0]) != 'T')
 			*critical = FALSE;
 		else
 			*critical = TRUE;
diff --git a/pkcs11/gck/gck-data-der.c b/pkcs11/gck/gck-data-der.c
index 1cf19a7..490177d 100644
--- a/pkcs11/gck/gck-data-der.c
+++ b/pkcs11/gck/gck-data-der.c
@@ -1223,9 +1223,9 @@ gck_data_der_read_key_usage (const guchar *data, gsize n_data, guint *key_usage)
 
 	memset (buf, 0, sizeof (buf));
 	len = sizeof (buf);
-  	res = asn1_read_value (asn, "", buf, &len);
-  	if (res != ASN1_SUCCESS)
-  		goto done;
+	res = asn1_read_value (asn, "", buf, &len);
+	if (res != ASN1_SUCCESS || len < 1 || len > 2)
+		goto done;
 
 	*key_usage = buf[0] | (buf[1] << 8);
 	ret = GCK_DATA_SUCCESS;
diff --git a/pkcs11/gck/tests/unit-test-data-der.c b/pkcs11/gck/tests/unit-test-data-der.c
index f78148e..cc04dfd 100644
--- a/pkcs11/gck/tests/unit-test-data-der.c
+++ b/pkcs11/gck/tests/unit-test-data-der.c
@@ -280,6 +280,7 @@ static const guchar*
 find_extension (ASN1_TYPE asn, const guchar *data, gsize n_data, const gchar *oid, gsize *n_extension)
 {
 	const guchar *value;
+	gsize n_exoid;
 	guchar *exoid;
 	gchar *name;
 	guint index;
@@ -291,13 +292,13 @@ find_extension (ASN1_TYPE asn, const guchar *data, gsize n_data, const gchar *oi
 		
 		/* Make sure it is present */
 		name = g_strdup_printf ("tbsCertificate.extensions.?%u.extnID", index);
-		exoid = egg_asn1_read_value (asn, name, NULL, NULL);
+		exoid = egg_asn1_read_value (asn, name, &n_exoid, NULL);
 		g_free (name);
 
 		if (!exoid)
 			return NULL;
 
-		if (strcmp ((gchar*)exoid, oid) == 0) {
+		if (n_exoid == strlen (oid) && strcmp ((gchar*)exoid, oid) == 0) {
 			g_free (exoid);
 			name = g_strdup_printf ("tbsCertificate.extensions.?%u.extnValue", index);
 			value = egg_asn1_read_content (asn, data, n_data, name, n_extension);
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]