user/dev discussion of public-inbox itself
 help / color / Atom feed
* [PATCH] http: fix RFC conformance w.r.t. message length
@ 2020-03-17  6:52 Eric Wong
  0 siblings, 0 replies; only message in thread
From: Eric Wong @ 2020-03-17  6:52 UTC (permalink / raw)
  To: meta

We need to favor "Transfer-Encoding: chunked" over the value of
the Content-Length header.  We should also reject bogus,
duplicate and/or unreasonable values for both these, since they
can trigger unexpected behavior when combined with other HTTP
parsers in proxies such as varnish, nginx, haproxy, etc...

See RFC 7230 (and RFC 2616) for more details:

	https://tools.ietf.org/html/rfc7230
	https://www.rfc-editor.org/errata_search.php?rfc=7230
---
 lib/PublicInbox/HTTP.pm | 35 ++++++++++++++++++++++++-----------
 t/httpd-corner.t        | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+), 11 deletions(-)

diff --git a/lib/PublicInbox/HTTP.pm b/lib/PublicInbox/HTTP.pm
index df328904..b73ce2d7 100644
--- a/lib/PublicInbox/HTTP.pm
+++ b/lib/PublicInbox/HTTP.pm
@@ -335,19 +335,31 @@ sub input_tmpfile ($) {
 
 sub input_prepare {
 	my ($self, $env) = @_;
-	my $input;
-	my $len = $env->{CONTENT_LENGTH};
-	if ($len) {
-		if ($len > $MAX_REQUEST_BUFFER) {
-			quit($self, 413);
-			return;
-		}
-		$input = input_tmpfile($self);
-	} elsif (env_chunked($env)) {
+	my ($input, $len);
+
+	# rfc 7230 3.3.2, 3.3.3,: favor Transfer-Encoding over Content-Length
+	my $hte = $env->{HTTP_TRANSFER_ENCODING};
+	if (defined $hte) {
+		# rfc7230 3.3.3, point 3 says only chunked is accepted
+		# as the final encoding.  Since neither public-inbox-httpd,
+		# git-http-backend, or our WWW-related code uses "gzip",
+		# "deflate" or "compress" as the Transfer-Encoding, we'll
+		# reject them:
+		return quit($self, 400) if $hte !~ /\Achunked\z/i;
+
 		$len = CHUNK_START;
 		$input = input_tmpfile($self);
 	} else {
-		$input = $null_io;
+		$len = $env->{CONTENT_LENGTH};
+		if (defined $len) {
+			# rfc7230 3.3.3.4
+			return quit($self, 400) if $len !~ /\A[0-9]+\z/;
+
+			return quit($self, 413) if $len > $MAX_REQUEST_BUFFER;
+			$input = $len ? input_tmpfile($self) : $null_io;
+		} else {
+			$input = $null_io;
+		}
 	}
 
 	# TODO: expire idle clients on ENFILE / EMFILE
@@ -358,7 +370,7 @@ sub input_prepare {
 	$self->{input_left} = $len || 0;
 }
 
-sub env_chunked { ($_[0]->{HTTP_TRANSFER_ENCODING} || '') =~ /\bchunked\b/i }
+sub env_chunked { ($_[0]->{HTTP_TRANSFER_ENCODING} // '') =~ /\Achunked\z/i }
 
 sub err ($$) {
 	eval { $_[0]->{httpd}->{env}->{'psgi.errors'}->print($_[1]."\n") };
@@ -451,6 +463,7 @@ sub quit {
 	my $h = "HTTP/1.1 $status " . status_message($status) . "\r\n\r\n";
 	$self->write(\$h);
 	$self->close;
+	undef; # input_prepare expects this
 }
 
 sub close {
diff --git a/t/httpd-corner.t b/t/httpd-corner.t
index cb813897..c99e5ec7 100644
--- a/t/httpd-corner.t
+++ b/t/httpd-corner.t
@@ -154,6 +154,38 @@ SKIP: {
 	like($head, qr/\b413\b/, 'got 413 response');
 }
 
+{
+	my $conn = conn_for($sock, '1.1 Transfer-Encoding bogus');
+	$conn->write("PUT /sha1 HTTP/1.1\r\nTransfer-Encoding: bogus\r\n\r\n");
+	$conn->read(my $buf, 4096);
+	like($buf, qr!\AHTTP/1\.[0-9] 400 !, 'got 400 response on bogus TE');
+}
+{
+	my $conn = conn_for($sock, '1.1 Content-Length bogus');
+	$conn->write("PUT /sha1 HTTP/1.1\r\nContent-Length: 3.3\r\n\r\n");
+	$conn->read(my $buf, 4096);
+	like($buf, qr!\AHTTP/1\.[0-9] 400 !, 'got 400 response on bad length');
+}
+
+{
+	my $req = "PUT /sha1 HTTP/1.1\r\nContent-Length: 3\r\n" .
+			"Content-Length: 3\r\n\r\n";
+	# this is stricter than it needs to be.  Due to the way
+	# Plack::HTTPParser, PSGI specs, and how hash tables work in common
+	# languages; it's not possible to tell the difference between folded
+	# and intentionally bad commas (e.g. "Content-Length: 3, 3")
+	if (0) {
+		require Plack::HTTPParser; # XS or pure Perl
+		require Data::Dumper;
+		Plack::HTTPParser::parse_http_request($req, my $env = {});
+		diag Data::Dumper::Dumper($env); # "Content-Length: 3, 3"
+	}
+	my $conn = conn_for($sock, '1.1 Content-Length dupe');
+	$conn->write($req);
+	$conn->read(my $buf, 4096);
+	like($buf, qr!\AHTTP/1\.[0-9] 400 !, 'got 400 response on dupe length');
+}
+
 {
 	my $conn = conn_for($sock, 'chunk with pipeline');
 	my $n = 10;

^ permalink raw reply	[flat|nested] only message in thread

only message in thread, back to index

Thread overview: (only message) (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-17  6:52 [PATCH] http: fix RFC conformance w.r.t. message length Eric Wong

user/dev discussion of public-inbox itself

Archives are clonable:
	git clone --mirror http://public-inbox.org/meta
	git clone --mirror http://czquwvybam4bgbro.onion/meta
	git clone --mirror http://hjrcffqmbrq6wope.onion/meta
	git clone --mirror http://ou63pmih66umazou.onion/meta

Example config snippet for mirrors

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.mail.public-inbox.meta
	nntp://ou63pmih66umazou.onion/inbox.comp.mail.public-inbox.meta
	nntp://czquwvybam4bgbro.onion/inbox.comp.mail.public-inbox.meta
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.mail.public-inbox.meta
	nntp://news.gmane.io/gmane.mail.public-inbox.general

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git