Change https://golang.org/cl/353792 mentions this issue: net/http/request: fix ParseHTTPVersion to correctly parse HTTP versions without minor

Sorry, but this isn't safe at this point. We'd explicitly documented a form of invalid input that we don't accept. People might be relying on that. We can't just start parsing it and treating it as valid.

/cc @neild

Sorry, but this isn't safe at this point. We'd explicitly documented a form of invalid input that we don't accept. People might be relying on that. We can't just start parsing it and treating it as valid.

The current code in the main branch parses values like HTTP/0.0, HTTP/00.00 and a lot of other combinations as valid. Considering this and the fact that multiple tools refer/use HTTP v2 as HTTP/2, I don't see why would it be a problem? If we are talking safety, my code is actually safer.

Permitting more inputs doesn't mean it's "safer".

https://datatracker.ietf.org/doc/html/rfc7230#appendix-B says:

   HTTP-version = HTTP-name "/" DIGIT "." DIGIT
...
   request-line = method SP request-target SP HTTP-version CRLF

That's the syntax that's supported.

Semantic rejections for things like HTTP/0.0 or HTTP/00.00 are handled at e.g. https://github.com/golang/go/blob/go1.17.1/src/net/http/server.go#L1037:

	// Reject HTTP/0.x, and all other HTTP/2+ requests (which
	// aren't encoded in ASCII anyway).
	return false

I see that it does incorrectly accept a request like:

crow5k:~ $ nc bradfitz.com 80
HEAD / HTTP/000001.000000000
Host: bradfitz.com

HTTP/1.0 200 OK
Accept-Ranges: bytes
Content-Length: 7486
Content-Type: text/html; charset=utf-8
Last-Modified: Wed, 04 Mar 2020 18:16:10 GMT
Date: Mon, 04 Oct 2021 15:26:52 GMT

So if you want something to fix, you could modify ParseHTTPVersion to strictly accept DIGIT "." DIGIT

Permitting more inputs doesn't mean it's "safer".

If you look at my code, you'll see that it is actually stricter, except the two scenarios that implicitly set the minor version to 0 in case of HTTP/2 and HTTP/3.

The reason why I added support for this, because it seems that requests made by multiple tools do support this: e.g. curl, firefox, burpsuite, and servers like cloudflare, google, github respond with HTTP/2 200 OK

Furthermore, quoting from the http2 FAQ page:

The Working Group decided to drop the minor version (“.0”) because it has caused a lot of confusion in HTTP/1.x.
In other words, the HTTP version only indicates wire compatibility, not feature sets or “marketing.”

At the same time looking at the RFC7540

That is, the connection preface starts with the string "PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n").

This indeed does have an implicit minor version, but this is related to the connection.

It seems like a mistake that http.ParseHTTPVersion exists as an exported API at all. What is it for?

Perhaps the best thing to do is deprecate it.

@neild
I am not using http.ParseHTTPVersion directly, but through http.ReadResponse. I am parsing dumped responses for passive analysis through an open-source vulnerability scanner.

@bradfitz I am willing to change my code, but first I'd like to make sure that it is not good as is, taking into account the points from my previous comment.

ReadResponse's documentation isn't explicit on this (probably because it predates HTTP/2), but it reads a response as returned by a HTTP/1 server on the wire.

It isn't possible to read an HTTP/2 response in a similar fashion. HTTP/2 interleaves multiple streams (request/response interactions) on a single TCP connection, reading an HTTP/2 streams requires bidirectional communication to negotiate flow control, and HTTP/2 request and response headers are encoded using a stateful compression algorithm.

The output of curl given above is not HTTP/2; it's curl's translation of an HTTP/2 stream into a human-readable form that mimics the familiar HTTP/1.x protocol.

Possibly it makes sense for http.ReadResponse to be able to parse this format? But if so, I'd want to know what that format is. Where is it specified? ("Whatever curl happens to output" is obviously not a specification.)

So if you want something to fix, you could modify ParseHTTPVersion to strictly accept DIGIT "." DIGIT

There is #46587 for which the fix https://go-review.googlesource.com/c/go/+/325874/ was proposed

The output of curl given above is not HTTP/2; it's curl's translation of an HTTP/2 stream into a human-readable form that mimics the familiar HTTP/1.x protocol.

Yes indeed, and this is the case for other tools as well.

I'd want to know what that format is

