Produits Entr'ouvert » Lasso

Jakub Hrozek a écrit :

RHEL-9 is trying to get rid of any usages of SHA-1 across the codebase. Since I maintain lasso and mod_auth_mellon, I was looking at how are the different digests used in lasso and wanted to get your advice since I'm not all that familiar with lasso code.

Here's what I was thinking on a high-level:
- add a configure-time switch --enable-sha1. upstream it would default to enabled, RHEL-9 would disable it.
- based on the switch value, there would be a default signature method
- add a function lasso_allowed_signature_method along the lines of lasso_validate_signature_method that would return always true if sha1 signatures are allowed and if sha1 is not allowed in the distribution, return true if the signature uses something else than sha1
- this would be used in lasso_node_impl_init_from_xml() and in all the functions in lasso/xml/tools.c like lasso_query_sign() lasso_query_verify_helper() or _lasso_xmlsec_load_key_from_buffer() or lasso_xmlnode_add_saml2_signature_template()
- there's a bunch of SHA1 based digests that are not configurable all over the place under lasso/id-ff/. I was thinking about just mass converting them to the default signature method, so nothing would change unless you configure lasso with --disable-sha1

It looks fine to me.

- I don't know what to do about the WSF subtree at all. I don't know anything about WSF, but I was thinking doing the above there as well.

You can ignore it, it's more than deprecated, just use --disable-wsf in your builds; saying that I just tested it and some #ifdef in C code and Makefile are lacking for it to work immediately. Restoring --disable-wsg still seems simpler than trying to fix this dead code.

On Mon, May 17, 2021 at 10:04:04PM +0200, redmine@entrouvert.com wrote:

Issue #54037 has been updated by Benjamin Dauvergne.

Jakub Hrozek a écrit :

RHEL-9 is trying to get rid of any usages of SHA-1 across the codebase. Since I maintain lasso and mod_auth_mellon, I was looking at how are the different digests used in lasso and wanted to get your advice since I'm not all that familiar with lasso code.

Here's what I was thinking on a high-level:
- add a configure-time switch --enable-sha1. upstream it would default to enabled, RHEL-9 would disable it.
- based on the switch value, there would be a default signature method
- add a function lasso_allowed_signature_method along the lines of lasso_validate_signature_method that would return always true if sha1 signatures are allowed and if sha1 is not allowed in the distribution, return true if the signature uses something else than sha1
- this would be used in lasso_node_impl_init_from_xml() and in all the functions in lasso/xml/tools.c like lasso_query_sign() lasso_query_verify_helper() or _lasso_xmlsec_load_key_from_buffer() or lasso_xmlnode_add_saml2_signature_template()
- there's a bunch of SHA1 based digests that are not configurable all over the place under lasso/id-ff/. I was thinking about just mass converting them to the default signature method, so nothing would change unless you configure lasso with --disable-sha1

It looks fine to me.

Great, what do you think about the attached patch?

- I don't know what to do about the WSF subtree at all. I don't know anything about WSF, but I was thinking doing the above there as well.

You can ignore it, it's more than deprecated, just use --disable-wsf in your builds; saying that I just tested it and some #ifdef in C code and Makefile are lacking for it to work immediately. Restoring --disable-wsg still seems simpler than trying to fix this dead code.

Actually, turns out that lasso in Fedora and RHEL already disables wsf,
cool.

--x5j2zftuhtf6h6h7
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
filename="0001-Fix-lasso_query_sign-HMAC-other-than-SHA1.patch"

From 84ad9615223f84d56e486a26a3cf9906c4cb5a5d Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek@redhat.com>
Date: Wed, 16 Jun 2021 10:18:30 +0200
Subject: [PATCH 1/6] Fix lasso_query_sign HMAC other than SHA1

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 | 35 +++++++++++++++++++++------------
tests/login_tests_saml2.c | 6 ++---
2 files changed, 26 insertions(), 15 deletions(-)

diff --git a/lasso/xml/tools.c b/lasso/xml/tools.c
index 96d88a2c4..290fd55f2 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:
md = EVP_sha1();
+ sigret_size = EVP_MD_size(md);
+ 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:
- 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();
+ md = EVP_sha512();
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;
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;
+ }

HMACnew_query,
strlen(new_query), sigret, &siglen);
status = 1;
diff --git a/tests/login_tests_saml2.c b/tests/login_tests_saml2.c
index e331c07a7..e1d78b5b1 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);

btw if pulling git trees is easier, see https://github.com/jhrozek/lasso branch sha1