Skip to content

Conversation

RasmusWL
Copy link
Member

@RasmusWL RasmusWL commented Feb 19, 2020

This is actually a part of my effort for writing the SSRF query, but I think we can discuss this part without the rest.

I'm a bit skeptical about this change, since we don't handle the sanitation steps below. Would very much like your opinion @tausbn. How do we proceed from here?

def test_sanitizer():
    tainted_string = TAINTED_STRING
    urlsplit_res = urlsplit(tainted_string)

    if urlsplit_res.netloc == "OK":
        test(urlsplit_res.netloc) # TODO: FP, should not be tainted here

    if urlsplit_res[2] == "OK":
        test(urlsplit_res[0]) # TODO: FP, should not be tainted here

Could I write a query that finds such places? Yes, yes I could... https://lgtm.com/query/1626963161462581314/

Following the setup I invented for library-tests/taint/unpacking.

TestStep is still a bit annoying, since the output is not easy to eyeball; but
for now I guess we can live with it :)

I honestly didn't get the point of DistinctStringKinds.ql, other than showing we
can handle multiple taint kinds
@RasmusWL RasmusWL requested a review from a team as a code owner February 19, 2020 16:13
@RasmusWL
Copy link
Member Author

I ended up writing a sanitizer for this. It's not a perfect solution, since I couldn't find a way to turn it on by default -- so right now the extra taint from urlsplit/urlparse is enabled by default, but the sanitizer isn't 😕

Anyway, do you feel happy about this @tausbn ? I realize now that I forgot to discuss this approach in person 😅

Copy link
Contributor

@tausbn tausbn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the short term, I think this solution is fine, especially as it'll unblock you on the larger query you're working. We definitely need a better way of handling this, though.

One comment to address, otherwise ready to merge.

@tausbn tausbn merged commit 8bd3063 into github:master Feb 27, 2020
@RasmusWL RasmusWL deleted the python-taint-urlsplit branch February 27, 2020 12:27
tausbn added a commit to tausbn/codeql that referenced this pull request Mar 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants