From 2b615f22d0ce78ba28a6d758d6a2a5c8cb33e10a Mon Sep 17 00:00:00 2001 From: Jeremy Harris Date: Wed, 1 Jan 2020 15:19:52 +0000 Subject: [PATCH] GSASL: provide $autnN for scram option expansions --- doc/doc-docbook/spec.xfpt | 22 +++++-- doc/doc-txt/ChangeLog | 3 + src/src/auths/gsasl_exim.c | 116 ++++++++++++++++++------------------- test/confs/3820 | 4 +- 4 files changed, 78 insertions(+), 67 deletions(-) diff --git a/doc/doc-docbook/spec.xfpt b/doc/doc-docbook/spec.xfpt index 560b72066..4d02bdc32 100644 --- a/doc/doc-docbook/spec.xfpt +++ b/doc/doc-docbook/spec.xfpt @@ -27544,16 +27544,28 @@ This specifies the SASL realm that the server claims to be in. Some mechanisms will use this data. -.option server_scram_iter gsasl string&!! unset +.option server_scram_iter gsasl string&!! 4096 This option provides data for the SCRAM family of mechanisms. -&$auth1$& is not available at evaluation time. -(This may change, as we receive feedback on use) +.new +The &$auth1$&, &$auth2$& and &$auth3$& variables are available for expansion. + +The result of expansion should be a decimal number, +and represents both a lower-bound on the security, and +a compute cost factor imposed on the client +(if it does not cache results, or the server changes +either the iteration count or the salt). +A minimum value of 4096 is required by the standards +for all current CRAM mechanism variants. +.wen .option server_scram_salt gsasl string&!! unset This option provides data for the SCRAM family of mechanisms. -&$auth1$& is not available at evaluation time. -(This may change, as we receive feedback on use) +.new +The &$auth1$&, &$auth2$& and &$auth3$& variables are available for expansion. +If unset or empty after expansion the library will provides a value for the +protocol conversation. +.wen .option server_service gsasl string &`smtp`& diff --git a/doc/doc-txt/ChangeLog b/doc/doc-txt/ChangeLog index 491ff5208..e1e1e3bf0 100644 --- a/doc/doc-txt/ChangeLog +++ b/doc/doc-txt/ChangeLog @@ -74,6 +74,9 @@ JH/17 Logging: when the deliver_time selector ise set, include the DT= field on delivery deferred (==) and failed (**) lines (if a delivery was attemtped). Previously it was only on completion (=>) lines. +JH/18 Authentication: the gsasl driver not provides the $authN variables in time + for the expansion of the server_scram_iter and server_scram_salt options. + Exim version 4.93 ----------------- diff --git a/src/src/auths/gsasl_exim.c b/src/src/auths/gsasl_exim.c index d1640f10b..80ae354f9 100644 --- a/src/src/auths/gsasl_exim.c +++ b/src/src/auths/gsasl_exim.c @@ -500,6 +500,36 @@ switch (exim_rc) return GSASL_AUTHENTICATION_ERROR; } + +static void +set_exim_authvar_from_prop(Gsasl_session * sctx, Gsasl_property prop) +{ +uschar * propval = US gsasl_property_fast(sctx, prop); +int i = expand_nmax, j = i + 1; +propval = propval ? string_copy(propval) : US""; +auth_vars[i] = expand_nstring[j] = propval; +expand_nlength[j] = Ustrlen(propval); +expand_nmax = j; +} + +static void +set_exim_authvars_from_a_az_r_props(Gsasl_session * sctx) +{ +if (expand_nmax > 0 ) return; + +/* Asking for GSASL_AUTHZID calls back into us if we use +gsasl_property_get(), thus the use of gsasl_property_fast(). +Do we really want to hardcode limits per mechanism? What happens when +a new mechanism is added to the library. It *shouldn't* result in us +needing to add more glue, since avoiding that is a large part of the +point of SASL. */ + +set_exim_authvar_from_prop(sctx, GSASL_AUTHID); +set_exim_authvar_from_prop(sctx, GSASL_AUTHZID); +set_exim_authvar_from_prop(sctx, GSASL_REALM); +} + + static int server_callback(Gsasl *ctx, Gsasl_session *sctx, Gsasl_property prop, auth_instance *ablock) @@ -522,15 +552,9 @@ switch (prop) case GSASL_VALIDATE_SIMPLE: HDEBUG(D_auth) debug_printf(" VALIDATE_SIMPLE\n"); /* GSASL_AUTHID, GSASL_AUTHZID, and GSASL_PASSWORD */ - propval = US gsasl_property_fast(sctx, GSASL_AUTHID); - auth_vars[0] = expand_nstring[1] = propval ? string_copy(propval) : US""; - propval = US gsasl_property_fast(sctx, GSASL_AUTHZID); - auth_vars[1] = expand_nstring[2] = propval ? string_copy(propval) : US""; - propval = US gsasl_property_fast(sctx, GSASL_PASSWORD); - auth_vars[2] = expand_nstring[3] = propval ? string_copy(propval) : US""; - expand_nmax = 3; - for (int i = 1; i <= 3; ++i) - expand_nlength[i] = Ustrlen(expand_nstring[i]); + set_exim_authvar_from_prop(sctx, GSASL_AUTHID); + set_exim_authvar_from_prop(sctx, GSASL_AUTHZID); + set_exim_authvar_from_prop(sctx, GSASL_PASSWORD); cbrc = condition_check(ablock, US"server_condition", ablock->server_condition); checked_server_condition = TRUE; @@ -544,12 +568,7 @@ switch (prop) cbrc = GSASL_AUTHENTICATION_ERROR; break; } - propval = US gsasl_property_fast(sctx, GSASL_AUTHZID); - - /* We always set $auth1, even if only to empty string. */ - auth_vars[0] = expand_nstring[1] = propval ? string_copy(propval) : US""; - expand_nlength[1] = Ustrlen(expand_nstring[1]); - expand_nmax = 1; + set_exim_authvar_from_prop(sctx, GSASL_AUTHZID); cbrc = condition_check(ablock, US"server_condition (EXTERNAL)", ablock->server_condition); @@ -564,13 +583,7 @@ switch (prop) cbrc = GSASL_AUTHENTICATION_ERROR; break; } - propval = US gsasl_property_fast(sctx, GSASL_ANONYMOUS_TOKEN); - - /* We always set $auth1, even if only to empty string. */ - - auth_vars[0] = expand_nstring[1] = propval ? string_copy(propval) : US""; - expand_nlength[1] = Ustrlen(expand_nstring[1]); - expand_nmax = 1; + set_exim_authvar_from_prop(sctx, GSASL_ANONYMOUS_TOKEN); cbrc = condition_check(ablock, US"server_condition (ANONYMOUS)", ablock->server_condition); @@ -588,13 +601,8 @@ switch (prop) to the first release of Exim with this authenticator, they've been switched to match the ordering of GSASL_VALIDATE_SIMPLE. */ - propval = US gsasl_property_fast(sctx, GSASL_GSSAPI_DISPLAY_NAME); - auth_vars[0] = expand_nstring[1] = propval ? string_copy(propval) : US""; - propval = US gsasl_property_fast(sctx, GSASL_AUTHZID); - auth_vars[1] = expand_nstring[2] = propval ? string_copy(propval) : US""; - expand_nmax = 2; - for (int i = 1; i <= 2; ++i) - expand_nlength[i] = Ustrlen(expand_nstring[i]); + set_exim_authvar_from_prop(sctx, GSASL_GSSAPI_DISPLAY_NAME); + set_exim_authvar_from_prop(sctx, GSASL_AUTHZID); /* In this one case, it perhaps makes sense to default back open? But for consistency, let's just mandate server_condition here too. */ @@ -608,66 +616,54 @@ switch (prop) HDEBUG(D_auth) debug_printf(" SCRAM_ITER\n"); if (ob->server_scram_iter) { + set_exim_authvars_from_a_az_r_props(sctx); tmps = CS expand_string(ob->server_scram_iter); + HDEBUG(D_auth) debug_printf(" '%s'\n", tmps); gsasl_property_set(sctx, GSASL_SCRAM_ITER, tmps); cbrc = GSASL_OK; } + else + HDEBUG(D_auth) debug_printf(" option not set\n"); break; case GSASL_SCRAM_SALT: HDEBUG(D_auth) debug_printf(" SCRAM_SALT\n"); - if (ob->server_scram_iter) + if (ob->server_scram_salt) { + set_exim_authvars_from_a_az_r_props(sctx); tmps = CS expand_string(ob->server_scram_salt); - gsasl_property_set(sctx, GSASL_SCRAM_SALT, tmps); + HDEBUG(D_auth) debug_printf(" '%s'\n", tmps); + if (*tmps) + gsasl_property_set(sctx, GSASL_SCRAM_SALT, tmps); cbrc = GSASL_OK; } + else + HDEBUG(D_auth) debug_printf(" option not set\n"); break; case GSASL_PASSWORD: HDEBUG(D_auth) debug_printf(" PASSWORD\n"); - /* DIGEST-MD5: GSASL_AUTHID, GSASL_AUTHZID and GSASL_REALM + /* SCRAM-SHA-1: GSASL_AUTHID, GSASL_AUTHZID and GSASL_REALM + DIGEST-MD5: GSASL_AUTHID, GSASL_AUTHZID and GSASL_REALM CRAM-MD5: GSASL_AUTHID PLAIN: GSASL_AUTHID and GSASL_AUTHZID LOGIN: GSASL_AUTHID */ - if (ob->server_scram_iter) - { - tmps = CS expand_string(ob->server_scram_iter); - gsasl_property_set(sctx, GSASL_SCRAM_ITER, tmps); - } - if (ob->server_scram_salt) - { - tmps = CS expand_string(ob->server_scram_salt); - gsasl_property_set(sctx, GSASL_SCRAM_SALT, tmps); - } - - /* Asking for GSASL_AUTHZID calls back into us if we use - gsasl_property_get(), thus the use of gsasl_property_fast(). - Do we really want to hardcode limits per mechanism? What happens when - a new mechanism is added to the library. It *shouldn't* result in us - needing to add more glue, since avoiding that is a large part of the - point of SASL. */ - - propval = US gsasl_property_fast(sctx, GSASL_AUTHID); - auth_vars[0] = expand_nstring[1] = propval ? string_copy(propval) : US""; - propval = US gsasl_property_fast(sctx, GSASL_AUTHZID); - auth_vars[1] = expand_nstring[2] = propval ? string_copy(propval) : US""; - propval = US gsasl_property_fast(sctx, GSASL_REALM); - auth_vars[2] = expand_nstring[3] = propval ? string_copy(propval) : US""; - expand_nmax = 3; - for (int i = 1; i <= 3; ++i) - expand_nlength[i] = Ustrlen(expand_nstring[i]); + set_exim_authvars_from_a_az_r_props(sctx); if (!ob->server_password) + { + HDEBUG(D_auth) debug_printf("option not set\n"); break; + } if (!(tmps = CS expand_string(ob->server_password))) { - sasl_error_should_defer = f.expand_string_forcedfail ? FALSE : TRUE; + sasl_error_should_defer = !f.expand_string_forcedfail; HDEBUG(D_auth) debug_printf("server_password expansion failed, so " "can't tell GNU SASL library the password for %s\n", auth_vars[0]); return GSASL_AUTHENTICATION_ERROR; } + HDEBUG(D_auth) debug_printf(" set\n"); gsasl_property_set(sctx, GSASL_PASSWORD, tmps); /* This is inadequate; don't think Exim's store stacks are geared diff --git a/test/confs/3820 b/test/confs/3820 index b60e467a3..c80d4d414 100644 --- a/test/confs/3820 +++ b/test/confs/3820 @@ -77,8 +77,8 @@ sasl3: # a GSASL_SCRAM_SALTED_PASSWORD - but that is only documented for client mode. # unclear if the salt is given in binary or base64 to the library - server_scram_salt = QSXCR+Q6sek8bf92 - server_password = pencil + server_scram_salt = ${if eq {$auth1}{ph10} {QSXCR+Q6sek8bf92}} + server_password = ${if eq {$auth1}{ph10} {pencil}{unset_password}} server_condition = true server_set_id = $auth1 -- 2.30.2