Bug 68905 - CONTENT_LENGTH omitted from POST requests
Summary: CONTENT_LENGTH omitted from POST requests
Alias: None
Product: Apache httpd-2
Classification: Unclassified
Component: mod_cgi (show other bugs)
Version: 2.5-HEAD
Hardware: PC Linux
: P2 normal (vote)
Target Milestone: ---
Assignee: Apache HTTPD Bugs Mailing List
Depends on:
Reported: 2024-04-16 23:17 UTC by Richard Hipp
Modified: 2024-04-17 15:21 UTC (History)
0 users

Fix Fossil's HTTP protocol handling (1.82 KB, patch)
2024-04-17 12:18 UTC, Yann Ylavic
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Richard Hipp 2024-04-16 23:17:29 UTC
I am the project lead for SQLite and Fossil.  I have not verified this report personally.  The information here is gleaned from a thread on the Fossil Forum:  <https://fossil-scm.org/forum/forumpost/12ac403fd29cfc89>

Fossil is a version-control system, used by SQLite and many other projects.  Fossil includes a web-interface that can be run using CGI.  There are thousands of project teams using Fossil, and many of them run Fossil underneath Apache using mod_cgi.

We have multiple reports from the field that after a recent Apache security update, Fossil has stopped working.  We have traced the problem to a missing CONTENT_LENGTH meta-variable.  In other words, it appears (as best as we can determine so far) that Apache has stopped setting the CONTENT_LENGTH environment variable for CGI requests that have content - such as POST requests.

According to RFC 3875, "The server MUST set this meta-variable if and only if the request is accompanied by a message-body entity."  Indeed, there is no way for Fossil to discover the content length on its own if the CONTENT_LENGTH environment variable is missing.  Fossil has to assume that CONTENT_LENGTH is zero, but that causes POST content to go missing.
Comment 1 Eric Covener 2024-04-16 23:58:39 UTC
The thread seems to be about responses from a CGI rather than requests.

I owe an update to https://httpd.apache.org/docs/2.4/env.html to document the variable (that we hoped wouldn't be widely needed but we added so we could respond without a new build)

