Gh 511 http header injection fix

[n0npax]

PoC of solution for issue #511

[natebosch]

cc @lrhn

Should we consider doing this check in an assert?

I don't think I'd frame this as a security issue - it's more of a potential footgun for authors. I don't know how often I'd expect an author to make a mistake with this API, but I'm on board with adding some rails if we think that it's likely. An assert adds some help for authors without any overhead in production.

[n0npax]

Please have a look at https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-26137 & https://bugs.python.org/issue39603
which is a very similar issue. Base on this I believe it's not just a foot gun, as the same risk exists here.

I'm happy to change contains to assert. I'll update the PR soon. As per: https://dart.dev/guides/language/language-tour#assert I believe using assert is not the right way, as they won't be used all the time. In example, if --enable-asserts is not passed, the assertion may be ignored

[OS-WS]

17bc4bf
Hi, is this commit the fix for CVE-2020-35669?

if not, what is?

[n0npax]

818cff6 is a fix. The remaining commits are just unit tests and some lint fixes.

To fix stuff properly there may be a need to bump the version in https://github.com/dart-lang/http/blob/master/pubspec.yaml#L2 & release, but it's up to maintainers to decide if they prefer 0.13.1 or backport to 0.12.3 🤷🏻‍♂️

[n0npax]

@lrhn can I ask for a review please?

[n0npax]

@kevmoo can I ask for a review please ?

[kevmoo]

@lrhn ?

[lrhn]

I'd test this properly, not just in an assert. Any overhead is negligible compared to doing a network request.

I agree that it's probably not a big security issue since you can equally easily do a direct TCP connection and send any request block. If malicious code is running in the same isolate as something else, you should assume that something else to be thoroughly compromised anyway, so unless code is taking the method value as input from a remote source, and not validating it at all (which is already dangerous when one of the values are DELETE), then it's very hard to do something bad with this mis-feature which isn't also possible without it.
It's still an unnecessary risk, someone could be living dangerously and accepting method values from strangers, so we might as well plug it.

[lrhn]

I don't know if this package uses the dart:io HTTP operations. If it does, we might be able to remove this again when they're fixed. I believe the browser XMLHttpRequest is already validating the method.

[kevmoo]

Should also bump the version and and an entry to the changelog!

[n0npax]

@kevmoo done, hope it's ok 🤞🏻

[google-cla]

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

[natebosch]

@googlebot I consent.

[google-cla]

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

[n0npax]

@googlebot I consent.