SAML 2.0: when verifying query signature, do not presume order of field and separator

 * lasso/xml/tools.c:
   in lasso_saml2_verify_query_signature, extract needed field and order
   them appropriately before computing digest, expect ';' as well as '&'
   as separator.
 * tests/random_test.c:
   add non-regression tests for query signature validation.
 * tests/Makefile.am:
   make tests link agains static version of liblasso, to get access to
   private functions.

3 files changed, 126 insertions, 33 deletions

diff --git a/lasso/xml/tools.c b/lasso/xml/tools.c
index 9d72e8f9..f40f10f6 100644
--- a/lasso/xml/tools.c
+++ b/lasso/xml/tools.c
@@ -681,45 +681,91 @@ lasso_saml2_query_verify_signature(const char *query, const xmlSecKey *sender_pu
 {
 	RSA *rsa = NULL;
 	DSA *dsa = NULL;
-	gchar **str_split = NULL;
 	char *digest = NULL, *b64_signature = NULL;
 	xmlSecByte *signature = NULL;
 	int key_size, status = 0, ret = 0;
+	char *query_copy = NULL;
+	char *signed_query = NULL;
+	char *i = NULL;
+	char **components = NULL, **j = NULL;
+	int n = 0;
+	char *saml_request_response = NULL;
+	char *relaystate = NULL;
 	char *sig_alg, *usig_alg = NULL;
 
-	g_return_val_if_fail(query != NULL, LASSO_PARAM_ERROR_INVALID_VALUE);
+	lasso_return_val_if_fail(query != NULL, LASSO_PARAM_ERROR_INVALID_VALUE);
+	lasso_return_val_if_fail(lasso_flag_verify_signature, 0);
+	lasso_return_val_if_fail(sender_public_key != NULL, LASSO_PARAM_ERROR_INVALID_VALUE);
+	lasso_return_val_if_fail(sender_public_key->value != NULL, LASSO_PARAM_ERROR_INVALID_VALUE);
 
-	if (lasso_flag_verify_signature == FALSE) {
-		return 0;
+	/* extract fields */
+	i = query_copy = g_strdup(query);
+	n = 1;
+	while (*i) {
+		if (*i == '&' || *i == ';')
+			n++;
+		i++;
+	}
+	components = g_new0(char*, n+1);
+	components[n] = NULL;
+	n = 0;
+	i = query_copy;
+	components[n] = query_copy;
+	n += 1;
+	while (*i) {
+		if (*i == '&' || *i == ';') {
+			*i = '\0';
+			components[n] = i + 1;
+			n++;
+		}
+		i++;
+	}
+	/* extract specific fields */
+	j = components;
+#define match_field(x) \
+	(strncmp(x "=", *j, sizeof(x)) == 0)
+#define value strchr(*j, '=') + 1
+	while (*j) {
+		if (match_field(LASSO_SAML2_FIELD_RESPONSE)
+				|| match_field(LASSO_SAML2_FIELD_REQUEST)) {
+			saml_request_response = *j;
+		} else if (match_field(LASSO_SAML2_FIELD_RELAYSTATE)) {
+			relaystate = *j;
+		} else if (match_field(LASSO_SAML2_FIELD_SIGALG)) {
+			sig_alg = *j;
+		} else if (match_field(LASSO_SAML2_FIELD_SIGNATURE)) {
+			b64_signature = value;
+			b64_signature = xmlURIUnescapeString(b64_signature, 0, NULL);
+		}
+		++j;
 	}
+#undef match_field
+#undef value
 
-	g_return_val_if_fail(sender_public_key != NULL, LASSO_PARAM_ERROR_INVALID_VALUE);
-	g_return_val_if_fail(sender_public_key->value != NULL, LASSO_PARAM_ERROR_INVALID_VALUE);
-
-	/* split query, the signature MUST be the last param of the query
-	 * actually there could be more params in the URL; but they wouldn't be
-	 * covered by the signature */
-
-	str_split = g_strsplit(query, "&Signature=", 0);
-	if (str_split[0] == NULL || str_split[1] == NULL) {
-		g_strfreev(str_split);
-		return LASSO_DS_ERROR_SIGNATURE_NOT_FOUND;
+	if (! saml_request_response) {
+		message(G_LOG_LEVEL_WARNING, "SAMLRequest or SAMLResponse missing in query");
+		ret = LASSO_PROFILE_ERROR_INVALID_QUERY;
+		goto done;
 	}
 
-	if (sender_public_key->value->id == xmlSecOpenSSLKeyDataRsaId) {
+	if (! b64_signature) {
+		ret = LASSO_DS_ERROR_SIGNATURE_NOT_FOUND;
+		goto done;
+	}
+	/* build the signed query */
+	if (relaystate) {
+		signed_query = g_strconcat(saml_request_response, "&", relaystate, "&", sig_alg, NULL);
 	} else {
-		/* no key; it will fail later */
+		signed_query = g_strconcat(saml_request_response, "&", sig_alg, NULL);
 	}
 
-	sig_alg = strstr(str_split[0], "&SigAlg=");
-	if (sig_alg == NULL) {
-		ret = critical_error(LASSO_DS_ERROR_INVALID_SIGALG);
+	sig_alg = strchr(sig_alg, '=')+1;
+	if (! sig_alg) {
+		ret = LASSO_DS_ERROR_INVALID_SIGALG;
 		goto done;
 	}
-	sig_alg = strchr(sig_alg, '=')+1;
-
 	usig_alg = xmlURIUnescapeString(sig_alg, 0, NULL);
-	if (strcmp(usig_alg, (char*)xmlSecHrefRsaSha1) == 0) {
+	if (g_strcmp0(usig_alg, (char*)xmlSecHrefRsaSha1) == 0) {
 		if (sender_public_key->value->id != xmlSecOpenSSLKeyDataRsaId) {
 			ret = critical_error(LASSO_DS_ERROR_PUBLIC_KEY_LOAD_FAILED);
 			goto done;
@@ -730,7 +776,7 @@ lasso_saml2_query_verify_signature(const char *query, const xmlSecKey *sender_pu
 			goto done;
 		}
 		key_size = RSA_size(rsa);
-	} else if (strcmp(usig_alg, (char*)xmlSecHrefDsaSha1) == 0) {
+	} else if (g_strcmp0(usig_alg, (char*)xmlSecHrefDsaSha1) == 0) {
 		if (sender_public_key->value->id != xmlSecOpenSSLKeyDataDsaId) {
 			ret = critical_error(LASSO_DS_ERROR_PUBLIC_KEY_LOAD_FAILED);
 			goto done;
@@ -746,21 +792,18 @@ lasso_saml2_query_verify_signature(const char *query, const xmlSecKey *sender_pu
 		goto done;
 	}
 
-	/* insure there is only the signature in str_split[1] */
-	if (strchr(str_split[1], '&')) {
-		strchr(str_split[1], '&')[0] = 0;
-	}
-
 	/* get signature (unescape + base64 decode) */
 	signature = xmlMalloc(key_size+1);
-	b64_signature = (char*)xmlURIUnescapeString(str_split[1], 0, NULL);
+	xmlSecErrorsDefaultCallbackEnableOutput(FALSE);
 	if (b64_signature == NULL || xmlSecBase64Decode((xmlChar*)b64_signature, signature, key_size+1) < 0) {
+		xmlSecErrorsDefaultCallbackEnableOutput(TRUE);
 		ret = LASSO_DS_ERROR_INVALID_SIGNATURE;
 		goto done;
 	}
+	xmlSecErrorsDefaultCallbackEnableOutput(TRUE);
 
 	/* compute signature digest */
-	digest = lasso_sha1(str_split[0]);
+	digest = lasso_sha1(signed_query);
 	if (digest == NULL) {
 		ret = critical_error(LASSO_DS_ERROR_DIGEST_COMPUTE_FAILED);
 		goto done;
@@ -781,7 +824,9 @@ done:
 	xmlFree(signature);
 	xmlFree(digest);
 	xmlFree(usig_alg);
-	g_strfreev(str_split);
+	lasso_release(components);
+	lasso_release(query_copy);
+	lasso_release(signed_query);
 
 	return ret;
 }
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 68cdaea0..62ee098c 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -26,7 +26,7 @@ tests_LDADD = \
 	$(top_builddir)/lasso/liblasso.la \
 	$(LASSO_LIBS) \
 	$(CHECK_LIBS)
-tests_LDFLAGS = -rpath `cd $(top_builddir)/lasso/.libs/; pwd`
+tests_LDFLAGS = -rpath `cd $(top_builddir)/lasso/.libs/; pwd` -static
 
 tests2_SOURCES = tests2.c
 tests2_LDADD = \
diff --git a/tests/random_tests.c b/tests/random_tests.c
index b0d6a1ba..24dea4b0 100644
--- a/tests/random_tests.c
+++ b/tests/random_tests.c
@@ -277,6 +277,53 @@ START_TEST(test06_lib_statuscode)
 }
 END_TEST
 
+extern xmlSecKey* lasso_xmlsec_load_private_key_from_buffer(const char *buffer, size_t length, const char *password);
+
+extern int lasso_saml2_query_verify_signature(const char *query, const xmlSecKey *sender_public_key);
+
+
+START_TEST(test07_saml2_query_verify_signature)
+{
+	/* normal query as produces by Lasso */
+	const char query1[] = "SAMLRequest=fZHNasMwEIRfxeieWrYTtQjb4DgJBNqSNqWHXopw1kQgS6523Z%2B3r%2BxQSKDkOppvd2aVo%2BpML6uBjvYZPgZAir47Y1FODwUbvJVOoUZpVQcoqZH76uFepjdc9t6Ra5xhZ8h1QiGCJ%2B0si7argr0vxTLJ1guRilpU8%2FWtyKpNnaXrukoF32SCRa%2FgMfgLFvAAIQ6wtUjKUpB4wmc8nSX8hXOZ3Ml0%2FsaijfMNTIUK1iqDMGK7sFl%2Fwp9S5mNWOY3z5ZGol3GM%2FSLugNRBkcrjc0N%2ButJj6LNd7ZzRzc%2B4plN0ve6o6MOsnayyH6sggSUW7XfjsKdBGd1q8AX7JwOLKmPcV%2B1BUUhOfgAWl6dkl19W%2FgI%3D&RelayState=fake%5B%5D&SigAlg=http%3A%2F%2Fwww.w3.org%2F2000%2F09%2Fxmldsig%23rsa-sha1&Signature=wDxMSEPKhK%2FuU06cmL50oVx%2B7eP5%2FQirShQE%2BLv9pT3CrVwb6WBV1Tp9XS2VVJ2odLHogdA%2FE1XDW7BIRKYgkN8bXVlC2GybSYBhyn8bwAuyHs%2BnMW48LF%2FE5vFiZxbw8tMWUAktdvDuaXoZLhubX7UgV%2B%2BdRyjhckolpXTC9xuJdoHJUDF0vzzNm8xZs6LR7tjWUoz5CcjMJA3LVfWmpE5UjCyRmGbi9knGWHdY75CFtArD%2BNSkGeNx9xySrUlik6e57Zlodv4V9WBdeopAWskO58BA27GqTmnSLooeo%2FrtLxc1NZeuau11YxNzwl%2FvN8%2FQ5IsR3Xic8X1TaCCtwg%3D%3D";
+	/* SAMLRequest field was moved in the middle, Signature to the beginning and all & were
+	 * changed to ; */
+	const char query2[] = "Signature=wDxMSEPKhK%2FuU06cmL50oVx%2B7eP5%2FQirShQE%2BLv9pT3CrVwb6WBV1Tp9XS2VVJ2odLHogdA%2FE1XDW7BIRKYgkN8bXVlC2GybSYBhyn8bwAuyHs%2BnMW48LF%2FE5vFiZxbw8tMWUAktdvDuaXoZLhubX7UgV%2B%2BdRyjhckolpXTC9xuJdoHJUDF0vzzNm8xZs6LR7tjWUoz5CcjMJA3LVfWmpE5UjCyRmGbi9knGWHdY75CFtArD%2BNSkGeNx9xySrUlik6e57Zlodv4V9WBdeopAWskO58BA27GqTmnSLooeo%2FrtLxc1NZeuau11YxNzwl%2FvN8%2FQ5IsR3Xic8X1TaCCtwg%3D%3D;RelayState=fake%5B%5D;SAMLRequest=fZHNasMwEIRfxeieWrYTtQjb4DgJBNqSNqWHXopw1kQgS6523Z%2B3r%2BxQSKDkOppvd2aVo%2BpML6uBjvYZPgZAir47Y1FODwUbvJVOoUZpVQcoqZH76uFepjdc9t6Ra5xhZ8h1QiGCJ%2B0si7argr0vxTLJ1guRilpU8%2FWtyKpNnaXrukoF32SCRa%2FgMfgLFvAAIQ6wtUjKUpB4wmc8nSX8hXOZ3Ml0%2FsaijfMNTIUK1iqDMGK7sFl%2Fwp9S5mNWOY3z5ZGol3GM%2FSLugNRBkcrjc0N%2ButJj6LNd7ZzRzc%2B4plN0ve6o6MOsnayyH6sggSUW7XfjsKdBGd1q8AX7JwOLKmPcV%2B1BUUhOfgAWl6dkl19W%2FgI%3D;SigAlg=http%3A%2F%2Fwww.w3.org%2F2000%2F09%2Fxmldsig%23rsa-sha1";
+	const char query3[] = "RelayState=fake%5B%5D&SAMLRequest=fZHNasMwEIRfxeieWrYTtQjb4DgJBNqSNqWHXopw1kQgS6523Z%2B3r%2BxQSKDkOppvd2aVo%2BpML6uBjvYZPgZAir47Y1FODwUbvJVOoUZpVQcoqZH76uFepjdc9t6Ra5xhZ8h1QiGCJ%2B0si7argr0vxTLJ1guRilpU8%2FWtyKpNnaXrukoF32SCRa%2FgMfgLFvAAIQ6wtUjKUpB4wmc8nSX8hXOZ3Ml0%2FsaijfMNTIUK1iqDMGK7sFl%2Fwp9S5mNWOY3z5ZGol3GM%2FSLugNRBkcrjc0N%2ButJj6LNd7ZzRzc%2B4plN0ve6o6MOsnayyH6sggSUW7XfjsKdBGd1q8AX7JwOLKmPcV%2B1BUUhOfgAWl6dkl19W%2FgI%3D&SigAlg=http%3A%2F%2Fwww.w3.org%2F2000%2F09%2Fxmldsig%23rsa-sha1&Signature=wDxMSEPKhK%2FuU06cmL50oVx%2B7eP5%2FQirShQE%2BLv9pT3CrVwb6WBV1Tp9XS2VVJ2odLHogdA%2FE1XDW7BIRKYgkN8bXVlC2GybSYBhyn8bwAuyHs%2BnMW48LF%2FE5vFiZxbw8tMWUAktdvDuaXoZLhubX7UgV%2B%2BdRyjhckolpXTC9xuJdoHJUDF0vzzNm8xZs6LR7tjWUoz5CcjMJA3LVfWmpE5UjCyRmGbi9knGWHdY75CFtArD%2BNSkGeNx9xySrUlik6e57Zlodv4V9WBdeopAWskO58BA27GqTmnSLooeo%2FrtLxc1NZeuau11YxNzwl%2FvN8%2FQ5IsR3Xic8X1TacCtwg%3D%3D";
+	/* sp5-saml2 key */
+	const char pkey[] = "-----BEGIN CERTIFICATE-----\n\
+MIIDnjCCAoagAwIBAgIBATANBgkqhkiG9w0BAQUFADBUMQswCQYDVQQGEwJGUjEP\n\
+MA0GA1UECBMGRnJhbmNlMQ4wDAYDVQQHEwVQYXJpczETMBEGA1UEChMKRW50cm91\n\
+dmVydDEPMA0GA1UEAxMGRGFtaWVuMB4XDTA2MTAyNzA5MDc1NFoXDTExMTAyNjA5\n\
+MDc1NFowVDELMAkGA1UEBhMCRlIxDzANBgNVBAgTBkZyYW5jZTEOMAwGA1UEBxMF\n\
+UGFyaXMxEzARBgNVBAoTCkVudHJvdXZlcnQxDzANBgNVBAMTBkRhbWllbjCCASIw\n\
+DQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAM06Hx6VgHYR9wUf/tZVVTRkVWNq\n\
+h9x+PvHA2qH4OYMuqGs4Af6lU2YsZvnrmRdcFWv0+UkdAgXhReCWAZgtB1pd/W9m\n\
+6qDRldCCyysow6xPPKRz/pOTwRXm/fM0QGPeXzwzj34BXOIOuFu+n764vKn18d+u\n\
+uVAEzk1576pxTp4pQPzJfdNLrLeQ8vyCshoFU+MYJtp1UA+h2JoO0Y8oGvywbUxH\n\
+ioHN5PvnzObfAM4XaDQohmfxM9Uc7Wp4xKAc1nUq5hwBrHpjFMRSz6UCfMoJSGIi\n\
++3xJMkNCjL0XEw5NKVc5jRKkzSkN5j8KTM/k1jPPsDHPRYzbWWhnNtd6JlkCAwEA\n\
+AaN7MHkwCQYDVR0TBAIwADAsBglghkgBhvhCAQ0EHxYdT3BlblNTTCBHZW5lcmF0\n\
+ZWQgQ2VydGlmaWNhdGUwHQYDVR0OBBYEFP2WWMDShux3iF74+SoO1xf6qhqaMB8G\n\
+A1UdIwQYMBaAFGjl6TRXbQDHzSlZu+e8VeBaZMB5MA0GCSqGSIb3DQEBBQUAA4IB\n\
+AQAZ/imK7UMognXbs5RfSB8cMW6iNAI+JZqe9XWjvtmLfIIPbHM96o953SiFvrvQ\n\
+BZjGmmPMK3UH29cjzDx1R/RQaYTyMrHyTePLh3BMd5mpJ/9eeJCSxPzE2ECqWRUa\n\
+pkjukecFXqmRItwgTxSIUE9QkpzvuQRb268PwmgroE0mwtiREADnvTFkLkdiEMew\n\
+fiYxZfJJLPBqwlkw/7f1SyzXoPXnz5QbNwDmrHelga6rKSprYKb3pueqaIe8j/AP\n\
+NC1/bzp8cGOcJ88BD5+Ny6qgPVCrMLE5twQumJ12V3SvjGNtzFBvg2c/9S5OmVqR\n\
+LlTxKnCrWAXftSm1rNtewTsF\n\
+-----END CERTIFICATE-----";
+
+	xmlSecKeyPtr key = lasso_xmlsec_load_private_key_from_buffer(pkey, sizeof(pkey)-1, NULL);
+
+	fail_unless(key != NULL, "Cannot load public key");
+	fail_unless(lasso_saml2_query_verify_signature(query1, key) == 0, "Signature was not validated");
+	/* test reordering and semi-colon separator support */
+	fail_unless(lasso_saml2_query_verify_signature(query2, key) == 0, "Disordered signature was not validated");
+	fail_unless(lasso_saml2_query_verify_signature(query3, key) != 0, "Altered signature was validated");
+}
+END_TEST
+
 Suite*
 random_suite()
 {
@@ -298,6 +345,7 @@ random_suite()
 	tcase_add_test(tc_node, test04_node_new_from_dump);
 	tcase_add_test(tc_node, test05_xsi_type);
 	tcase_add_test(tc_node, test06_lib_statuscode);
+	tcase_add_test(tc_node, test07_saml2_query_verify_signature);
 
 	return s;
 }