Since WeakCrypto always makes me think that it's about all weak crypto (like
using MD5, or completely broken ciphers such as ARC4 ro DES) and not just about
weak key generation.
instead of points-to.
Looking at query results also made me realize I didn't supply a very good
"origin" for ECC in cryptography package, so I improved that 👍 -- maybe that
sohuld have been split into multiple commits... too late :(
* Removed backend arugment that is not required
* Added DSA constants (they are just accidentially the same as RSA right now)
* Removed FakeWeakEllipticCurve and used a real weak elliptic curve instead
I did spend some time to figure out how to best write `minimumSecureKeySize`
predicate. I wanted to write once and for all the recommended sizes for each
cryptosystem.
I considered making the predicate such as
```codeql
int minimumSecureKeySize() {
this.getName() = "RSA" and result = 2048
or
this.getName() = "DSA" and result = 2048
or
this.getName() = "ECC" and result = 244
}
```
but then it would be impossible to add a new model without also being able to
modify the body of this predicate -- which seems like a bad way to start off a
brand new way of modeling things.
So I considered if we could add it to the non-range class, such as
```codeql
class RSAKeyGeneration extends KeyGeneration {
RSAKeyGeneration() { this.getName() = "RSA" }
override int minimumSecureKeySize() { result = 2048 }
}
```
This has the major problem that when you're writing the models for a new
API (and therefore extending KeyGeneration::Range), there is no way for you to
see that you need to take this extra step :| (also problem about how we should
define `minimumSecureKeySize` on `KeyGeneration` class then, since if we make it
abstract, we effectively disable the ability to refine `KeyGeneration` since any
subclass must provide an implementation.)
So, therefore I ended up with this solution ;)
Tests working can be verified by running
```
ls ql/python/ql/test/experimental/library-tests/frameworks/crypto*/*.py | xargs -L1 sh -c 'python $0 || exit 255'
```
This was raised as a question at review, and I don't really have a good enough
argument for moving it under POI. At the end of the day, they are _security_
related enough I guess :)
This makes collecting metrics on framework coverage a bit simpler (specifically
giving the RoutedParameter class a more descriptive result for getSourceType).
I guess it can also help a bit when trying to get an overview of a new DB, but
making metrics collection easier is my main motivation for this.
One minor change to the tests results needed: there is no longer local
flow going into the `ModuleVariableNode` for `attr_ref` in the
`moduleattr.ql` test, but I think this is reasonable.
This fixes some spurious results that occurred when we considered
_any_ use of `request.something` to be a source, even ones we had
tracked into other functions. To prevent this, using
`getAnImmediateUse` better captures the fact that we want the source
to be just the actual attribute access.
Reported in https://github.com/github/codeql/issues/2650
I found this during a bit of spring cleaning in my working
directory. As this doesn't have any immediate security implications, I
don't know when we'll get round to fixing it, but it can't hurt to
have the test case checked in.
