[OpenWrt-Devel] [PATCH 3/6] uclient-http: auth digest: Handle multiple possible memory allocation failures

[John Crispin]

Hi,

comments inline ...


On 18/02/18 04:36, Tobias Schramm wrote:
> Signed-off-by: Tobias Schramm <tobleminer at gmail.com>
> ---
>   uclient-http.c | 41 ++++++++++++++++++++++++++++++++---------
>   1 file changed, 32 insertions(+), 9 deletions(-)
>
> diff --git a/uclient-http.c b/uclient-http.c
> index 36e051b..2a3cf5d 100644
> --- a/uclient-http.c
> +++ b/uclient-http.c
> @@ -433,9 +433,10 @@ static void add_field(char **buf, int *ofs, int *len, const char *name, const ch
>   	*ofs = cur - *buf;
>   }
>   
> -static void
> +static int
>   uclient_http_add_auth_digest(struct uclient_http *uh)
>   {
> +	int err = 0;
>   	struct uclient_url *url = uh->uc.url;
>   	const char *realm = NULL, *opaque = NULL;
>   	const char *user, *password;
> @@ -454,14 +455,21 @@ uclient_http_add_auth_digest(struct uclient_http *uh)
>   	};
>   
>   	len = strlen(uh->auth_str) + 1;
> -	if (len > 512)
> -		return;
> +	if (len > 512) {
> +		err = -EINVAL;
> +		goto fail;
> +	}
>   
>   	buf = alloca(len);
> +	if(!buf) {
> +		err = -ENOMEM;
> +		goto fail;
> +	}
> +
>   	strcpy(buf, uh->auth_str);
>   
>   	/* skip auth type */
> -	strsep(&buf, " ");
> +	char *buf_orig = strsep(&buf, " ");

definitions should always be at the start of the function.

>   
>   	next = buf;
>   	while (*next) {
> @@ -495,8 +503,10 @@ uclient_http_add_auth_digest(struct uclient_http *uh)
>   		*dest = digest_unquote_sep(&next);
>   	}
>   
> -	if (!realm || !data.qop || !data.nonce)
> -		return;
> +	if (!realm || !data.qop || !data.nonce) {
> +		err = -EINVAL;
> +		goto fail_buf;
> +	}
>   
>   	sprintf(nc_str, "%08x", uh->nc++);
>   	get_cnonce(cnonce_str);
> @@ -510,10 +520,17 @@ uclient_http_add_auth_digest(struct uclient_http *uh)
>   		char *user_buf;
>   
>   		len = password - url->auth;
> -		if (len > 256)
> -			return;
> +		if (len > 256) {
> +			err = -EINVAL;
> +			goto fail_buf;
> +		}
>   
>   		user_buf = alloca(len + 1);
> +		if(!user_buf) {

missing space in front of the open bracket.

> +			err = -ENOMEM;
> +			goto fail_buf;
> +		}
> +
>   		strncpy(user_buf, url->auth, len);
>   		user_buf[len] = 0;
>   		user = user_buf;
> @@ -540,7 +557,13 @@ uclient_http_add_auth_digest(struct uclient_http *uh)
>   		add_field(&buf, &ofs, &len, "opaque", opaque);
>   
>   	ustream_printf(uh->us, "Authorization: Digest nc=%s, qop=%s%s\r\n", data.nc, data.qop, buf);
> -	free(buf);

will this not leak ?

     John

> +
> +	return 0;
> +
> +fail_buf:
> +	free(buf_orig);
> +fail:
> +	return err;
>   }
>   
>   static void
_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel