Re: httpd: HTTP suffix-range requests omits file contents

On Sat Jul 18, 2026 at 07:27:07PM +0200, Kirill A. Korinsky wrote:
> On Sat, 18 Jul 2026 15:25:00 +0200,
> Rafael Sadowski <[email protected]> wrote:
> > 
> > On Sun Jul 12, 2026 at 03:15:15PM +0200, Kirill A. Korinsky wrote:
> > > On Sat, 04 Jul 2026 00:27:00 +0200,
> > > Andrew Kloet <[email protected]> wrote:
> > > > 
> > > > On June 18, 2026 7:44:31 PM UTC, Andrew Kloet <[email protected]> wrote:
> > > > >>Synopsis:      HTTP suffix-range requests omits file contents
> > > > >>Category:      user
> > > > >>Environment:
> > > > >	System      : OpenBSD 7.9
> > > > >	Details     : OpenBSD 7.9 (GENERIC.MP) #449: Wed May  6 13:17:25 MDT 2026
> > > > >			[email protected]:/usr/src/sys/arch/amd64/compile/GENERIC.MP
> > > > >
> > > > >	Architecture: OpenBSD.amd64
> > > > >	Machine     : amd64
> > > > >>Description:
> > > > >	There is a logic flaw inside httpd's parse_range_sepc when
> > > > >	processing HTTP Byte-Range Requests.
> > > > >
> > > > >	When a client issues a suffix-range request (Range: bytes=-num to
> > > > >	request the trailing num bytes of a resource) where the requested
> > > > >	count is larger than the target file size, an overflow guard caps
> > > > >	r->end to size - 1 prematurely.
> > > > >
> > > > >	Because this truncation happens before the evaluation block
> > > > >	calculates the starting byte offset (r->start = size - r->end), the
> > > > >	resulting offset math evaluates to 1 instead of 0. As a result,
> > > > >	httpd serves a 206 Partial Content response that completely omits
> > > > >	the very first byte (offset 0) of the target file.
> > > > >>How-To-Repeat:
> > > > >	# Create an 11-byte file
> > > > >	$ echo "0123456789" > /var/www/htdocs/test
> > > > >
> > > > >	# Send an HTTP request with a suffix range exceeding the file size.
> > > > >	# Note the response headers show an incorrect range and omit the
> > > > >	# leading character
> > > > >	$ curl -v -H "Range: bytes=-15" http://127.0.0.1/test
> > > > >	< HTTP/1.1 206 Partial Content
> > > > >	< Content-Range: bytes 1-10/11
> > > > >	< Content-Length: 10
> > > > >	123456789
> > > > >>Fix:
> > > > >	Decouple the suffix range processing from standard ranges within
> > > > >	parse_range_spec so that ceiling adjustments to r->end don't corrupt
> > > > >	the initial offset computation.
> > > > >
> > > > >--- a/usr.sbin/httpd/server_file.c
> > > > >+++ b/usr.sbin/httpd/server_file.c
> > > > >@@ -797,15 +797,18 @@ parse_range_spec(char *str, size_t size, struct range *r)
> > > > > 	if ((start_str_len == 0) && (end_str_len == 0))
> > > > > 		return (0);
> > > > > 
> > > > >-	if (end_str_len) {
> > > > >+	if (start_str_len == 0) {
> > > > > 		r->end = strtonum(end_str, 0, LLONG_MAX, &errstr);
> > > > >-		if (errstr)
> > > > >+		if (errstr || r->end == 0)
> > > > > 			return (0);
> > > > > 
> > > > >-		if ((size_t)r->end >= size)
> > > > >-			r->end = size - 1;
> > > > >-	} else
> > > > >+		if ((size_t)r->end > size)
> > > > >+			r->start = 0;
> > > > >+		else
> > > > >+			r->start = size - r->end;
> > > > > 		r->end = size - 1;
> > > > >+		return (1);
> > > > >+	}
> > > > > 
> > > > > 	if (start_str_len) {
> > > > > 		r->start = strtonum(start_str, 0, LLONG_MAX, &errstr);
> > > > >@@ -814,11 +817,17 @@ parse_range_spec(char *str, size_t size, struct range *r)
> > > > > 
> > > > > 		if ((size_t)r->start >= size)
> > > > > 			return (0);
> > > > >-	} else {
> > > > >-		r->start = size - r->end;
> > > > >-		r->end = size - 1;
> > > > > 	}
> > > > > 
> > > > >+	if (end_str_len) {
> > > > >+		r->end = strtonum(end_str, 0, LLONG_MAX, &errstr);
> > > > >+		if (errstr)
> > > > >+			return (0);
> > > > >+		if ((size_t)r->end >= size)
> > > > >+			r->end = size - 1;
> > > > >+	} else
> > > > >+		r->end = size - 1;
> > > > >+
> > > > > 	if (r->end < r->start)
> > > > > 		return (0);
> > > > 
> > > > bump
> > > > 
> > > 
> > > I think you have fixed only one edge case. After spending some time on this,
> > > I had this diff which fixed a few more cases plus addresses tests.
> > > 
> > > Here, I calculate suffix ranges before clamping end offsets so requests at
> > > least as large as the file return the complete representation. Reject zero
> > > length and malformed ranges, retain 64 bit sizes throughout range handling,
> > > and add regression coverage for boundaries and overflow.
> > > 
> > > Ok?
> > 
> > A thought on this. You could keep the strtonum and compare the errstr
> > strcmp(errstr, "too large"). I think that would make the code easier to
> > read and you safe the errno dance, but that's just my personal opinion.
> > 
> > It’s up to you to decide, OK rsadowski if you want go with this version.
> > 
> 
> You're right, that make that error dance simpler.
> 
> Ok?

OK rsadowski

> 
> Index: usr.sbin/httpd/httpd.h
> ===================================================================
> RCS file: /home/cvs/src/usr.sbin/httpd/httpd.h,v
> diff -u -p -r1.178 httpd.h
> --- usr.sbin/httpd/httpd.h	2 Jul 2026 04:59:16 -0000	1.178
> +++ usr.sbin/httpd/httpd.h	12 Jul 2026 12:19:37 -0000
> @@ -280,7 +280,7 @@ struct range_data {
>  
>  	/* For the Content headers in each part */
>  	struct media_type	*range_media;
> -	size_t			 range_total;
> +	off_t			 range_total;
>  };
>  
>  struct client {
> Index: usr.sbin/httpd/server_file.c
> ===================================================================
> RCS file: /home/cvs/src/usr.sbin/httpd/server_file.c,v
> diff -u -p -r1.82 server_file.c
> --- usr.sbin/httpd/server_file.c	21 Jun 2026 19:23:56 -0000	1.82
> +++ usr.sbin/httpd/server_file.c	18 Jul 2026 17:17:51 -0000
> @@ -52,8 +52,8 @@ int		 server_file_index(struct httpd *, 
>  int		 server_file_modified_since(struct http_descriptor *,
>  		    const struct timespec *);
>  int		 server_file_method(struct client *);
> -int		 parse_range_spec(char *, size_t, struct range *);
> -int		 parse_ranges(struct client *, char *, size_t);
> +int		 parse_range_spec(char *, off_t, struct range *);
> +int		 parse_ranges(struct client *, char *, off_t);
>  static int	 select_visible(const struct dirent *);
>  
>  int
> @@ -351,7 +351,8 @@ server_partial_file_request(struct httpd
>  	struct media_type	 multipart_media;
>  	struct range_data	*r = &clt->clt_ranges;
>  	struct range		*range;
> -	size_t			 content_length = 0, bufsiz;
> +	off_t			 content_length = 0, range_length;
> +	size_t			 bufsiz;
>  	int			 code = 500, i, nranges, ret;
>  	char			 content_range[64];
>  	const char		*errstr = NULL;
> @@ -396,12 +397,22 @@ server_partial_file_request(struct httpd
>  				goto abort;
>  
>  			/* Add data length */
> -			content_length += ret + range->end - range->start + 1;
> +			range_length = range->end - range->start + 1;
> +			if (content_length > LLONG_MAX - ret ||
> +			    range_length > LLONG_MAX - content_length - ret) {
> +				errno = EOVERFLOW;
> +				goto abort;
> +			}
> +			content_length += ret + range_length;
>  
>  		}
>  		if ((ret = snprintf(NULL, 0, "\r\n--%llu--\r\n",
>  		    clt->clt_boundary)) < 0)
>  			goto abort;
> +		if (content_length > LLONG_MAX - ret) {
> +			errno = EOVERFLOW;
> +			goto abort;
> +		}
>  		content_length += ret;
>  
>  		/* prepare multipart/byteranges media type */
> @@ -743,7 +754,7 @@ server_file_modified_since(struct http_d
>  }
>  
>  int
> -parse_ranges(struct client *clt, char *str, size_t file_sz)
> +parse_ranges(struct client *clt, char *str, off_t file_sz)
>  {
>  	int			 i = 0;
>  	char			*p, *q;
> @@ -783,9 +794,10 @@ parse_ranges(struct client *clt, char *s
>  }
>  
>  int
> -parse_range_spec(char *str, size_t size, struct range *r)
> +parse_range_spec(char *str, off_t size, struct range *r)
>  {
>  	size_t		 start_str_len, end_str_len;
> +	off_t		 value;
>  	char		*p, *start_str, *end_str;
>  	const char	*errstr;
>  
> @@ -802,27 +814,41 @@ parse_range_spec(char *str, size_t size,
>  	if ((start_str_len == 0) && (end_str_len == 0))
>  		return (0);
>  
> -	if (end_str_len) {
> -		r->end = strtonum(end_str, 0, LLONG_MAX, &errstr);
> -		if (errstr)
> -			return (0);
> +	if (size == 0)
> +		return (0);
>  
> -		if ((size_t)r->end >= size)
> -			r->end = size - 1;
> -	} else
> +	if (start_str_len == 0) {
> +		if (*end_str < '0' || *end_str > '9')
> +			return (0);
> +		value = strtonum(end_str, 0, size, &errstr);
> +		if (errstr != NULL && errno != ERANGE)
> +			return (0);
> +		if (errstr != NULL)
> +			r->start = 0;
> +		else if (value == 0)
> +			return (0);
> +		else
> +			r->start = size - value;
>  		r->end = size - 1;
> +		return (1);
> +	}
>  
> -	if (start_str_len) {
> -		r->start = strtonum(start_str, 0, LLONG_MAX, &errstr);
> -		if (errstr)
> -			return (0);
> +	if (*start_str < '0' || *start_str > '9')
> +		return (0);
> +	r->start = strtonum(start_str, 0, size - 1, &errstr);
> +	if (errstr != NULL)
> +		return (0);
>  
> -		if ((size_t)r->start >= size)
> +	if (end_str_len) {
> +		if (*end_str < '0' || *end_str > '9')
>  			return (0);
> -	} else {
> -		r->start = size - r->end;
> +		r->end = strtonum(end_str, 0, size - 1, &errstr);
> +		if (errstr != NULL && errno != ERANGE)
> +			return (0);
> +		if (errstr != NULL)
> +			r->end = size - 1;
> +	} else
>  		r->end = size - 1;
> -	}
>  
>  	if (r->end < r->start)
>  		return (0);
> Index: usr.sbin/httpd/server_http.c
> ===================================================================
> RCS file: /home/cvs/src/usr.sbin/httpd/server_http.c,v
> diff -u -p -r1.167 server_http.c
> --- usr.sbin/httpd/server_http.c	2 Jul 2026 04:59:50 -0000	1.167
> +++ usr.sbin/httpd/server_http.c	12 Jul 2026 13:04:29 -0000
> @@ -724,7 +724,7 @@ server_read_httprange(struct bufferevent
>  			if (server_bufferevent_printf(clt,
>  			    "\r\n--%llu\r\n"
>  			    "Content-Type: %s/%s\r\n"
> -			    "Content-Range: bytes %lld-%lld/%zu\r\n\r\n",
> +			    "Content-Range: bytes %lld-%lld/%lld\r\n\r\n",
>  			    clt->clt_boundary,
>  			    media->media_type, media->media_subtype,
>  			    range->start, range->end, r->range_total) == -1)
> 
> 
> -- 
> wbr, Kirill
>