From: Omar Polo <op@omarpolo.com>
Subject: Re: gotweb cfgi buffer length checks
To: Stefan Sperling <stsp@stsp.name>
Cc: gameoftrees@openbsd.org
Date: Wed, 4 Dec 2024 15:40:51 +0100

Hello,


sorry for the delay!


On 02/12/24 18:01, Stefan Sperling wrote:
> dump_fcgi_record() gets called for a length that is at least the size
> of the header, but the function also processes parts of the body. Add
> a length check to skip the body if the buffer is too short.
>
> ok?

Looks fine to me, but what about deferring the call to dump_fcgi_record() in fcgi_parse_record() by a couple of lines? Likewise, the other call can be just changed to dump_fcgi_record_header() since it's called on a just constructed header.

I think it's cleaner if dump_fcgi_record() is defined to work only on a fully-read fcgi record.

> M  gotwebd/fcgi.c     |  11+  3-
> M  gotwebd/gotwebd.h  |   0+  1-
>
> 2 files changed, 11 insertions(+), 4 deletions(-)
>
> commit - 8ec2386c77a195a74ef5fcaf6c5695e200dbae21
> commit + de4367eca3fd8fad9122290f92c24cc3a5039cdd
> blob - ecad0c59902279155f09c3e1229c8ba25759a7d5
> blob + c53504026e82a6a995676787cae2919d2846b5a7
> --- gotwebd/fcgi.c
> +++ gotwebd/fcgi.c
> @@ -45,6 +45,7 @@ void	 fcgi_parse_begin_request(uint8_t *, uint16_t, st
>  void	 fcgi_parse_params(uint8_t *, uint16_t, struct request *, uint16_t);
>  int	 fcgi_send_response(struct request *, int, const void *, size_t);
>  
> +void	 dump_fcgi_record(const char *, struct fcgi_record_header *, size_t);
>  void	 dump_fcgi_record_header(const char *, struct fcgi_record_header *);
>  void	 dump_fcgi_begin_request_body(const char *,
>  	    struct fcgi_begin_request_body *);
> @@ -119,7 +120,7 @@ fcgi_parse_record(uint8_t *buf, size_t n, struct reque
>  
>  	h = (struct fcgi_record_header*) buf;
>  
> -	dump_fcgi_record("", h);
> +	dump_fcgi_record("", h, n);
>  
>  	if (n < sizeof(struct fcgi_record_header) + ntohs(h->content_len)
>  	    + h->padding_len)
> @@ -306,7 +307,7 @@ send_response(struct request *c, int type, const uint8
>  	iov[2].iov_base = (void *)padding;
>  	iov[2].iov_len = header.padding_len;
>  
> -	dump_fcgi_record("resp ", &header);
> +	dump_fcgi_record("resp ", &header, sizeof(header));
>  
>  	/*
>  	 * XXX: add some simple write heuristics here
> @@ -411,10 +412,17 @@ fcgi_cleanup_request(struct request *c)
>  }
>  
>  void
> -dump_fcgi_record(const char *p, struct fcgi_record_header *h)
> +dump_fcgi_record(const char *p, struct fcgi_record_header *h, size_t avail)
>  {
> +	if (avail < sizeof(struct fcgi_record_header))
> +		return;
> +
>  	dump_fcgi_record_header(p, h);
>  
> +	avail -= sizeof(struct fcgi_record_header);
> +	if (avail < sizeof(struct fcgi_begin_request_body))
> +		return;
> +
>  	if (h->type == FCGI_BEGIN_REQUEST)
>  		dump_fcgi_begin_request_body(p,
>  		    (struct fcgi_begin_request_body *)(h + 1));
> blob - 59fef09083a448dfb1173969b2c5b5426af439a8
> blob + 07263753b899195d6adff82948378fd8a64c4c26
> --- gotwebd/gotwebd.h
> +++ gotwebd/gotwebd.h
> @@ -483,7 +483,6 @@ void fcgi_request(int, short, void *);
>  void fcgi_timeout(int, short, void *);
>  void fcgi_cleanup_request(struct request *);
>  void fcgi_create_end_record(struct request *);
> -void dump_fcgi_record(const char *, struct fcgi_record_header *);
>  int fcgi_write(void *, const void *, size_t);
>  
>  /* got_operations.c */
>