[inets/3392] Fix for CVE-2016-1000107

[marcellanz]

Fixes #3392 Reproduction and Fix: https://github.com/erlang/otp/issues/3392#issuecomment-1213289354

Open tasks

• [ ] add test(s)
• [ ] probably document the behaviour for httpd_script_env:create_env that it skips PROXY HTTP headers because of this CVE.
• [x] review my beginners erlang code

[github-actions[bot]]

CT Test Results

    2 files    21 suites   13m 8s :stopwatch: 332 tests 310 :heavy_check_mark: 21 :zzz: 1 :x: 554 runs  517 :heavy_check_mark: 37 :zzz: 0 :x:

For more details on these failures, see this check.

Results for commit 65738835.

:recycle: This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

• Complete CT logs (Download Logs)
• HTML Documentation (Download HTML Docs)
• Windows Installer

// Erlang/OTP Github Action Bot

[artman41]

@marcellanz is proxy a case-insentive varname or is it either proxy or PROXY?

If it's case-insensitive, I'd be looking to add a case statement doing case string:lowercase(VarName0) of to the existing cgi case for http_env_element/3

[IngelaAndin]

@marcellanz Thank you for the PR, we still have some people on vacation so please be patient. I will ask @RaimoNiskanen to take a look when he is back.

[RaimoNiskanen]

Should we handle mixed case variants too, such as pRoXy?

[KennethL]

Since we are creating an upper case HTTP_PROXY if the request contains a HTTP header named proxy in whatever mixed case the comparision ought to be done on a header name which is normalized to lower or upper case. Something like this:

http_env_element(cgi, VarName0, Value)  ->
    case http_util:to_upper(VarName0) of
           "PROXY" -> 
                   skipped;
          VarName1 ->
                   VarNameUpper = re:replace(VarName1,"-","_", [{return,list}, global]),
                  {"HTTP_"++ VarNameUpper, Value}
     end.

[marcellanz]

I agree. According to the defining RFC 7230, http header field names are case-insensitive, normalizing to an uppercase value seems to make perfect sense. Similar the CGI spec defines the protocol specific meta variables are converted to upper case names; so it would be safe to use that normalization here too.

[RaimoNiskanen]

According to RFC 3875, in particular section 4.1.18 variable names are case insensitive, so a case canonicalization before comparison is required.

Why not speculatively do both re:replace/4 and http_util:to_upper/1 and then compare with "PROXY"?

[marcellanz]

This makes sense for sure. I was not sure if the current implementation actually does this somewhere before with how CGI is implemented and a variable would arrive already upper case. Implementing this in a defensive way still is preferable I think.

[marcellanz]

I've updated the branch to reflect the discussion above by canonicalizing the HTTP variable name to upper case before comparing. I'm not sure with tests. I've tested the behaviour like I have described it here; for a permanent test with this change, would one do a integration like test or would a scoped test towards http_env_element be enough? I'm not used to work with the libraries so far, so my question.

[IngelaAndin]

I think I could be ok with a white box test in http_format_SUITE. You would have to create some internal data to be able to call httpd_script_env:create_env/3

[marcellanz]

I'll provide such a test.

[marcellanz]

I was absorbed with a lot of "work work" lately; I will find some time over xmas to provide this test.

[IngelaAndin]

@marcellanz ping

[marcellanz]

true – I'd do that soon enough. Actually "work work" reduced a bit so I shall be able to do it. Thanks for the patience.

[RaimoNiskanen]

Ping?