From f648941f018cbbaa9f3bd095f1bbf1ef63f5c8b6 Mon Sep 17 00:00:00 2001 From: Benjamin Dauvergne Date: Wed, 10 Feb 2010 00:34:55 +0000 Subject: [PATCH] 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. --- lasso/xml/tools.c | 113 ++++++++++++++++++++++++++++++------------- tests/Makefile.am | 2 +- tests/random_tests.c | 48 ++++++++++++++++++ 3 files changed, 128 insertions(+), 35 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++; } - - 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; + 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++; } - - if (sender_public_key->value->id == xmlSecOpenSSLKeyDataRsaId) { - } else { - /* no key; it will fail later */ + /* 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 - sig_alg = strstr(str_split[0], "&SigAlg="); - if (sig_alg == NULL) { - ret = critical_error(LASSO_DS_ERROR_INVALID_SIGALG); + if (! saml_request_response) { + message(G_LOG_LEVEL_WARNING, "SAMLRequest or SAMLResponse missing in query"); + ret = LASSO_PROFILE_ERROR_INVALID_QUERY; goto done; } - sig_alg = strchr(sig_alg, '=')+1; + 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 { + signed_query = g_strconcat(saml_request_response, "&", sig_alg, NULL); + } + + sig_alg = strchr(sig_alg, '=')+1; + if (! sig_alg) { + ret = LASSO_DS_ERROR_INVALID_SIGALG; + goto done; + } 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; }