[gnome-keyring] gcr: Accept slightly invalid PKCS#12 files

[Stefan Walter <stefw src gnome org>]

commit e58e985b13a02a8803ab418d8d71019c22f2835f
Author: Stef Walter <stefw collabora co uk>
Date:   Mon Sep 12 12:21:05 2011 +0200

    gcr: Accept slightly invalid PKCS#12 files
    
     * In particular when the order of a SET OF is incorrect as is generated
       by certain implementations.
     * Revert cbecc802e8cf5803aac9fbd3c546b539773220b2 since this fix was
       wrong.
     * Add egg_asn1x_decode_no_validate() so that callers can validate on
       their own and specify validation options.

 egg/egg-asn1x.c  |   70 +++++++++++++++++++++++++++++++++++------------------
 egg/egg-asn1x.h  |    7 ++++-
 gcr/gcr-parser.c |   58 +++++++++++++++++++++++++++++++++++++++-----
 3 files changed, 103 insertions(+), 32 deletions(-)
---
diff --git a/egg/egg-asn1x.c b/egg/egg-asn1x.c
index 743ba2c..50bc133 100644
--- a/egg/egg-asn1x.c
+++ b/egg/egg-asn1x.c
@@ -158,7 +158,7 @@ struct _Abits {
 /* Forward Declarations */
 static gboolean anode_decode_anything (GNode*, Atlv*);
 static gboolean anode_decode_anything_for_flags (GNode *, Atlv*, gint);
-static gboolean anode_validate_anything (GNode*);
+static gboolean anode_validate_anything (GNode*, gboolean);
 static gboolean anode_encode_prepare (GNode*, gboolean want);
 
 static gint
@@ -354,13 +354,13 @@ anode_opts_lookup (GNode *node, gint type, const gchar *name)
 static gint
 compare_tlvs (Atlv *tlva, Atlv *tlvb)
 {
-	gint la = tlva->len;
-	gint lb = tlvb->len;
+	gint la = tlva->off + tlva->len;
+	gint lb = tlvb->off + tlvb->len;
 	gint res;
 
 	g_assert (tlva->buf);
 	g_assert (tlvb->buf);
-	res = memcmp (tlva->buf + tlva->off, tlvb->buf + tlvb->off, MIN (la, lb));
+	res = memcmp (tlva->buf, tlvb->buf, MIN (la, lb));
 	if (la == lb || res != 0)
 		return res;
 	return la < lb ? -1 : 1;
@@ -1139,14 +1139,12 @@ anode_decode_anything (GNode *node, Atlv *tlv)
 }
 
 gboolean
-egg_asn1x_decode (GNode *asn, gconstpointer data, gsize n_data)
+egg_asn1x_decode_no_validate (GNode *asn,
+                              gconstpointer data,
+                              gsize n_data)
 {
 	Atlv tlv;
 
-	g_return_val_if_fail (asn, FALSE);
-	g_return_val_if_fail (data, FALSE);
-	g_return_val_if_fail (n_data, FALSE);
-
 	egg_asn1x_clear (asn);
 
 	if (!anode_decode_tlv_for_data (data, (const guchar*)data + n_data, &tlv))
@@ -1158,7 +1156,25 @@ egg_asn1x_decode (GNode *asn, gconstpointer data, gsize n_data)
 	if (tlv.end - tlv.buf != n_data)
 		return FALSE;
 
-	return egg_asn1x_validate (asn);
+	return TRUE;
+}
+
+gboolean
+egg_asn1x_decode (GNode *asn,
+                  gconstpointer data,
+                  gsize n_data)
+{
+	gboolean ret;
+
+	g_return_val_if_fail (asn, FALSE);
+	g_return_val_if_fail (data, FALSE);
+	g_return_val_if_fail (n_data, FALSE);
+
+	ret = egg_asn1x_decode_no_validate (asn, data, n_data);
+	if (!ret)
+		return ret;
+
+	return egg_asn1x_validate (asn, TRUE);
 }
 
 /* -----------------------------------------------------------------------------------
@@ -1705,7 +1721,7 @@ egg_asn1x_encode (GNode *asn, EggAllocator allocator, gsize *n_data)
 		return NULL;
 
 	if (anode_encode_build (asn, data, length) &&
-	    anode_validate_anything (asn)) {
+	    anode_validate_anything (asn, TRUE)) {
 		anode_encode_commit (asn);
 		*n_data = length;
 		return data;
@@ -3334,7 +3350,8 @@ anode_validate_time (GNode *node, Atlv *tlv)
 }
 
 static gboolean
-anode_validate_choice (GNode *node)
+anode_validate_choice (GNode *node,
+                       gboolean strict)
 {
 	GNode *child, *choice;
 	Anode *an;
@@ -3344,7 +3361,7 @@ anode_validate_choice (GNode *node)
 	if (!choice)
 		return anode_failure (node, "one choice must be set");
 
-	if (!anode_validate_anything (choice))
+	if (!anode_validate_anything (choice, strict))
 		return FALSE;
 
 	for (child = node->children; child; child = child->next) {
@@ -3359,7 +3376,8 @@ anode_validate_choice (GNode *node)
 }
 
 static gboolean
-anode_validate_sequence_or_set (GNode *node)
+anode_validate_sequence_or_set (GNode *node,
+                                gboolean strict)
 {
 	GNode *child;
 	gulong tag = 0;
@@ -3371,7 +3389,7 @@ anode_validate_sequence_or_set (GNode *node)
 
 	/* All of the children must validate properly */
 	for (child = node->children; child; child = child->next) {
-		if (!anode_validate_anything (child))
+		if (!anode_validate_anything (child, strict))
 			return FALSE;
 
 		/* Tags must be in ascending order */
@@ -3388,7 +3406,8 @@ anode_validate_sequence_or_set (GNode *node)
 }
 
 static gboolean
-anode_validate_sequence_or_set_of (GNode *node)
+anode_validate_sequence_or_set_of (GNode *node,
+                                   gboolean strict)
 {
 	GNode *child;
 	Atlv *tlv, *ptlv;
@@ -3406,7 +3425,7 @@ anode_validate_sequence_or_set_of (GNode *node)
 	for (child = node->children; child; child = child->next) {
 		tlv = anode_get_tlv_data (child);
 		if (tlv) {
-			if (!anode_validate_anything (child))
+			if (!anode_validate_anything (child, strict))
 				return FALSE;
 
 			/* Tag must have same tag as top */
@@ -3416,7 +3435,8 @@ anode_validate_sequence_or_set_of (GNode *node)
 				return anode_failure (node, "invalid mismatched content");
 
 			/* Set of must be in ascending order */
-			if (type == TYPE_SET_OF && ptlv && compare_tlvs (ptlv, tlv) > 0)
+			if (strict && type == TYPE_SET_OF &&
+			    ptlv != NULL && compare_tlvs (ptlv, tlv) > 0)
 				return anode_failure (node, "content must be in ascending order");
 			ptlv = tlv;
 			++count;
@@ -3427,7 +3447,8 @@ anode_validate_sequence_or_set_of (GNode *node)
 }
 
 static gboolean
-anode_validate_anything (GNode *node)
+anode_validate_anything (GNode *node,
+                         gboolean strict)
 {
 	Atlv *tlv;
 	gint type;
@@ -3471,16 +3492,16 @@ anode_validate_anything (GNode *node)
 	case TYPE_ANY:
 		return TRUE;
 	case TYPE_CHOICE:
-		return anode_validate_choice (node);
+		return anode_validate_choice (node, strict);
 
 	/* Structured types */
 	case TYPE_SEQUENCE:
 	case TYPE_SET:
-		return anode_validate_sequence_or_set (node);
+		return anode_validate_sequence_or_set (node, strict);
 
 	case TYPE_SEQUENCE_OF:
 	case TYPE_SET_OF:
-		return anode_validate_sequence_or_set_of (node);
+		return anode_validate_sequence_or_set_of (node, strict);
 
 	default:
 		g_return_val_if_reached (FALSE);
@@ -3488,10 +3509,11 @@ anode_validate_anything (GNode *node)
 }
 
 gboolean
-egg_asn1x_validate (GNode *asn)
+egg_asn1x_validate (GNode *asn,
+                    gboolean strict)
 {
 	g_return_val_if_fail (asn, FALSE);
-	return anode_validate_anything (asn);
+	return anode_validate_anything (asn, strict);
 }
 
 /* -----------------------------------------------------------------------------------
diff --git a/egg/egg-asn1x.h b/egg/egg-asn1x.h
index 86b76c5..b6c8427 100644
--- a/egg/egg-asn1x.h
+++ b/egg/egg-asn1x.h
@@ -54,7 +54,12 @@ gboolean            egg_asn1x_decode                 (GNode *asn,
                                                       gconstpointer data,
                                                       gsize n_data);
 
-gboolean            egg_asn1x_validate               (GNode *asn);
+gboolean            egg_asn1x_decode_no_validate     (GNode *asn,
+                                                      gconstpointer data,
+                                                      gsize n_data);
+
+gboolean            egg_asn1x_validate               (GNode *asn,
+                                                      gboolean strict);
 
 gpointer            egg_asn1x_encode                 (GNode *asn,
                                                       EggAllocator allocator,
diff --git a/gcr/gcr-parser.c b/gcr/gcr-parser.c
index 1f93c1f..e3efec0 100644
--- a/gcr/gcr-parser.c
+++ b/gcr/gcr-parser.c
@@ -815,6 +815,38 @@ done:
  * PKCS12
  */
 
+static GNode *
+decode_pkcs12_asn1_accepting_invalid_crap (const ASN1_ARRAY_TYPE *defs,
+                                           const gchar *identifier,
+                                           gconstpointer data,
+                                           gsize n_data)
+{
+	GNode *asn;
+
+	/*
+	 * Because PKCS#12 files, the bags specifically, are notorious for
+	 * being crappily constructed and are often break rules such as DER
+	 * sorting order etc.. we parse the DER in a non-strict fashion.
+	 *
+	 * The rules in DER are designed for X.509 certificates, so there is
+	 * only one way to represent a given certificate (although they fail
+	 * at that as well). But with PKCS#12 we don't have such high
+	 * requirements, and we can slack off on our validation.
+	 */
+
+	asn = egg_asn1x_create (defs, identifier);
+	g_return_val_if_fail (asn != NULL, NULL);
+
+	/* Passing FALSE as the strictness argument */
+	if (!egg_asn1x_decode_no_validate (asn, data, n_data) ||
+	    !egg_asn1x_validate (asn, FALSE)) {
+		egg_asn1x_destroy (asn);
+		asn = NULL;
+	}
+
+	return asn;
+}
+
 static gint
 handle_pkcs12_cert_bag (GcrParser *self, const guchar *data, gsize n_data)
 {
@@ -826,7 +858,9 @@ handle_pkcs12_cert_bag (GcrParser *self, const guchar *data, gsize n_data)
 	gint ret;
 
 	ret = GCR_ERROR_UNRECOGNIZED;
-	asn = egg_asn1x_create_and_decode (pkix_asn1_tab, "pkcs-12-CertBag", data, n_data);
+	asn = decode_pkcs12_asn1_accepting_invalid_crap (pkix_asn1_tab,
+	                                                 "pkcs-12-CertBag",
+	                                                 data, n_data);
 	if (!asn)
 		goto done;
 
@@ -902,7 +936,9 @@ handle_pkcs12_bag (GcrParser *self, const guchar *data, gsize n_data)
 
 	ret = GCR_ERROR_UNRECOGNIZED;
 
-	asn = egg_asn1x_create_and_decode (pkix_asn1_tab, "pkcs-12-SafeContents", data, n_data);
+	asn = decode_pkcs12_asn1_accepting_invalid_crap (pkix_asn1_tab,
+	                                                 "pkcs-12-SafeContents",
+	                                                 data, n_data);
 	if (!asn)
 		goto done;
 
@@ -981,7 +1017,9 @@ handle_pkcs12_encrypted_bag (GcrParser *self, const guchar *data, gsize n_data)
 
 	ret = GCR_ERROR_UNRECOGNIZED;
 
-	asn = egg_asn1x_create_and_decode (pkix_asn1_tab, "pkcs-7-EncryptedData", data, n_data);
+	asn = decode_pkcs12_asn1_accepting_invalid_crap (pkix_asn1_tab,
+	                                                 "pkcs-7-EncryptedData",
+	                                                 data, n_data);
 	if (!asn)
 		goto done;
 
@@ -1068,7 +1106,9 @@ handle_pkcs12_safe (GcrParser *self, const guchar *data, gsize n_data)
 
 	ret = GCR_ERROR_UNRECOGNIZED;
 
-	asn = egg_asn1x_create_and_decode (pkix_asn1_tab, "pkcs-12-AuthenticatedSafe", data, n_data);
+	asn = decode_pkcs12_asn1_accepting_invalid_crap (pkix_asn1_tab,
+	                                                 "pkcs-12-AuthenticatedSafe",
+	                                                 data, n_data);
 	if (!asn)
 		goto done;
 
@@ -1097,7 +1137,9 @@ handle_pkcs12_safe (GcrParser *self, const guchar *data, gsize n_data)
 		if (oid == GCR_OID_PKCS7_DATA) {
 
 			egg_asn1x_destroy (asn_content);
-			asn_content = egg_asn1x_create_and_decode (pkix_asn1_tab, "pkcs-7-Data", bag, n_bag);
+			asn_content = decode_pkcs12_asn1_accepting_invalid_crap (pkix_asn1_tab,
+			                                                         "pkcs-7-Data",
+			                                                         bag, n_bag);
 			if (!asn_content)
 				goto done;
 
@@ -1236,7 +1278,8 @@ parse_der_pkcs12 (GcrParser *self, const guchar *data, gsize n_data)
 
 	ret = GCR_ERROR_UNRECOGNIZED;
 
-	asn = egg_asn1x_create_and_decode (pkix_asn1_tab, "pkcs-12-PFX", data, n_data);
+	asn = decode_pkcs12_asn1_accepting_invalid_crap (pkix_asn1_tab, "pkcs-12-PFX",
+	                                                 data, n_data);
 	if (!asn)
 		goto done;
 
@@ -1256,7 +1299,8 @@ parse_der_pkcs12 (GcrParser *self, const guchar *data, gsize n_data)
 	if (!element)
 		goto done;
 
-	asn_content = egg_asn1x_create_and_decode (pkix_asn1_tab, "pkcs-7-Data", element, n_element);
+	asn_content = decode_pkcs12_asn1_accepting_invalid_crap (pkix_asn1_tab, "pkcs-7-Data",
+	                                                         element, n_element);
 	if (!asn_content)
 		goto done;