Header/Content-Length parsing isn't case insensitive causing empty bodies (Version: 2.5.x)

[mankins]

Issue Description

This is a work in progress bug report to raise awareness of a potential bug and have one place to talk about it if so.

There have been reports of some node operators having problems connecting to download the unl over https. It was suggested that we open up http version of the unl to mitigate this issue. In investigating this I began to look at the source code and found a possible reason that downloads of the unl might fail.

In reviewing the source code I found that the http client parses content-length in a case sensitive manner:

rippled/src/libxrpl/net/HTTPClient.cpp

Lines 385 to 386 in cf5f65b

	static boost::regex reSize{
	"\\`.*\\r\\nContent-Length:\\s+([0-9]+).*\\'"};

which could lead to a stuck unl download as the client thinks the body is zero:

rippled/src/libxrpl/net/HTTPClient.cpp

Lines 422 to 426 in cf5f65b

	if (responseSize == 0)
	{
	// no body wanted or available
	invokeComplete(ecResult, mStatus);
	}

Steps to Reproduce

#!/usr/bin/env bash                                                                                                                                                                                                                                 

# run multiple times, changing this value...
url="https://vl.ripple.com/" # result: Content-Length                                                                                                                                                                                              
#url="https://unl.xrplf.org/" # result: no content-length                                                                                                                                                                                           
#url="https://v1.xrplf.org/" # result: no content-length                                                                                                                                                                                            
#url="https://unl.xrpl.foundation/" # result: Content-Length                                                                                                                                                                                         

for i in {1..20}; do
  echo "--- Request $i ---"
  curl -s -D - -o /dev/null --http1.1 "$url" | grep -i content-length
done

I would also think you could turn on the logs and see this in action.

rippled log_level net trace

Expected Result

You can see that headers will vary in Capitalization. We should guard against this.

Actual Result

Different servers will return different capitalizations...

Comments

Just reading the source at the moment, so it's possible this isn't the code path that is causing these issues, but I wanted to raise awareness in case it's an issue. I'm skeptical of this finding being valid myself as this would mean that we're actually only using one UNL, the ripple version in practice (as it seems to default to Content-Length: vs content-length).

[a1q123456]

Yes, http headers are not case sensitive so content-length is perfectly legal and it's wrong that rippled doesn't recognise it.

[a1q123456]

PR: #5767

[mankins]

Also this function doesn't seem to really use the timeout. I think the mDeadline needs to be moved down outside of the catch?

 try
 {
     mDeadline.expires_after(mTimeout);
 }
 catch (boost::system::system_error const& e)
 {
     mShutdown = e.code();
     JLOG(j_.trace()) << "expires_after: " << mShutdown.message();
-    mDeadline.async_wait(std::bind(
-        &HTTPClientImp::handleDeadline,
-        shared_from_this(),
-        std::placeholders::_1));
 }
+
+// Always arm the deadline wait
+mDeadline.async_wait(std::bind(
+    &HTTPClientImp::handleDeadline,
+    shared_from_this(),
+    std::placeholders::_1));

[mankins]

This bug could also be the reason that this "workaround" is in place. I'd guess once this bug is fixed we should remove/tighten this:

rippled/src/xrpld/rpc/detail/RPCCall.cpp

Lines 1679 to 1686 in 6bf8338

	// Number of bytes to try to receive if no
	// Content-Length header received
	constexpr auto RPC_REPLY_MAX_BYTES = megabytes(256);
	using namespace std::chrono_literals;
	auto constexpr RPC_WEBHOOK_TIMEOUT = 30s;
	HTTPClient::request(

I'd think as it is now it could be used as a DOS vector if you somehow could get an endpoint to repeatedly read a 256 MB url. (unlikely, but still seems like it could be improved.)

[vvysokikh1]

Fixed in #5767

[a1q123456]

To investigate further