False positive: Confusing "... may be at an arbitrary position in the sanitized URL."
- Langage dominant
- CodeQL
- Étoiles
- 10.1k
- Forks
- 2.1k
- Merge moyen
- 2 j 15 h
- PR mergées (30 j)
- 141
Description
**Description of the false positive**
To be honest I ~~don't~~ didn't really understand the aim of the rule at all - but it is reporting code like:
- `assert 'http://www.example.com/' in web_history`
- `assert 'https://github.com' in bookmarks.marks`
- `assert 'https://python.org' in web_history_populated`
- `assert 'http://example.com' in bm.marks`
All of those are checking whether a string (which happens to be an URL) is in a set or list. This doesn't seem to be related to sanitization at all...
Looking at the [query](https://github.com/github/codeql/blob/master/python/ql/src/Security/CWE-020/IncompleteUrlSubstringSanitization.ql) and [help](https://github.com/github/codeql/blob/master/python/ql/src/Security/CWE-020/IncompleteUrlSubstringSanitization.qhelp) I think flagging things like the above isn't the intention of the rule? The help says:
> Sanitizing untrusted URLs is an important technique for
> preventing attacks such as request forgeries and malicious
> redirections. Usually, this is done by checking that the host of a URL
> is in a set of allowed hosts.
>
> However, it is notoriously error-prone to treat the URL as
> a string and check if one of the allowed hosts is a substring of the
> URL. Malicious URLs can bypass such security checks by embedding one
> of the allowed hosts in an unexpected location.
>
> Even if the substring check is not used in a
> security-critical context, the incomplete check may still cause
> undesirable behaviors when the check succeeds accidentally.
So I assume what the query actually indends to check is something like `if 'https://example.com' in url` - but then the RHS must be a string, which is not the case here.
cc @markshannon who added the rule in #820.
**URL to the alert on the project page on LGTM.com**
I'm using CodeQL via GitHub's security scanning (which is in Beta) - so I hope I'm reporting this in the right place, given that the rules behind the two sites seem to be the same...
Here are the alerts I think are false-positives: https://github.com/qutebrowser/qutebrowser/security/code-scanning?query=tool%3ACodeQL+is%3Aclosed+id%3Apy%2Fincomplete-url-substring-sanitization
Guide de contribution
Ouvrir le guide de contribution
Évaluation
Cette issue n'a pas encore été évaluée.