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

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?

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