I think the gist of the final post of the thread is fair. If the CGI is trusted (untrusted users can't supply it) and can't be fooled into doing bad things, the variable is fine and very narrow.
Comment 2 Eric Covener 2024-04-17 00:25:24 UTC
I updated the doc and opened a bug for making the opt-in less necessary:
Comment 3 Joe Orton 2024-04-17 07:49:17 UTC
I think it is quite reasonable that the server has no compulsion to send Content-Length in HTTP/1.0 responses even if the the CGI script sends them, and relying on that seems highly dubious.

https://fossil-scm.org/home/file?name=src/http.c&ci=trunk is a naively written HTTP/1.0 client which does not follow https://datatracker.ietf.org/doc/html/rfc1945#section-7.2.2 (nearly 30 years old) in assuming the response body is always delimited by Content-Length
Comment 4 Yann Ylavic 2024-04-17 09:57:56 UTC
I agree, for HTTP/1.1 httpd would send "Transfer-Encoding: chunked" but not here because of the HTTP/1.0 request.

So the only option for httpd is either:
1. bufferize/spool the response to get the full Content-Length before sending the whole in a one go
2. forward the CGI provided Content-Length and verify that the actual body does not overflow it (or abort the connection)
3. trust the CGI provided Content-Length (which is SetEnv "ap_trust_cgilike_cl") and risk response splitting vulns.

And 3. is the simpler and probably the only worth the effort for HTTP/1.0 (IMHO).
Comment 5 Yann Ylavic 2024-04-17 10:16:31 UTC
Note: The response without C-L nor T-E provided by httpd when it cannot determine the body length is what's described in in https://www.rfc-editor.org/rfc/rfc9112.html#name-message-body-length up to bullet #8 (last resort..).
Comment 6 Richard Hipp 2024-04-17 11:31:29 UTC
Thank y'all for your time and analysis.  Perhaps I was wrong and it is the reply that omits Content-Length, not the request as I originally thought.  I see that the Fossil client does not deal well with a missing Content-Length on the reply - because I know that the CGI sent a Content-Length and it never occurred to me that the intervening server might suppress the Content-Length.  I'll fix that and see if it resolves the issue.

Sorry for the imprecision in this report - I'm not able to reproduce the problem locally and am having to go off of field reports.
Comment 7 Yann Ylavic 2024-04-17 12:18:37 UTC
Created attachment 39671 [details]
Fix Fossil's HTTP protocol handling

I think that Fossil's HTTP protocol handling needs tising here: https://fossil-scm.org/home/file?ci=trunk&name=src/http.c&ln=664
Something like this patch..
Comment 8 Yann Ylavic 2024-04-17 12:24:49 UTC
(sorry fat fingers => s/tising/fixing/)
Comment 9 Richard Hipp 2024-04-17 12:42:32 UTC
(In reply to Yann Ylavic from comment #7)
> I think that Fossil's HTTP protocol handling needs fixing here:
> https://fossil-scm.org/home/file?ci=trunk&name=src/http.c&ln=664

I agree.  Thanks for taking the time to look into this. I had already implemented the patch at <https://fossil-scm.org/home/info/dfefd069b6026eff> prior to me seeing your post.  The patch is on a branch.  We are testing it now to see if that resolves the issue.  I'll report back once I know more.
Comment 10 Richard Hipp 2024-04-17 13:02:25 UTC
Thanks again, everybody.

The problem was indeed in Fossil.  Since the Fossil CGI on the server side was always sending Content-Length in the reply, it was expecting to always get a Content-Length back on the client side.  It never occurred to me that the intervening web server would elide that header field.  Fossil has been updated at <https://fossil-scm.org/home/info/a8e33fb161f45b65> to resolve this inadequacy.
Comment 11 Yann Ylavic 2024-04-17 13:11:32 UTC
(In reply to Richard Hipp from comment #10)
> https://fossil-scm.org/home/info/a8e33fb161f45b65

FWIW, I think the correct check for the "server did not reply" case is "iLength<0 && !closeConnection" rather than "iHttpVersion<0", because responses with no Content-Length nor Transfer-Encoding is allowed only for HTTP/1.1 responses plus "Connection: close" or with HTTP/1.0 responses (where "Connection: close" is implicit if not specified).
An HTTP/1.1 response with no "Connection: close" defaults to keep-alive so it might not be wise to accept no C-L nor T-E for those.
Comment 12 Richard Hipp 2024-04-17 13:31:50 UTC
That is a reasonable suggestion.  Thanks.  There are other considerations in play - that same routine is also used to parse raw CGI replies in the case of a sync via SSH - and so for that reason I have implemented your suggestion slightly differently, but I think the end result is the same.  See the change at <https://fossil-scm.org/home/info/71919ad1b542832c>.
Comment 13 Yann Ylavic 2024-04-17 13:43:34 UTC
Maybe something more explicit like this:
diff --git a/src/http.c b/src/http.c
index 0460e04d1..c71fde995 100644
--- a/src/http.c
+++ b/src/http.c
@@ -676,6 +676,10 @@ int http_exchange(
       goto write_err;
+  if( iLength<0 && !closeConnection ){
+    fossil_warning("\"content-length\" missing from %d keep-alive reply", rc);
+    goto write_err;
+  }
   if( rc!=200 ){
     fossil_warning("\"location:\" missing from %d redirect reply", rc);
     goto write_err;

Otherwise in https://fossil-scm.org/home/info/71919ad1b542832c it's possibly missing an "else" now?
Comment 14 Richard Hipp 2024-04-17 14:04:03 UTC
Implemented here: <https://fossil-scm.org/home/info/f4ffefe708793b03>
Comment 15 Yann Ylavic 2024-04-17 14:36:20 UTC
I don't how the "g.url.isSsh" + Status header protocol works but I get that if there is no content-length specified then there is also no body to expect, thus you can keep the connection alive?
Comment 16 Richard Hipp 2024-04-17 15:02:58 UTC
I think the connections are always "close".  But I'm not sure.  Better to safe that hit a problem later.
Comment 17 Yann Ylavic 2024-04-17 15:21:05 UTC
It seems that for ssh the connection is not closed finally (unless HTTP/1.0 or "connection: close"), so the safe way could be to "goto write_err;" for the new warning?
Reusing a connection without knowing if there is/was a body (and thus ignoring it) looks a bit hazardous to me, but as I said I don't know how this ssh+status protocol works..