From 6724002732c1bedb4ae4217871d432972672747e Mon Sep 17 00:00:00 2001 From: Sebastien Buisson Date: Thu, 19 Dec 2019 23:30:11 +0900 Subject: [PATCH] LU-12992 gss: retry in case of short computed shared key Lustre uses OpenSSL's DH_compute_key() to compute shared secret key. There is around 1 chance out of 256 that the returned shared key is shorter than expected. https://www.qacafe.com/articles/router-vpn-implementation-pitfalls/ If the returned key is short by 1 or 2 bytes, we prepend it with 0s. Otherwise, we retry twice as it should finally be the expected length. Test-Parameters: envdefinitions=SHARED_KEY=true testlist=sanity,recovery-small Test-Parameters: envdefinitions=SHARED_KEY=true testlist=sanity,recovery-small Test-Parameters: envdefinitions=SHARED_KEY=true testlist=sanity,recovery-small Test-Parameters: envdefinitions=SHARED_KEY=true testlist=sanity,recovery-small Test-Parameters: envdefinitions=SHARED_KEY=true testlist=sanity,recovery-small Signed-off-by: Sebastien Buisson Change-Id: I519194ef52539ca9a6305120fbf00b60ca985b1f Reviewed-on: https://review.whamcloud.com/37064 Tested-by: jenkins Reviewed-by: Andreas Dilger Reviewed-by: Jeremy Filizetti Tested-by: Maloo Reviewed-by: Oleg Drokin --- lustre/utils/gss/lgss_sk_utils.c | 2 +- lustre/utils/gss/sk_utils.c | 24 ++++++++++++++++++++---- lustre/utils/gss/svcgssd_proc.c | 25 ++++++++++++++++++++++++- 3 files changed, 45 insertions(+), 6 deletions(-) diff --git a/lustre/utils/gss/lgss_sk_utils.c b/lustre/utils/gss/lgss_sk_utils.c index 04133f1..3d12921 100644 --- a/lustre/utils/gss/lgss_sk_utils.c +++ b/lustre/utils/gss/lgss_sk_utils.c @@ -176,7 +176,7 @@ static int lgss_sk_validate_cred(struct lgss_cred *cred, gss_buffer_desc *token, } rc = sk_compute_dh_key(skc, &bufs[SK_RESP_PUB_KEY]); - if (rc == GSS_S_DEFECTIVE_TOKEN) { + if (rc == GSS_S_BAD_QOP) { /* Defective token for short key means we need to retry * because there is a chance that the parameters generated * resulted in a key that is 1 byte short */ diff --git a/lustre/utils/gss/sk_utils.c b/lustre/utils/gss/sk_utils.c index a5c09df..d3cf5ff 100644 --- a/lustre/utils/gss/sk_utils.c +++ b/lustre/utils/gss/sk_utils.c @@ -1162,10 +1162,26 @@ uint32_t sk_compute_dh_key(struct sk_cred *skc, const gss_buffer_desc *pub_key) ERR_error_string(ERR_get_error(), NULL)); goto out_err; } else if (status < dh_shared->length) { - printerr(0, "DH_compute_key() returned a short key of %d " - "bytes, expected: %zu\n", status, dh_shared->length); - rc = GSS_S_DEFECTIVE_TOKEN; - goto out_err; + /* there is around 1 chance out of 256 that the returned + * shared key is shorter than expected + */ + if (status >= dh_shared->length - 2) { + int shift = dh_shared->length - status; + /* if the key is short by only 1 or 2 bytes, just + * prepend it with 0s + */ + memmove((void *)(dh_shared->value + shift), + dh_shared->value, status); + memset(dh_shared->value, 0, shift); + } else { + /* if the key is really too short, return GSS_S_BAD_QOP + * so that the caller can retry to generate + */ + printerr(0, "DH_compute_key() returned a short key of %d bytes, expected: %zu\n", + status, dh_shared->length); + rc = GSS_S_BAD_QOP; + goto out_err; + } } rc = GSS_S_COMPLETE; diff --git a/lustre/utils/gss/svcgssd_proc.c b/lustre/utils/gss/svcgssd_proc.c index f2e94fc..d3e1b04 100644 --- a/lustre/utils/gss/svcgssd_proc.c +++ b/lustre/utils/gss/svcgssd_proc.c @@ -364,6 +364,7 @@ int handle_sk(struct svc_nego_data *snd) uint32_t version; uint32_t flags; int i; + int attempts = 0; printerr(3, "Handling sk request\n"); memset(bufs, 0, sizeof(gss_buffer_desc) * SK_INIT_BUFFERS); @@ -458,12 +459,34 @@ int handle_sk(struct svc_nego_data *snd) goto cleanup_partial; } +redo: rc = sk_gen_params(skc); if (rc != GSS_S_COMPLETE) { printerr(0, "Failed to generate DH params for responder\n"); goto cleanup_partial; } - if (sk_compute_dh_key(skc, &remote_pub_key)) { + rc = sk_compute_dh_key(skc, &remote_pub_key); + if (rc == GSS_S_BAD_QOP && attempts < 2) { + /* GSS_S_BAD_QOP means the generated shared key was shorter + * than expected. Just retry twice before giving up. + */ + attempts++; + if (skc->sc_params) { + DH_free(skc->sc_params); + skc->sc_params = NULL; + } + if (skc->sc_pub_key.value) { + free(skc->sc_pub_key.value); + skc->sc_pub_key.value = NULL; + } + skc->sc_pub_key.length = 0; + if (skc->sc_dh_shared_key.value) { + free(skc->sc_dh_shared_key.value); + skc->sc_dh_shared_key.value = NULL; + } + skc->sc_dh_shared_key.length = 0; + goto redo; + } else if (rc != GSS_S_COMPLETE) { printerr(0, "Failed to compute session key from DH params\n"); goto cleanup_partial; } -- 1.8.3.1