Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Python: Add SSRF queries #7420

Merged
merged 26 commits into from Dec 17, 2021
Merged

Python: Add SSRF queries #7420

merged 26 commits into from Dec 17, 2021

Conversation

@RasmusWL
Copy link
Member

@RasmusWL RasmusWL commented Dec 16, 2021

I've added 2 queries:

  • one that detects full SSRF, where an attacker can control the full URL, which is always bad
  • and one for partial SSRF, where an attacker can only control parts of an URL (such as the path, query parameters, or fragment), which is not a big problem in many cases (but could still be exploitable)

full SSRF should run by default, and partial SSRF should not (but having the query included makes it easy to run). I got inspired by this setup from Java where they have a precise and imprecise version of the same query.

Current status

Most of the query work/library modeling is done (although we could always add support for more libraries). Still need to do:

  • write qhelp
  • write change-note
  • Polish sanitizer for full SSRF query so we're able to detect "https://" + user_input is in fact controlling the full URL.
  • verify FP rates from run across many repos
  • verify performance looks ok
  • make some updates to the Ruby code so we're better aligned (but that can wait until after this PR is merged I think)

Commits

Some of the commits changes the concepts that was added in the very first commit. I've kept things this way so it could help to illustrate why I wanted to diverge from the Ruby code.

What is SSRF even?

See https://portswigger.net/web-security/ssrf if you need a refresher on SSRF 😊

RasmusWL added 12 commits Dec 13, 2021
Taken from Ruby, except that `getURL` member predicate was changed to
`getUrl` to keep consistency with the rest of our concepts, and stick
to our naming convention.
For the snippet below, our current query is able to show _why_ we
consider `var` to be a falsey value that would disable SSL/TLS
verification. I'm not sure we're going to need the part that Ruby did,
for being able to specify _where_ the verification was removed, but
we'll see.

```
requests.get(url, verify=var)
```
Also adjusts test slightly. Writing
`clientRequestDisablesCertValidation=False` to mean that certificate
validation was disabled by the `False` expression is just confusing, as
it easily reads as _certificate validate was NOT disabled_ :|

The new one ties to each request that is being made, which seems like
the right setup.
I think `getUrl` is a bit too misleading, since from the name, I would
only ever expect ONE result for one request being made.

`getAUrlPart` captures that there could be multiple results, and that
they might not constitute a whole URl.

Which is the same naming I used when I tried to model this a long time ago
https://github.com/github/codeql/blob/a80860cdc6b06b363b0d0919600ab383a470b449/python/ql/lib/semmle/python/web/Http.qll#L102-L111
I've added 2 queries:

- one that detects full SSRF, where an attacker can control the full URL,
  which is always bad
- and one for partial SSRF, where an attacker can control parts of an
  URL (such as the path, query parameters, or fragment), which is not a
  big problem in many cases (but might still be exploitable)

full SSRF should run by default, and partial SSRF should not (but makes
it easy to see the other results).

Some elements of the full SSRF queries needs a bit more polishing, like
being able to detect `"https://" + user_input` is in fact controlling
the full URL.
RasmusWL and others added 8 commits Dec 16, 2021
Since that might not be the same place where the vulnerable URL part is.
Now full-ssrf will only alert if **all** URL parts are fully
user-controlled.
They were very misleading before, because a sanitizer that happened
early, would remove taint from the rest of the cases by use-use flow :|
@RasmusWL RasmusWL requested a review from yoff Dec 17, 2021
I included examples of both types in the qhelp of both queries, to
provide context of what each of them actually are.
@RasmusWL RasmusWL marked this pull request as ready for review Dec 17, 2021
@RasmusWL RasmusWL requested a review from as a code owner Dec 17, 2021
Copy link
Contributor

@yoff yoff left a comment

LGTM - thanks for the offline explanations

yoff
yoff approved these changes Dec 17, 2021
Copy link
Contributor

@yoff yoff left a comment

Lgtm

@RasmusWL
Copy link
Member Author

@RasmusWL RasmusWL commented Dec 17, 2021

:shipit:

@codeql-ci codeql-ci merged commit 5054d5b into github:main Dec 17, 2021
15 checks passed
@RasmusWL RasmusWL deleted the ssrf-new branch Dec 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants