From 6a2c32cb705e73820c29e965394333f2874ba770 Mon Sep 17 00:00:00 2001 From: Jeremy Harris Date: Sun, 17 Nov 2019 16:32:06 +0000 Subject: [PATCH] tidying --- src/src/auths/cyrus_sasl.c | 94 +++++++++++++++------------------- src/src/auths/gsasl_exim.c | 100 +++++++++++++++++++------------------ 2 files changed, 90 insertions(+), 104 deletions(-) diff --git a/src/src/auths/cyrus_sasl.c b/src/src/auths/cyrus_sasl.c index 480010bab..4b4f45b94 100644 --- a/src/src/auths/cyrus_sasl.c +++ b/src/src/auths/cyrus_sasl.c @@ -89,16 +89,13 @@ to be set up. */ /* Auxiliary function, passed in data to sasl_server_init(). */ static int -mysasl_config(void *context, - const char *plugin_name, - const char *option, - const char **result, - unsigned int *len) +mysasl_config(void *context, const char *plugin_name, const char *option, + const char **result, unsigned int *len) { if (context && !strcmp(option, "mech_list")) { *result = context; - if (len != NULL) *len = strlen(*result); + if (len) *len = strlen(*result); return SASL_OK; } return SASL_FAIL; @@ -124,41 +121,37 @@ sasl_callback_t cbs[] = { {SASL_CB_LIST_END, NULL, NULL}}; /* default the mechanism to our "public name" */ -if (ob->server_mech == NULL) - ob->server_mech = string_copy(ablock->public_name); -expanded_hostname = expand_string(ob->server_hostname); -if (expanded_hostname == NULL) +if (!ob->server_mech) ob->server_mech = string_copy(ablock->public_name); + +if (!(expanded_hostname = expand_string(ob->server_hostname))) log_write(0, LOG_PANIC_DIE|LOG_CONFIG_FOR, "%s authenticator: " "couldn't expand server_hostname [%s]: %s", ablock->name, ob->server_hostname, expand_string_message); realm_expanded = NULL; -if (ob->server_realm != NULL) { - realm_expanded = CS expand_string(ob->server_realm); - if (realm_expanded == NULL) - log_write(0, LOG_PANIC_DIE|LOG_CONFIG_FOR, "%s authenticator: " - "couldn't expand server_realm [%s]: %s", - ablock->name, ob->server_realm, expand_string_message); -} +if ( ob->server_realm + && !(realm_expanded = CS expand_string(ob->server_realm))) + log_write(0, LOG_PANIC_DIE|LOG_CONFIG_FOR, "%s authenticator: " + "couldn't expand server_realm [%s]: %s", + ablock->name, ob->server_realm, expand_string_message); /* we're going to initialise the library to check that there is an - * authenticator of type whatever mechanism we're using - */ +authenticator of type whatever mechanism we're using */ cbs[0].proc = (int(*)(void)) &mysasl_config; cbs[0].context = ob->server_mech; -if ((rc = sasl_server_init(cbs, "exim")) != SASL_OK ) +if ((rc = sasl_server_init(cbs, "exim")) != SASL_OK) log_write(0, LOG_PANIC_DIE|LOG_CONFIG_FOR, "%s authenticator: " "couldn't initialise Cyrus SASL library.", ablock->name); if ((rc = sasl_server_new(CS ob->server_service, CS expanded_hostname, - realm_expanded, NULL, NULL, NULL, 0, &conn)) != SASL_OK ) + realm_expanded, NULL, NULL, NULL, 0, &conn)) != SASL_OK) log_write(0, LOG_PANIC_DIE|LOG_CONFIG_FOR, "%s authenticator: " "couldn't initialise Cyrus SASL server connection.", ablock->name); -if ((rc = sasl_listmech(conn, NULL, "", ":", "", (const char **)&list, &len, &i)) != SASL_OK ) +if ((rc = sasl_listmech(conn, NULL, "", ":", "", (const char **)&list, &len, &i)) != SASL_OK) log_write(0, LOG_PANIC_DIE|LOG_CONFIG_FOR, "%s authenticator: " "couldn't get Cyrus SASL mechanism list.", ablock->name); @@ -173,16 +166,16 @@ HDEBUG(D_auth) } /* the store_get / store_reset mechanism is hierarchical - * the hierarchy is stored for us behind our back. This point - * creates a hierarchy point for this function. - */ + the hierarchy is stored for us behind our back. This point + creates a hierarchy point for this function. */ + rs_point = store_mark(); /* loop until either we get to the end of the list, or we match the - * public name of this authenticator - */ -while ( ( buffer = string_nextinlist(&listptr, &i, NULL, 0) ) && - strcmpic(buffer,ob->server_mech) ); +public name of this authenticator */ + +while ( (buffer = string_nextinlist(&listptr, &i, NULL, 0)) + && strcmpic(buffer,ob->server_mech) ); if (!buffer) log_write(0, LOG_PANIC_DIE|LOG_CONFIG_FOR, "%s authenticator: " @@ -206,8 +199,7 @@ sasl_done(); /* For interface, see auths/README */ /* note, we don't care too much about memory allocation in this, because this is entirely - * within a shortlived child - */ +within a shortlived child */ int auth_cyrus_sasl_server(auth_instance *ablock, uschar *data) @@ -276,6 +268,9 @@ if (tls_in.cipher) } else HDEBUG(D_auth) debug_printf("Cyrus SASL set EXTERNAL SSF to %d\n", tls_in.bits); + + /*XXX Set channel-binding here with sasl_channel_binding_t / SASL_CHANNEL_BINDING + Unclear what the "name" element does though, ditto the "critical" flag. */ } else HDEBUG(D_auth) debug_printf("Cyrus SASL: no TLS, no EXTERNAL SSF set\n"); @@ -291,45 +286,34 @@ So the docs are too strict and we shouldn't worry about :: contractions. */ /* Set properties for remote and local host-ip;port */ for (int i = 0; i < 2; ++i) { - struct sockaddr_storage ss; - int (*query)(int, struct sockaddr *, socklen_t *); - int propnum, port; - const uschar *label; - uschar *address, *address_port; + int propnum; + const uschar * label; + uschar * address_port; const char *s_err; socklen_t sslen; if (i) { - query = &getpeername; propnum = SASL_IPREMOTEPORT; label = CUS"peer"; + address_port = string_sprintf("%s;%d", + sender_host_address, sender_host_port); } else { - query = &getsockname; propnum = SASL_IPLOCALPORT; label = CUS"local"; + address_port = string_sprintf("%s;%d", interface_address, interface_port); } - sslen = sizeof(ss); - if ((rc = query(fileno(smtp_in), (struct sockaddr *) &ss, &sslen)) < 0) - { - HDEBUG(D_auth) - debug_printf("Failed to get %s address information: %s\n", - label, strerror(errno)); - break; - } - - address = host_ntoa(-1, &ss, NULL, &port); - address_port = string_sprintf("%s;%d", address, port); - if ((rc = sasl_setprop(conn, propnum, address_port)) != SASL_OK) { - s_err = sasl_errdetail(conn); HDEBUG(D_auth) + { + s_err = sasl_errdetail(conn); debug_printf("Failed to set %s SASL property: [%d] %s\n", label, rc, s_err ? s_err : ""); + } break; } HDEBUG(D_auth) debug_printf("Cyrus SASL set %s hostport to: %s\n", @@ -353,7 +337,7 @@ for (rc = SASL_CONTINUE; rc == SASL_CONTINUE; ) if ((rc = auth_get_data(&input, out2, outlen)) != OK) { /* we couldn't get the data, so free up the library before - * returning whatever error we get */ + returning whatever error we get */ sasl_dispose(&conn); sasl_done(); return rc; @@ -422,9 +406,9 @@ for (rc = SASL_CONTINUE; rc == SASL_CONTINUE; ) case SASL_NOMECH: /* this is a temporary failure, because the mechanism is not - * available for this user. If it wasn't available at all, we - * shouldn't have got here in the first place... - */ + available for this user. If it wasn't available at all, we + shouldn't have got here in the first place... */ + HDEBUG(D_auth) debug_printf("Cyrus SASL temporary failure %d (%s)\n", rc, sasl_errstring(rc, NULL, NULL)); auth_defer_msg = diff --git a/src/src/auths/gsasl_exim.c b/src/src/auths/gsasl_exim.c index faf30bb8a..06c91ea3f 100644 --- a/src/src/auths/gsasl_exim.c +++ b/src/src/auths/gsasl_exim.c @@ -134,27 +134,29 @@ auth_gsasl_options_block *ob = the default for the mechanism name; we don't handle multiple mechanisms in one authenticator, but the same driver can be used multiple times. */ -if (ob->server_mech == NULL) +if (!ob->server_mech) ob->server_mech = string_copy(ablock->public_name); /* Can get multiple session contexts from one library context, so just initialise the once. */ -if (gsasl_ctx == NULL) { - rc = gsasl_init(&gsasl_ctx); - if (rc != GSASL_OK) { + +if (!gsasl_ctx) + { + if ((rc = gsasl_init(&gsasl_ctx)) != GSASL_OK) log_write(0, LOG_PANIC_DIE|LOG_CONFIG_FOR, "%s authenticator: " "couldn't initialise GNU SASL library: %s (%s)", ablock->name, gsasl_strerror_name(rc), gsasl_strerror(rc)); - } + gsasl_callback_set(gsasl_ctx, main_callback); -} + } /* We don't need this except to log it for debugging. */ -rc = gsasl_server_mechlist(gsasl_ctx, &p); -if (rc != GSASL_OK) + +if ((rc = gsasl_server_mechlist(gsasl_ctx, &p)) != GSASL_OK) log_write(0, LOG_PANIC_DIE|LOG_CONFIG_FOR, "%s authenticator: " "failed to retrieve list of mechanisms: %s (%s)", ablock->name, gsasl_strerror_name(rc), gsasl_strerror(rc)); + HDEBUG(D_auth) debug_printf("GNU SASL supports: %s\n", p); supported = gsasl_client_support_p(gsasl_ctx, CCS ob->server_mech); @@ -163,19 +165,21 @@ if (!supported) "GNU SASL does not support mechanism \"%s\"", ablock->name, ob->server_mech); -if ((ablock->server_condition == NULL) && - (streqic(ob->server_mech, US"EXTERNAL") || - streqic(ob->server_mech, US"ANONYMOUS") || - streqic(ob->server_mech, US"PLAIN") || - streqic(ob->server_mech, US"LOGIN"))) +if ( !ablock->server_condition + && ( streqic(ob->server_mech, US"EXTERNAL") + || streqic(ob->server_mech, US"ANONYMOUS") + || streqic(ob->server_mech, US"PLAIN") + || streqic(ob->server_mech, US"LOGIN") + ) ) log_write(0, LOG_PANIC_DIE|LOG_CONFIG_FOR, "%s authenticator: " "Need server_condition for %s mechanism", ablock->name, ob->server_mech); /* This does *not* scale to new SASL mechanisms. Need a better way to ask which properties will be needed. */ -if ((ob->server_realm == NULL) && - streqic(ob->server_mech, US"DIGEST-MD5")) + +if ( !ob->server_realm + && streqic(ob->server_mech, US"DIGEST-MD5")) log_write(0, LOG_PANIC_DIE|LOG_CONFIG_FOR, "%s authenticator: " "Need server_realm for %s mechanism", ablock->name, ob->server_mech); @@ -187,7 +191,8 @@ etc) it clearly is critical. So don't activate without server_condition, this might be relaxed in the future. */ -if (ablock->server_condition != NULL) ablock->server = TRUE; + +if (ablock->server_condition) ablock->server = TRUE; ablock->client = FALSE; } @@ -206,7 +211,7 @@ HDEBUG(D_auth) debug_printf("GNU SASL Callback entered, prop=%d (loop prop=%d)\n", prop, callback_loop); -if (cb_state == NULL) +if (!cb_state) { HDEBUG(D_auth) debug_printf(" not from our server/client processing.\n"); return GSASL_NO_CALLBACK; @@ -259,8 +264,7 @@ HDEBUG(D_auth) debug_printf("GNU SASL: initialising session for %s, mechanism %s.\n", ablock->name, ob->server_mech); -rc = gsasl_server_start(gsasl_ctx, CCS ob->server_mech, &sctx); -if (rc != GSASL_OK) +if ((rc = gsasl_server_start(gsasl_ctx, CCS ob->server_mech, &sctx)) != GSASL_OK) { auth_defer_msg = string_sprintf("GNU SASL: session start failure: %s (%s)", gsasl_strerror_name(rc), gsasl_strerror(rc)); @@ -286,6 +290,7 @@ if (ob->server_realm) } /* We don't support protection layers. */ gsasl_property_set(sctx, GSASL_QOPS, "qop-auth"); + #ifndef DISABLE_TLS if (tls_channelbinding_b64) { @@ -315,11 +320,9 @@ if (tls_channelbinding_b64) CCS tls_channelbinding_b64); } else - { HDEBUG(D_auth) debug_printf("Auth %s: Not enabling channel-binding (data available)\n", ablock->name); - } } else HDEBUG(D_auth) @@ -334,9 +337,7 @@ to_send = NULL; exim_error = exim_error_override = OK; do { - rc = gsasl_step64(sctx, received, &to_send); - - switch (rc) + switch (rc = gsasl_step64(sctx, received, &to_send)) { case GSASL_OK: if (!to_send) @@ -373,10 +374,8 @@ do { goto STOP_INTERACTION; } - if ((rc == GSASL_NEEDS_MORE) || - (to_send && *to_send)) - exim_error = - auth_get_no64_data((uschar **)&received, US to_send); + if ((rc == GSASL_NEEDS_MORE) || (to_send && *to_send)) + exim_error = auth_get_no64_data((uschar **)&received, US to_send); if (to_send) { @@ -419,29 +418,25 @@ return checked_server_condition ? OK : auth_check_serv_cond(ablock); static int condition_check(auth_instance *ablock, uschar *label, uschar *condition_string) { -int exim_rc; - -exim_rc = auth_check_some_cond(ablock, label, condition_string, FAIL); - -if (exim_rc == OK) - return GSASL_OK; -else if (exim_rc == DEFER) +int exim_rc = auth_check_some_cond(ablock, label, condition_string, FAIL); +switch (exim_rc) { - sasl_error_should_defer = TRUE; - return GSASL_AUTHENTICATION_ERROR; + case OK: return GSASL_OK; + case DEFER: sasl_error_should_defer = TRUE; + return GSASL_AUTHENTICATION_ERROR; + case FAIL: return GSASL_AUTHENTICATION_ERROR; + default: log_write(0, LOG_PANIC_DIE|LOG_CONFIG_FOR, "%s authenticator: " + "Unhandled return from checking %s: %d", + ablock->name, label, exim_rc); } -else if (exim_rc == FAIL) - return GSASL_AUTHENTICATION_ERROR; -log_write(0, LOG_PANIC_DIE|LOG_CONFIG_FOR, "%s authenticator: " - "Unhandled return from checking %s: %d", - ablock->name, label, exim_rc); /* NOTREACHED */ return GSASL_AUTHENTICATION_ERROR; } static int -server_callback(Gsasl *ctx, Gsasl_session *sctx, Gsasl_property prop, auth_instance *ablock) +server_callback(Gsasl *ctx, Gsasl_session *sctx, Gsasl_property prop, + auth_instance *ablock) { char *tmps; uschar *propval; @@ -475,13 +470,14 @@ switch (prop) break; case GSASL_VALIDATE_EXTERNAL: - if (ablock->server_condition == NULL) + if (!ablock->server_condition) { HDEBUG(D_auth) debug_printf("No server_condition supplied, to validate EXTERNAL.\n"); 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 ? propval : US""; expand_nlength[1] = Ustrlen(expand_nstring[1]); @@ -493,14 +489,16 @@ switch (prop) break; case GSASL_VALIDATE_ANONYMOUS: - if (ablock->server_condition == NULL) + if (!ablock->server_condition) { HDEBUG(D_auth) debug_printf("No server_condition supplied, to validate ANONYMOUS.\n"); 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 ? propval : US""; expand_nlength[1] = Ustrlen(expand_nstring[1]); expand_nmax = 1; @@ -516,10 +514,10 @@ switch (prop) by the SASL integration after authentication; protected against tampering (if the SASL mechanism supports that, which Kerberos does) but is unverified, same as normal for other mechanisms. - - First coding, we had these values swapped, but for consistency and prior + First coding, we had these values swapped, but for consistency and prior 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 ? propval : US""; propval = US gsasl_property_fast(sctx, GSASL_AUTHZID); @@ -530,6 +528,7 @@ switch (prop) /* In this one case, it perhaps makes sense to default back open? But for consistency, let's just mandate server_condition here too. */ + cbrc = condition_check(ablock, US"server_condition (GSSAPI family)", ablock->server_condition); checked_server_condition = TRUE; @@ -551,12 +550,14 @@ switch (prop) 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 ? propval : US""; propval = US gsasl_property_fast(sctx, GSASL_AUTHZID); @@ -567,8 +568,7 @@ switch (prop) for (int i = 1; i <= 3; ++i) expand_nlength[i] = Ustrlen(expand_nstring[i]); - tmps = CS expand_string(ob->server_password); - if (tmps == NULL) + if (!(tmps = CS expand_string(ob->server_password))) { sasl_error_should_defer = f.expand_string_forcedfail ? FALSE : TRUE; HDEBUG(D_auth) debug_printf("server_password expansion failed, so " @@ -576,9 +576,11 @@ switch (prop) return GSASL_AUTHENTICATION_ERROR; } gsasl_property_set(sctx, GSASL_PASSWORD, tmps); + /* This is inadequate; don't think Exim's store stacks are geared for memory wiping, so expanding strings will leave stuff laying around. But no need to compound the problem, so get rid of the one we can. */ + memset(tmps, '\0', strlen(tmps)); cbrc = GSASL_OK; break; -- 2.25.1