The closes match to an official specification probably would be/have been the HAR (HTTP archive format), but it wasn't officially published and has been abandoned since by the W3C. Although major browsers (Firefox, Chrome, Edge, Safari) and a number of other tools do support this and you can export HTTP2 requests in this format.
e.g.

"response": {
          "status": 200,
          "statusText": "OK",
          "httpVersion": "HTTP/2",
          "headers": [
            {
              "name": "server",
              "value": "AkamaiNetStorage"
            },
            {
              "name": "content-length",
              "value": "10747"
            },
            {
              "name": "content-type",
              "value": "text/html;charset=UTF-8"
            },            
            {
              "name": "accept-ch",
              "value": "DPR, Width, Viewport-Width, Downlink, Save-Data"
            },
            {
              "name": "X-Firefox-Spdy",
              "value": "h2"
            }
          ],
          "cookies": [],
          "content": {
            "mimeType": "text/html; charset=UTF-8",
            "size": 10747,
            "text": "<!DOCTYPE html>\r\n<html>\r\n<head>\r\n
                         <meta content=\"width=device-width,initial-scale=1\" name=\"viewport\">\r\n
                         <meta name=\"Keywords\" content=\"HTTP/2, HTTP/2.0, H2, Akamai, Cloud Computing, CDN, Mobile Experience, Content Delivery, Application Acceleration, Application Performance Management\">\r\n
                         <title>HTTP/2: the Future of the Internet | Akamai</title> ..."

Dumping HTTP2 requests with net/httpu/httputil.DumpResponse (although this dumps the HTTP version with a minor):

"DumpRequest returns the given request in its HTTP/1.x wire representation. It should only be used by servers to debug client requests. The returned representation is an approximation only; some details of the initial request are lost while parsing it into an http.Request. In particular, the order and case of header field names are lost. The order of values in multi-valued headers is kept intact. HTTP/2 requests are dumped in HTTP/1.x form, not in their original binary representations."

func TestDumpHttp2(t *testing.T) {
	resp, err := http.Get("http://http2.akamai.com")
	if err != nil {
		log.Fatal(err)
	}
	defer resp.Body.Close()

	dump, err := httputil.DumpResponse(resp, false)
	if err != nil {
		log.Fatal(err)
	}

	fmt.Printf("%q", dump)
}

Result:

HTTP/2.0 200 OK
Content-Length: 10747
Accept-Ch: DPR, Width, Viewport-Width, Downlink, Save-Data
Access-Control-Allow-Credentials: false
Access-Control-Allow-Headers: *
Access-Control-Allow-Methods: GET,HEAD,POST
Access-Control-Allow-Origin: *
Access-Control-Max-Age: 86400
Cache-Control: max-age=43200
Content-Type: text/html;charset=UTF-8
Date: Wed, 06 Oct 2021 09:42:11 GMT
Etag: \"2f2ae1d6d26ed84f077e28d84a685f71:1592955077.626271\"
Expires: Wed, 06 Oct 2021 21:42:11 GMT
Server: AkamaiNetStorage
Strict-Transport-Security: max-age=31536000 ; includeSubDomains

<!DOCTYPE html>
<html>
<head>
...

Other tools (the ones that I've interacted with) do not include the minor, probably because:
Quoting from the HTTP2 FAQ page:

The Working Group decided to drop the minor version (“.0”) because it has caused a lot of confusion in HTTP/1.x.
In other words, the HTTP version only indicates wire compatibility, not feature sets or “marketing.”

For this reason, it would have been convenient to be slightly more permissive, but I can also understand if you want to follow the RFC strictly, although in this case, the proposed code should only accept the following HTTP versions: 0.9, 1.0, 1.1, 2.0 and maybe 3.0 (which is not the case).

TL;DR I don't think that there is an official specification around this (at least for now), hence we can close this issue.

@forgedhallpass
Hi, as far as I know plaintext http version in HTTP/2 is only present in the HTTP/2 Connection Preface which is defined exactly as PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n

@AlexanderYastrebov yes, I know, but my use case was about parsing dumped HTTP/2 responses.
If this scenario is not to be supported, from a strictness point of view, you are right, the http.ParseHTTPVersion should probably only consist in:

func ParseHTTPVersion(version string) (major, minor int, ok bool) {
	switch version {
	case "HTTP/1.1":
		return 1, 1, true
	case "HTTP/1.0":
		return 1, 0, true
	case "HTTP/0.9":
		return 0, 9, true
	default:
		return 0, 0, false
	}
}

because otherwise this logic is not used for HTTP/2 requests at all.