Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

curl sends invalid overlong HTTP requests when PUTting a file that is being appended to #11222

Closed
JustAnotherArchivist opened this issue May 29, 2023 · 0 comments
Labels

Comments

@JustAnotherArchivist
Copy link
Contributor

I did this

Uploading a file with curl sends a Content-Length header and then – potentially after a 100 response – sends the file contents in the body without regard to the size sent in the header. This results in a race condition if the file is being appended to at the time of the upload. The outcome varies by server interpretation of the data. Caddy, for example, appears to silently ignore the trailing data (= next request) on HTTP/1.1 requests, but it errors out with a 502 on HTTP/2. Other servers might not support keep-alive anyway and just read as much data as is reported in the header and then close the connections.

I expected the following

Either curl should only send the first N bytes according to the header value, or it should produce an error.

Of course, reading from a file that is being written to is always dangerous. In the case of appending specifically though, the only possible corruption is a truncated final line. And even if the data is rendered inconsistent by concurrent writes with seeks, I would still expect curl to not emit invalid HTTP requests. Also, if the file were to get truncated during the upload, I would expect curl to notice that and present an error. I haven't experienced or tested any of these other scenarios, only the appending case.

curl/libcurl version

curl 7.74.0 (x86_64-pc-linux-gnu) libcurl/7.74.0 OpenSSL/1.1.1n zlib/1.2.11 brotli/1.0.9 libidn2/2.3.0 libpsl/0.21.0 (+libidn2/2.3.0) libssh2/1.9.0 nghttp2/1.43.0 librtmp/2.3
Release-Date: 2020-12-09
Protocols: dict file ftp ftps gopher http https imap imaps ldap ldaps mqtt pop3 pop3s rtmp rtsp scp sftp smb smbs smtp smtps telnet tftp
Features: alt-svc AsynchDNS brotli GSS-API HTTP2 HTTPS-proxy IDN IPv6 Kerberos Largefile libz NTLM NTLM_WB PSL SPNEGO SSL TLS-SRP UnixSockets

operating system

Debian bullseye, Linux 9e00e3433692 4.19.0-23-amd64 #1 SMP Debian 4.19.269-1 (2022-12-20) x86_64 GNU/Linux

Reproducer

Since real HTTP servers' interpretation might vary, here's a simple/silly Python socket server that just waits out curl's expect100 timeout, reads 10 KiB from the socket, and responds with an invalid 502 response:

import socketserver
import time


class Handler(socketserver.BaseRequestHandler):
	def handle(self):
		time.sleep(2)
		data = self.request.recv(10240)
		print(data)
		self.request.sendall(b'HTTP/1.1 502 \r\n')


with socketserver.TCPServer(('127.0.0.1', 8000), Handler) as server:
	server.serve_forever()

Taking advantage of the default one-second expect100 timeout to trigger the race condition:

echo foo >file; (sleep 0.5; echo bar >>file)& curl -sv --upload-file file http://127.0.0.1:8000/

results in the following output from the Python server:

b'PUT /file HTTP/1.1\r\nHost: 127.0.0.1:8000\r\nUser-Agent: curl/7.74.0\r\nAccept: */*\r\nContent-Length: 4\r\nExpect: 100-continue\r\n\r\nfoo\nbar\n'

The extra bar line is included in the body.

Analysis

curl uses the infilesize for the header, which ultimately comes from st_size here. But then it passes -1 for the size in the Curl_setup_transfer call, which likely causes it to read/send until EOF. Perhaps the fix is as simple as passing the value from the header there. This should effectively send the part of the file that was present at fstat(2) time.

Workarounds

Piping the file to curl to trigger a chunked transfer should send everything up to whenever curl encounters EOF (which may or may not result in a truncated final line, of course). In our specific case, it turned out that switching from HTTP/2 to HTTP/1.1 also 'fixes' the problem because the server seems to ignore the trailing rubbish on that parser.

@bagder bagder added the HTTP label May 30, 2023
bagder added a commit that referenced this issue May 30, 2023
... and have curl trim the end when it reaches the expected total amount
of bytes instead of over-sending.

Reported-by: JustAnotherArchivist on github
Fixes #11222
bagder added a commit that referenced this issue May 30, 2023
Closes #11223
Fixes #11222
Reported-by: JustAnotherArchivist on github
bagder added a commit that referenced this issue May 30, 2023
... and have curl trim the end when it reaches the expected total amount
of bytes instead of over-sending.

Reported-by: JustAnotherArchivist on github
Fixes #11222
bagder added a commit that referenced this issue Jun 1, 2023
Closes #11223
Fixes #11222
Reported-by: JustAnotherArchivist on github
bagder added a commit that referenced this issue Jun 1, 2023
... and have curl trim the end when it reaches the expected total amount
of bytes instead of over-sending.

Reported-by: JustAnotherArchivist on github
Fixes #11222
@bagder bagder closed this as completed in 1f85420 Jun 1, 2023
bch pushed a commit to bch/curl that referenced this issue Jul 19, 2023
Closes curl#11223
Fixes curl#11222
Reported-by: JustAnotherArchivist on github
ptitSeb pushed a commit to wasix-org/curl that referenced this issue Sep 25, 2023
Closes curl#11223
Fixes curl#11222
Reported-by: JustAnotherArchivist on github
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging a pull request may close this issue.

2 participants