On Wed, Jul 13, 2016 at 07:47:04PM -0700, Kevin J. McCarthy wrote: > On Wed, Jul 13, 2016 at 08:05:03PM -0500, Will Fiveash wrote: > > On Tue, Jul 12, 2016 at 07:02:01PM -0700, Kevin J. McCarthy wrote: > > > It looks like the SASL password is leaked, but it's not clear to me how > > > we're supposed to free that. I saw one of the example clients used a > > > static variable and reused it for each authentication. I'm reluctant to > > > make that change though: I'd rather leak the memory than have an > > > implementation try to free it and then we realloc the freed memory. > > > > What do you mean by "an implementation"? > > Perhaps Mutt only works with the Cyrus implementation of SASL, but I was > worried there might be another one with different behavior. On the other > hand I do see the documentation in Cyrus SASL say the rule is "if you > allocate it, you free it", so maybe I'm worrying about nothing.
If a different implementation of SASL is free'ing the secret provided by a SASL_CB_PASS callback function then it is broken. > > + static unsigned char passwd_buf[520]; > > I'd really rather we not cap the size like this (especially after I had > to do https://dev.mutt.org/hg/mutt/rev/b315c4d4ede7). If we're going to > fix this, I'd rather safe_realloc() it each time. Makes sense, I've attached a diff showing use of safe_realloc() that appears to work. > > /* *psecret = (sasl_secret_t*) safe_malloc (sizeof (sasl_secret_t) + > > len); */ > > /* (*psecret)->len = len; */ > > /* strcpy ((char*)(*psecret)->data, account->pass); *//* > > __STRCPY_CHECKED__ */ > > memcpy(((sasl_secret_t *) passwd_buf)->data, account->pass, (size_t) len); > > There is a len field, but I'd also still like to see the account->pass > nul-terminated. I see plugins/plugin_common.c doing that in the sasl > source (when they prompt, not when they call the callback), so it's > probably a good idea. I disagree. SASL does not expect the data field in sasl_secret_t to be treated as a string and adding a terminating '\0' is just complicating the code. I'm assuming that's because the secret data could in fact be any binary byte values including '\0'. -- Will Fiveash
diff --git a/mutt_sasl.c b/mutt_sasl.c --- a/mutt_sasl.c +++ b/mutt_sasl.c @@ -86,6 +86,8 @@ static int mutt_sasl_start (void); +static sasl_secret_t *secret_ptr; + /* callbacks */ static int mutt_sasl_cb_log (void* context, int priority, const char* message); static int mutt_sasl_cb_authname (void* context, int id, const char** result, @@ -445,9 +447,10 @@ len = strlen (account->pass); - *psecret = (sasl_secret_t*) safe_malloc (sizeof (sasl_secret_t) + len); - (*psecret)->len = len; - strcpy ((char*)(*psecret)->data, account->pass); /* __STRCPY_CHECKED__ */ + safe_realloc(&secret_ptr, sizeof (sasl_secret_t) + len); + memcpy((char *) secret_ptr->data, account->pass, (size_t) len); + secret_ptr->len = len; + *psecret = secret_ptr; return SASL_OK; }
