From: Stefan Sperling Subject: Re: gotweb cfgi buffer length checks To: Omar Polo Cc: gameoftrees@openbsd.org Date: Thu, 5 Dec 2024 18:44:51 +0100 On Wed, Dec 04, 2024 at 03:40:51PM +0100, Omar Polo wrote: > 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. Oh, yes, that approach seems better. Is this ok? M gotwebd/fcgi.c | 6+ 5- M gotwebd/gotwebd.h | 0+ 1- 2 files changed, 6 insertions(+), 6 deletions(-) commit - 8c5906074ad5756611aa1977542e314d8e3662bb commit + 388441cdf262f31be12cb25a5dee475902c75f3c blob - ecad0c59902279155f09c3e1229c8ba25759a7d5 blob + 4c8f861d6c42639779642701c2392fb2bd30606d --- 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_request_body(const char *, struct fcgi_record_header *); 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,12 +120,14 @@ fcgi_parse_record(uint8_t *buf, size_t n, struct reque h = (struct fcgi_record_header*) buf; - dump_fcgi_record("", h); + dump_fcgi_record_header("", h); if (n < sizeof(struct fcgi_record_header) + ntohs(h->content_len) + h->padding_len) return 0; + dump_fcgi_request_body("", h); + if (h->version != 1) log_warn("wrong version"); @@ -306,7 +309,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_header("resp ", &header); /* * XXX: add some simple write heuristics here @@ -411,10 +414,8 @@ fcgi_cleanup_request(struct request *c) } void -dump_fcgi_record(const char *p, struct fcgi_record_header *h) +dump_fcgi_request_body(const char *p, struct fcgi_record_header *h) { - dump_fcgi_record_header(p, h); - 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 */