Ruby/Python: Add a BlockMode concept for CryptographicOperations#9157
Conversation
| /** | ||
| * Holds if `name` corresponds to a weak block cipher mode of operation. | ||
| */ | ||
| predicate isWeakBlockMode(string name) { name = "ECB" } |
There was a problem hiding this comment.
This is technically a breaking change, but it's inside an /internal/ folder, so it's fine.
JS 👍
| } | ||
|
|
||
| override predicate hasActualResult(Location location, string element, string tag, string value) { | ||
| exists(location.getFile().getRelativePath()) and |
There was a problem hiding this comment.
Copied this from another test but it's not relevant here. Deleted.
There was a problem hiding this comment.
it's probably from a Python test. Since we extract dependencies/standard library, sometimes we will get results in code that is not user-written, which we don't want.
hmac
left a comment
There was a problem hiding this comment.
The Ruby changes look good to me! I wonder, could we directly share the CryptographicOperation concept between Python and Ruby by moving it to ConceptsShared.qll?
|
@hmac thanks for the review. I noticed a regression in the DCA results where I was making an incorrect assumption that ciphers instantiated with
Yeah, I think this makes sense. I held off doing this mostly because Javascript implements |
|
The way that shared concepts are structured, JS can (I believe) opt to selectively import the ones they want, as a stop-gap until they can adopt the concept properly. So if we have good alignment across Python and Ruby, I think that's a good enough reason to share the concept. @RasmusWL would you agree? |
👋 just following a bit along on the sidelines, since I should be reviewing this PR soon. It's fine to share the concept between Ruby and Python initially, and let JS adopt it later on. |
|
Cool, I will have a go at moving this into |
RasmusWL
left a comment
There was a problem hiding this comment.
Nice 🎉 Thanks for porting this to Python as well 💪 A few minor things, otherwise looks good to me 👍
Co-authored-by: Rasmus Wriedt Larsen <rasmuswriedtlarsen@gmail.com>
… modes Co-authored-by: Rasmus Wriedt Larsen <rasmuswriedtlarsen@gmail.com>
|
I've run into a somewhat interesting issue in sharing the This predicate is now unused in Ruby. I think it should probably be deprecated and eventually removed - whilst an operation may be insecure in other ways (e.g. insufficient key size), I think we'd still usually want to expose to a user why the operation is weak - something that a simple predicate on the operation alone can't communicate. I've approached this in the current PR by:
It doesn't feel great to add a predicate which is then immediately deprecated, but it seemed like the least bad solution which complied with the deprecation policy. |
Yeah, this is a shame. I personally don't think we should be following the strict deprecation policy for Ruby while it is in beta, since it makes it harder to clean things up like this. |
I'd agree - our policy on 1 year deprecation periods applies to stable releases, which arguably means that beta languages should not be included. The policy should perhaps be more explicit about languages in beta.
|
The old code was my own suggestion, that I thought would just work, but
was also slightly skeptical about.
I tested out whether it works with the code below
```codeql
predicate foo(int input, string res) {
input = 1 and res = "that was one"
}
from int input, string res
where
input in [1, 2] and
if foo(input, res)
then any()
else res = "not one"
select input, res
```
which gave the 3 results
```
1 | that was one
1 | not one
2 | not one
```
only by rewriting the code to be the one below, did I get down to the 2
results I actually wanted. So I've done the same kind of rewrite in the
commit.
```codeql
predicate foo(int input, string res) {
input = 1 and res = "that was one"
}
from int input, string res
where
input in [1, 2] and
if foo(input, _)
then foo(input, res)
else res = "not one"
select input, res
```
RasmusWL
left a comment
There was a problem hiding this comment.
I just double checked my suggestion in #9157 (comment), and it was not working as intended, so I just fixed that up. (and happy to have done that before the PR was merged
But overall looks good to me now 👍 (not sure whether this is blocked on anything at the moment?)
Thanks for the fix, I see the problem now after reading through the commit message. It's a shame that the I'll just wait on a final approval from @github/codeql-ruby before merging this. |
Cryptographic operations that use a block cipher such as AES often require a block mode of operation. A common case for this is in encrypting data of arbitrary length using a block cipher. These block modes can have inherent weaknesses that make them less suited to cryptographic purposes - in particular the electronic codebook (ECB) scheme can reveal patterns in data even if the underlying block cipher is secure.
The existing
rb/weak-cryptographic-algorithmquery checks for uses of ECB, but the message reported to the user was inaccurate, as in cases such asAES-ECB, it would reportAESas being insecure rather than theECBblock mode. This is corrected in this PR by adding a newBlockModeconcept with anisWeakmember predicate and adding aBlockMode getBlockMode()predicate toCryptographicOperationalongside the existingCryptographicAlgorithm getAlgorithm()predicate.I've made a similar change to the python version of this query. This mostly changes the
cryptodomeandcryptographylibraries. I'm happy to remove the Python changes, or to move them out to a separate PR if that's more convenient than having everything in this PR.Whilst implementing this, I noticed that the Ruby and Python
CryptographicOperations are conceptually different. Ruby uses it to mean an instantiation of a cryptographic cipher or a use of that cipher (e.g. in encrypting some data), whereas it seems like Python uses it to mean only a use of such a cipher. I think that the Python version of this makes more sense, as the Ruby version seems like it could be unnecessarily noisy.On another note,
CryptographicOperationencompasses quite a lot of different operations, including many where a block mode is not relevant. This means that many implementations are justnone(). This might suggest thatCryptographicOperationshould be subclassed for different cases (e.g. encrypting a stream of data, cryptographic hashing, etc).