[pacman-dev] [PATCH] curl_gethost() potential bug fixups

[Dave Reisner]

On Thu, Oct 13, 2011 at 12:56:25PM -0500, Dan McGee wrote:
> This is in the realm of "probably not going to happen", but if someone
> were to translate "disk" to a string longer than 256 characters, we
> would have a smashed/corrupted stack due to our unchecked strcpy() call.
> Rework the function to always length-check the value we copy into the
> hostname buffer, and do it with memcpy rather than the more cumbersome
> and unnecessary snprintf.
> 
> Finally, move the magic 256 value into a constant and pass it into the
> function which is going to get inlined anyway.
> 
> Signed-off-by: Dan McGee <dan at archlinux.org>

Ack. I'm embarassed for having written this.

d

> ---
>  lib/libalpm/dload.c |   24 ++++++++++++++----------
>  1 files changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c
> index 83060f9..cd2857c 100644
> --- a/lib/libalpm/dload.c
> +++ b/lib/libalpm/dload.c
> @@ -127,13 +127,14 @@ static int curl_progress(void *file, double dltotal, double dlnow,
>  	return 0;
>  }
>  
> -static int curl_gethost(const char *url, char *buffer)
> +static int curl_gethost(const char *url, char *buffer, size_t buf_len)
>  {
>  	size_t hostlen;
>  	char *p, *q;
>  
>  	if(strncmp(url, "file://", 7) == 0) {
> -		strcpy(buffer, _("disk"));
> +		p = _("disk");
> +		hostlen = strlen(p);
>  	} else {
>  		p = strstr(url, "//");
>  		if(!p) {
> @@ -154,13 +155,14 @@ static int curl_gethost(const char *url, char *buffer)
>  			hostlen -= q - p + 1;
>  			p = q + 1;
>  		}
> +	}
>  
> -		if(hostlen > 255) {
> -			/* buffer overflow imminent */
> -			return 1;
> -		}
> -		snprintf(buffer, hostlen + 1, "%s", p);
> +	if(hostlen > buf_len - 1) {
> +		/* buffer overflow imminent */
> +		return 1;
>  	}
> +	memcpy(buffer, p, hostlen);
> +	buffer[hostlen] = '\0';
>  
>  	return 0;
>  }
> @@ -310,14 +312,16 @@ static FILE *create_tempfile(struct dload_payload *payload, const char *localpat
>  	return fp;
>  }
>  
> +/* RFC1123 states applications should support this length */
> +#define HOSTNAME_SIZE 256
> +
>  static int curl_download_internal(struct dload_payload *payload,
>  		const char *localpath, char **final_file)
>  {
>  	int ret = -1;
>  	FILE *localf = NULL;
>  	char *effective_url;
> -	/* RFC1123 states applications should support this length */
> -	char hostname[256];
> +	char hostname[HOSTNAME_SIZE];
>  	char error_buffer[CURL_ERROR_SIZE] = {0};
>  	struct stat st;
>  	long timecond, respcode = 0, remote_time = -1;
> @@ -332,7 +336,7 @@ static int curl_download_internal(struct dload_payload *payload,
>  	if(!payload->remote_name) {
>  		payload->remote_name = strdup(get_filename(payload->fileurl));
>  	}
> -	if(!payload->remote_name || curl_gethost(payload->fileurl, hostname) != 0) {
> +	if(!payload->remote_name || curl_gethost(payload->fileurl, hostname, sizeof(hostname)) != 0) {
>  		_alpm_log(handle, ALPM_LOG_ERROR, _("url '%s' is invalid\n"), payload->fileurl);
>  		RET_ERR(handle, ALPM_ERR_SERVER_BAD_URL, -1);
>  	}
> -- 
> 1.7.7
> 
>