security review and patches for libpurple

[Dan Auerbach]

On 08/11/2011 08:38 PM, Paul Aurich wrote:
> And Ethan Blanton spoke on 08/11/2011 08:19 AM, saying:
>> * auth_cyrus.c
>>
>>    This is bogus like the first patch; len is strlen(pw).  If we can
>>    determine the size of sasl_secret_t.data, we need to use that.  I
>>    haven't looked into this yet.
> sasl_secret_t.data is malloc()d directly prior in the function, so it can
> never be an overflow -- the object is defined the same as the rejected
> patch for stringref.c.
>
>  From sasl.h:
> typedef struct sasl_secret {
>      unsigned long len;
>      unsigned char data[1];		/* variable sized */
> } sasl_secret_t;
>
>  From auth_cyrus.c:
> static int jabber_sasl_cb_secret(sasl_conn_t *conn, void *ctx, int id,
> sasl_secret_t **secret)
> {
> ...
> 	len = strlen(pw);
> 	/* Not an off-by-one because sasl_secret_t defines char data[1] */
> 	/* TODO: This can probably be moved to glib's allocator */
> 	js->sasl_secret = malloc(sizeof(sasl_secret_t) + len);
> 	if (!js->sasl_secret)
> 		return SASL_NOMEM;
>
> 	js->sasl_secret->len = len;
> 	strcpy((char*)js->sasl_secret->data, pw);
>
> 	*secret = js->sasl_secret;
> 	return SASL_OK;
> }
>
> (It does seem like there might now be a memory leak in here if Cyrus calls
> the secret_cb function multiple times [prior to fixing a race condition we
> had, it was a realloc of a static struct], but that's unrelated).
>
> ~Paul
>

Thanks for sending this out.  I agree with your analysis, and will 
reiterate my point about wanting to deprecate strcpy even in these 
cases, and my concern from the other patch. The memory leak seems like 
it should be looked into and worth a bug.