uri: fix the query attribute parsing

The pin-* attributes belong to the query part. We should not parse them
until we see a '?' and they're separated with a '&'.

This might be an important thing -- some of the query attributes may
have security implications reaching outside scope of the token itself, to the
host system itself. E.g. a pin-source may cause the consumer to access a file
or module-path (unimplemented) execute code. The user may want to just chop the
attribute part off if they want the consumer access the token and not take the
security considerations into account.

2 files changed, 42 insertions, 14 deletions

diff --git a/p11-kit/test-uri.c b/p11-kit/test-uri.c
index 1fb5081..7c38e60 100644
--- a/p11-kit/test-uri.c
+++ b/p11-kit/test-uri.c
@@ -1335,7 +1335,7 @@ test_uri_pin_source (void)
 	assert (strstr (string, "pin-source=%7cmy-pin-file") != NULL);
 	free (string);
 
-	ret = p11_kit_uri_parse ("pkcs11:pin-source=blah%2Fblah", P11_KIT_URI_FOR_ANY, uri);
+	ret = p11_kit_uri_parse ("pkcs11:?pin-source=blah%2Fblah", P11_KIT_URI_FOR_ANY, uri);
 	assert_num_eq (P11_KIT_URI_OK, ret);
 
 	pin_source = p11_kit_uri_get_pin_source (uri);
@@ -1371,7 +1371,7 @@ test_uri_pin_value (void)
 	assert (strstr (string, "pkcs11:pin-value=1%2a%26%23%25%26%40%28") != NULL);
 	free (string);
 
-	ret = p11_kit_uri_parse ("pkcs11:pin-value=blah%2Fblah", P11_KIT_URI_FOR_ANY, uri);
+	ret = p11_kit_uri_parse ("pkcs11:?pin-value=blah%2Fblah", P11_KIT_URI_FOR_ANY, uri);
 	assert_num_eq (P11_KIT_URI_OK, ret);
 
 	pin_value = p11_kit_uri_get_pin_value (uri);
@@ -1389,7 +1389,7 @@ test_uri_pin_value_bad (void)
 	uri = p11_kit_uri_new ();
 	assert_ptr_not_null (uri);
 
-	ret = p11_kit_uri_parse ("pkcs11:pin-value=blahblah%2", P11_KIT_URI_FOR_ANY, uri);
+	ret = p11_kit_uri_parse ("pkcs11:?pin-value=blahblah%2", P11_KIT_URI_FOR_ANY, uri);
 	assert_num_eq (P11_KIT_URI_BAD_ENCODING, ret);
 
 	p11_kit_uri_free (uri);
diff --git a/p11-kit/uri.c b/p11-kit/uri.c
index ddb29a5..832980d 100644
--- a/p11-kit/uri.c
+++ b/p11-kit/uri.c
@@ -1393,17 +1393,14 @@ p11_kit_uri_parse (const char *string, P11KitUriType uri_type,
 	uri->pin_value = NULL;
 	uri->slot_id = (CK_SLOT_ID)-1;
 
+	/* Parse the path. */
 	for (;;) {
-		spos = strchr (string, ';');
-		if (spos == NULL) {
-			spos = string + strlen (string);
-			assert (*spos == '\0');
-			if (spos == string)
-				break;
-		}
+		spos = string + strcspn (string, ";?");
+		if (spos == string)
+			break;
 
 		epos = strchr (string, '=');
-		if (epos == NULL || spos == string || epos == string || epos >= spos) {
+		if (epos == NULL || epos == string || epos >= spos) {
 			free (allocated);
 			return P11_KIT_URI_BAD_SYNTAX;
 		}
@@ -1423,8 +1420,6 @@ p11_kit_uri_parse (const char *string, P11KitUriType uri_type,
 			ret = parse_module_info (string, epos, epos + 1, spos, uri);
 		if (ret == 0 && (uri_type & P11_KIT_URI_FOR_MODULE_WITH_VERSION) == P11_KIT_URI_FOR_MODULE_WITH_VERSION)
 			ret = parse_module_version_info (string, epos, epos + 1, spos, uri);
-		if (ret == 0)
-			ret = parse_extra_info (string, epos, epos + 1, spos, uri);
 
 		if (ret < 0) {
 			free (allocated);
@@ -1433,9 +1428,42 @@ p11_kit_uri_parse (const char *string, P11KitUriType uri_type,
 		if (ret == 0)
 			uri->unrecognized = true;
 
+		string = spos;
 		if (*spos == '\0')
 			break;
-		string = spos + 1;
+		if (*spos == '?')
+			break;
+		string++;
+	}
+
+	/* Parse the query. */
+	for (;;) {
+		if (*string == '\0')
+			break;
+		string++;
+		spos = strchr (string, '&');
+		if (spos == NULL) {
+			spos = string + strlen (string);
+			assert (*spos == '\0');
+			if (spos == string)
+				break;
+		}
+
+		epos = strchr (string, '=');
+		if (epos == NULL || spos == string || epos == string || epos >= spos) {
+			free (allocated);
+			return P11_KIT_URI_BAD_SYNTAX;
+		}
+
+		ret = parse_extra_info (string, epos, epos + 1, spos, uri);
+		if (ret < 0) {
+			free (allocated);
+			return ret;
+		}
+		if (ret == 0)
+			uri->unrecognized = true;
+
+		string = spos;
 	}
 
 	free (allocated);