Python: Promote Header Injection query from experimental

[joefarebrother]

Promotes #5463.
Includes models for sinks in Flask, Werkzeug, and stdlib wsgiref.
Django was originally modeled in the experimental query, but it has been determined that it does not accept newlines as part of the header name or value, and thus is not vulnerable to header injection; so those models have been removed.

It may be helpful to review per-commit.

[github-actions]

QHelp previews:

python/ql/src/Security/CWE-113/HeaderInjection.qhelp

HTTP Response Splitting

Directly writing user input (for example, an HTTP request parameter) to an HTTP header can lead to an HTTP response-splitting vulnerability.

If user-controlled input is used in an HTTP header that allows line break characters, an attacker can inject additional headers or control the response body, leading to vulnerabilities such as XSS or cache poisoning.

Recommendation

Ensure that user input containing line break characters is not written to an HTTP header.

Example

In the following example, the case marked BAD writes user input to the header name. In the GOOD case, input is first escaped to not contain any line break characters.

@app.route("/example_bad")
def example_bad():
    rfs_header = request.args["rfs_header"]
    response = Response()
    custom_header = "X-MyHeader-" + rfs_header
    # BAD: User input is used as part of the header name.
    response.headers[custom_header] = "HeaderValue" 
    return response

@app.route("/example_good")
def example_bad():
    rfs_header = request.args["rfs_header"]
    response = Response()
    custom_header = "X-MyHeader-" + rfs_header.replace("\n", "").replace("\r","").replace(":","")
    # GOOD: Line break characters are removed from the input.
    response.headers[custom_header] = "HeaderValue" 
    return response

References

• SecLists.org: HTTP response splitting.
• OWASP: HTTP Response Splitting.
• Wikipedia: HTTP response splitting.
• CAPEC: CAPEC-105: HTTP Request Splitting
• Common Weakness Enumeration: CWE-113.
• Common Weakness Enumeration: CWE-79.

[RasmusWL]

Overall LGTM, I think you did really a really good job promoting this query, thinking about many of the corner cases that could be encountered.

To align better with the Java query, I would suggest we rename this query into HTTP response splitting. Although I don't find anything wrong with the Header Injection name, I feel better alignment wins in this scenario. (but do let me know if you disagree).

Nitpicking a bit, I would recommend merging Tests1 and Tests2 folders for query testing. Navigating the two folders is a bit annoying, and when the only difference is showing that using a validator for wsgiref mitigates this attack, I think you can just include that in the other test file (test_app3/4).

ConceptsTests

In terms of testing the new concepts, we've traditionally added these to a shared ConceptsTest. This test is run on all our library modeling test folders. The core idea being that we test our library modeling inside the library-tests/frameworks/<library> folder, and then only have the bare essentials in the query test folder. For example, instead of testing all ways of setting a cookie in the ql/test/query-tests/Security/CWE-113-HeaderInjection/Tests1/wsgiref_tests.py file, we would do so in ql/test/library-tests/frameworks/stdlib/wsgiref_simple_server_test.py. This way, if another query wants to utilize the header write modeling, we don't need to replicate all the different ways to write to a header in the query test files, but can just rely on the library test files.

So while there's not anything wrong with the test you have done, I would be violating our test principle if I didn't ask you to add a new case to ConceptsTests 😊

(see examples of cookie handling by following the links in the section below)

optional followup work:

A bit of followup work that we could do, is to rewrite the library modeling that handles setting cookie values, as some of them look for writes to the Set-Cookie headers. Example modeling + tests.

---

I also noted that we could restore django header writes by reverting this specific change.

[joefarebrother]

The reason for the tests being split into Test1 and Test2 folders is that the models only check for the presence of a validator anywhere in the database, so a separate DB is needed to test for the case in which it isn't used.

[joefarebrother]

@RasmusWL Tests have been added to ConceptTests

[RasmusWL]

Thanks for the updates to the tests, I think you did a good job following the general way our other ConceptTests have been structured 👍

---

The reason for the tests being split into Test1 and Test2 folders is that the models only check for the presence of a validator anywhere in the database, so a separate DB is needed to test for the case in which it isn't used.

Makes sense 👍 would be great if we can highlight that in the top of the file, so it's obvious why we need it. I would probably name the tests something like this, so make it clear:

• python/ql/test/query-tests/Security/CWE-113-HeaderInjection/
• python/ql/test/query-tests/Security/CWE-113-HeaderInjection-wsgi-validator/

Actually, looking through the code a bit more, we could extend the startResponse type-tracker to keep track of what WsgirefSimpleServerApplication it was initiated from (so we can know whether a validator is applied or not). NOTE: this will only solve the validated/unvalidated problem for the start response call and not any of the other ways to set headers... I think that's OK 👍 (but that's just my gut feeling, I don't have any actual data to back it up)

[joefarebrother]

I have now renamed one of the test folders and added an explanatory comment.

I considered tracking validation status to the startResponse calls; however other sinks (such as the Headers class) would still have to rely on the validator existence check, so it doesn't really make the test structure that much more convenient; and in terms of actual results it could make a difference just in a case where there are multiple wsgi apps, only some of which are validated. But my intuition says that this doesn't seem like a very likely situation outside of tests.

[RasmusWL]

Nice 👍