Validate method parameter of HttpClient.open(), check it has no CR, LF

[licy183]

import 'dart:convert';
import 'dart:io';

void main() {
  HttpClient client = HttpClient();
  client
      .open("GET / HTTP/1.1\r\nHost: example.com\r\n\r\nGET", "example.com",
          80, "/index.html")
      .then((request) => request.close())
      .then((value) => value.toList())
      .then((value) {
    print(utf8.decode(value.fold([], (previousValue, element) {
      previousValue.addAll(element);
      return previousValue;
    })));
  });
}

The code above will generate a Http Request which looks like the following.

GET / HTTP/1.1
HOST: EXAMPLE.COM

GET /index.html HTTP/1.1
user-agent: Dart/2.12 (dart:io)
test: test
accept-encoding: gzip
content-length: 0
host: example.com

This request will let the server response twice and make the HttpClient throws an exception.

Unhandled exception:
HttpException: Unexpected response (unsolicited response without request).

And this will cause a CRLF injection.
Related Issue: cfug/dio#1130

[a-siva]

/cc @aam

[aam]

If I understand CRLF injection correctly assumption is that some unexpected code gets in the middle between client and server and updates method of the HTTP request with headers separated by CRLF.
In this example, though, all of the code lives in one client dart application - how some code is expected to get in the middle?
If it is the developer who writes such code that adds headers to the method parameter, how that would be different from that developer actually using headers field of HttpClientRequest produced by HttpClient.open?

[licy183]

The RFC-2616 on Page 35, states the following:

The Request-Line begins with a method token, followed by the Request-URI and the protocol version, and ending with CRLF. The elements are separated by SP characters. No CR or LF is allowed except in the final CRLF sequence.
Request-Line = Method SP Request-URI SP HTTP-Version CRLF

According to this, maybe it is better to check the method parameter to ensure that it contains neither CR('\r') nor LF('\n').

[aam]

Yeah, we could add such a check. That would be a breaking change I imagine.
Lowering to P3 from P1.

[flbaue]

I just noticed this issue because dio has a CVE entry about this: https://nvd.nist.gov/vuln/detail/CVE-2021-31402 which is scored as high.
How do you estimate the risk of this?

[aam]

Unless I am missing something it's dart code itself that sends a request that also controls the method name. There is nowhere for attacker to insert itself so they update method name maliciously. Because of that I think the risk is non-existent/low.

Still though, validating that method argument(https://github.com/dart-lang/sdk/blob/main/sdk/lib/_http/http.dart#L1380 and https://github.com/dart-lang/sdk/blob/main/sdk/lib/_http/http.dart#L1395 has no CRLF makes sense.

cc @brianquinlan

[mkranus]

@aam @brianquinlan What is the progress on this? German regulators have caught up to this problem due to CVE priority. Please either fix this, or negotiate with CVE to revert.

[aam]

Looks like it slipped through the cracks.
https://dart-review.googlesource.com/c/sdk/+/256429 should address the problem.