The output might end up being slightly more noisy since we don't
collapse positional and keyword arguments when the external target
function is included in the database, but this aligns with our long-term
goal of not doing that anymore, so I think it's fine.
The current PAM auth bypass query which was contributed by me a few months back, alert on a vulenrable function but does not check if the function is actually function. This leads to a lot of fasle positives.
With this PR, I add a taint-tracking configuration to check if the username parameter can actually be supplied by an attacker.
This should bring the FP's significantly down.
note how the test file is partially annotated
and those annotations can now be expressed
In this particular test file, absolute line numbers
might have been better than relative ones.
We might remove line numbers altogether,
but should check more querries to see how it looks.
It's really hard to audit that this is all good.. I tried my best with
`icdiff` though -- and there is a problem with
ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.ql
that needs to be fixed in the next commit
This partly reverts the changes from https://github.com/github/codeql/pull/10252
Although consistency is nice, the new messages didn't sound as natural.
New alert message would read
> Insecure hashing algorithm (md5) depends on sensitive data (password). (...)
I'm not sure what it means that a hashing algorithm depends on data. So
for me, the original text below is much easier to understand.
> Sensitive data (password) is used in a hashing algorithm (md5) that is insecure (...)
Same goes for the other sensitive data queries.
Using API graphs instead of points-to.
Unfortunately, some results will be lost because of this, due to the
fact that points-to tracks bitwise operations on small numbers (i.e.
flags), whereas API graphs does no such thing. This means using
something like `stat.S_IWUSR | stat.S_IWGRP` will not work.
A custom type tracker (like the one used for `re` flags) could be used
to recapture this behaviour, but I think that's best left as future
work, as it's not clear to me that this query is actually worth the
effort it would take to implement this.
