From 8b8fd22a168860c5034822472d1fb5745f8fa0f5 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Wed, 16 Jun 2021 10:18:30 +0200 Subject: [PATCH] Fix lasso_query_sign HMAC other than SHA1 (#54037) The switch clause was using SHA1 digests for all digest types when signing. This obviously breaks verifying the signatures if HMAC-SHAXXX is used and XXX is something else than 1. --- lasso/xml/tools.c | 39 +++++++++++++++++++++++++-------------- tests/login_tests_saml2.c | 6 +++--- 2 files changed, 28 insertions(+), 17 deletions(-) diff --git a/lasso/xml/tools.c b/lasso/xml/tools.c index 96d88a2c..290fd55f 100644 --- a/lasso/xml/tools.c +++ b/lasso/xml/tools.c @@ -594,22 +594,20 @@ lasso_query_sign(char *query, LassoSignatureContext context) sigret_size = DSA_size(dsa); break; case LASSO_SIGNATURE_METHOD_HMAC_SHA1: - case LASSO_SIGNATURE_METHOD_HMAC_SHA256: - case LASSO_SIGNATURE_METHOD_HMAC_SHA384: - case LASSO_SIGNATURE_METHOD_HMAC_SHA512: - if ((rc = lasso_get_hmac_key(key, (void**)&hmac_key, - &hmac_key_length))) { - message(G_LOG_LEVEL_CRITICAL, "Failed to get hmac key (%s)", lasso_strerror(rc)); - goto done; - } - g_assert(hmac_key); md = EVP_sha1(); sigret_size = EVP_MD_size(md); - /* key should be at least 128 bits long */ - if (hmac_key_length < 16) { - critical("HMAC key should be at least 128 bits long"); - goto done; - } + break; + case LASSO_SIGNATURE_METHOD_HMAC_SHA256: + md = EVP_sha256(); + sigret_size = EVP_MD_size(md); + break; + case LASSO_SIGNATURE_METHOD_HMAC_SHA384: + md = EVP_sha384(); + sigret_size = EVP_MD_size(md); + break; + case LASSO_SIGNATURE_METHOD_HMAC_SHA512: + md = EVP_sha512(); + sigret_size = EVP_MD_size(md); break; default: g_assert_not_reached(); @@ -645,6 +643,19 @@ lasso_query_sign(char *query, LassoSignatureContext context) case LASSO_SIGNATURE_METHOD_HMAC_SHA256: case LASSO_SIGNATURE_METHOD_HMAC_SHA384: case LASSO_SIGNATURE_METHOD_HMAC_SHA512: + if ((rc = lasso_get_hmac_key(key, (void**)&hmac_key, + &hmac_key_length))) { + message(G_LOG_LEVEL_CRITICAL, "Failed to get hmac key (%s)", lasso_strerror(rc)); + goto done; + } + g_assert(hmac_key); + + /* key should be at least 128 bits long */ + if (hmac_key_length < 16) { + critical("HMAC key should be at least 128 bits long"); + goto done; + } + HMAC(md, hmac_key, hmac_key_length, (unsigned char *)new_query, strlen(new_query), sigret, &siglen); status = 1; diff --git a/tests/login_tests_saml2.c b/tests/login_tests_saml2.c index e331c07a..e1d78b5b 100644 --- a/tests/login_tests_saml2.c +++ b/tests/login_tests_saml2.c @@ -981,7 +981,7 @@ sso_initiated_by_sp(LassoServer *idp_context, LassoServer *sp_context, SsoCallba lasso_release_gobject(sp_login_context); } -START_TEST(test07_sso_sp_with_hmac_sha1_signatures) +START_TEST(test07_sso_sp_with_hmac_sha256_signatures) { LassoServer *idp_context = NULL; LassoServer *sp_context = NULL; @@ -990,7 +990,7 @@ START_TEST(test07_sso_sp_with_hmac_sha1_signatures) /* Create the shared key */ key = lasso_key_new_for_signature_from_memory("xxxxxxxxxxxxxxxx", 16, - NULL, LASSO_SIGNATURE_METHOD_HMAC_SHA1, NULL); + NULL, LASSO_SIGNATURE_METHOD_HMAC_SHA256, NULL); check_true(LASSO_IS_KEY(key)); /* Create an IdP context for IdP initiated SSO with provider metadata 1 */ @@ -1640,7 +1640,7 @@ login_saml2_suite() tcase_add_test(tc_spSloSoap, test04_sso_then_slo_soap); tcase_add_test(tc_idpKeyRollover, test05_sso_idp_with_key_rollover); tcase_add_test(tc_spKeyRollover, test06_sso_sp_with_key_rollover); - tcase_add_test(tc_hmacSignature, test07_sso_sp_with_hmac_sha1_signatures); + tcase_add_test(tc_hmacSignature, test07_sso_sp_with_hmac_sha256_signatures); tcase_add_test(tc_spLogin, test08_test_authnrequest_flags); tcase_add_test(tc_ecp, test09_ecp); tcase_add_test(tc_ecp, test10_ecp);