JS: Use shared `CryptographicOperation` concept #12080

alexrford · 2023-02-02T20:32:51Z

This mostly affects JS, but also changes some behaviour for Ruby and Python as well. This change should allow us to add the Python version of weak-cryptographic-algorithm as a (new) JS query.

The JS and Ruby/Python versions of CryptographicOperation were pretty similar, with 2 main differences:

getInput() in JS vs getAnInput() in Ruby/Python
JS lacked the BlockMode getBlockMode() predicate

I added a deprecated getInput() predicate to the shared concept for the first difference, and implemented getBlockMode() for the existing JS CryptographicOperation implementations. There were 10 implementations in CryptoLibraries.qll, but only 4 of these libraries seemed to support encryption using block ciphers.

There are a couple of other changes that affect all languages.

The first is the addition of a BlockMode#matchesString(string) predicate for convenience
The other is a pair of more significant change to make CryptographicAlgorithm#matchesName(string).
- We now consider splitting the candidate name on underscores - e.g. asmCrypto uses "AES_OFB" as an algorithm name to specify AES encryption using the OFB block mode - previously matchesString would have just read this as "AESOFB" and not matched against AES
- matchesName now only holds when the algorithm is the "most specific" matching algorithm - in practice this means the matching algorithm with the longest name. This is specifically for things like "SHA3_224" which has a valid match as both SHA3 and SHA3_224, but the latter is a more precise match. This is relevant for e.g. https://pycryptodome.readthedocs.io/en/latest/src/hash/sha3_224.html

…getInput() predicate

… getBlockMode()

…derscores

…f that algorithm is the most specific match

…icOperation#getBlockMode() for compatibility with external code

erik-krogh

Looks good, with some minor comments.

But we need a loud change-note, and some more instructions on how to migrate to the new code (both in change-note and QLDoc).

erik-krogh · 2023-02-03T16:44:56Z

javascript/ql/lib/semmle/javascript/frameworks/CryptoLibraries.qll

-/**
- * An application of a cryptographic algorithm.
- */
-abstract class CryptographicOperation extends DataFlow::Node {


This is problematic.
You are changing an abstract class to a non abstract class that instead uses the ::Range pattern.

This will at minimum require a loud change-note.
CryptographicOperation is on the "global" scope in JavaScript, so we can't leave a deprecated class behind.
Anyone overridding the old CryptographicOperation class will get deprecation warnings, so I think it's good enough with a change-note.

I've added a change note that hopefully explains the required changes to any external implementations of CryptographicOperation clearly enough.

javascript/ql/lib/semmle/javascript/frameworks/CryptoLibraries.qll

erik-krogh · 2023-02-03T18:13:41Z

javascript/ql/lib/semmle/javascript/internal/ConceptsShared.qll

    DataFlow::Node getAnInput() { result = super.getAnInput() }

+    /** DEPRECATED. This predicate has been renamed to `getAnInput`. */
+    deprecated final DataFlow::Node getInput() { result = super.getInput() }


This will cause compile-errors for any external users that have extended this.
(Also see my former comment about CryptographicOperation and change-note).

I think we'll have to use breaking in the change-note.

I think the QLDoc should loudly explain what to do, which is both extend getAnInput instead, and make sure you extend CryptographicOperation::Range instead of CryptographicOperation.

I've updated the QLDoc to mention this (the predicate itself has moved to Concepts.qll to avoid introducing a deprecated predicate to Ruby/Py as well). It's a little bit clumsy as the deprecation message has to read properly for both users and implementers of CryptographicOperation#getInput().

erik-krogh · 2023-02-03T18:13:55Z

javascript/ql/lib/semmle/javascript/internal/ConceptsShared.qll

      abstract DataFlow::Node getAnInput();

+      /** DEPRECATED. This predicate has been renamed to `getAnInput`. */
+      deprecated final DataFlow::Node getInput() { result = this.getAnInput() }


Same as above.

CryptographicOperation::Range#getInput() has been removed - I don't think there's a need to include this as the ::Range class is new for JS.

RasmusWL

Overall looks good to me 👍 Thanks for moving our languages closer to each other 💪

I'm wondering whether the changes in b968b59 to matchesName is important for anything, or it just seemed like a good idea? -- We've previously seen performance problems with the Python analysis when doing too much string manipulation, so that is why I'm interested in this bit in particular (and would at least like to see a performance evaluation before giving a final approval for this)

RasmusWL · 2023-02-14T13:43:34Z

javascript/ql/lib/semmle/javascript/internal/ConceptsShared.qll

+    /** DEPRECATED. This predicate has been renamed to `getAnInput`. */
+    deprecated final DataFlow::Node getInput() { result = super.getInput() }


NIT: We don't need to add this deprecated predicate to the shared file, it's possible to only have it part of JS like we've done for Ruby in the past (example).

👍 it seems worth avoiding adding a deprecated predicate for all languages.

I've removed this from Ruby/Py in c7aaad9. The explicit aliases feel a bit clumsy, but I'm not aware of a good way to avoid them.

Thanks, yes those aliases are a bit clumsy. I wonder if the new shadowing mechanism in QL would allow us to do it more nicely 🤔

RasmusWL · 2023-02-14T13:53:41Z

javascript/ql/lib/semmle/javascript/internal/ConceptsShared.qll

       * is a stream cipher rather than a block cipher.
       */
-      abstract BlockMode getBlockMode();
+      BlockMode getBlockMode() { none() }


As mentioned in other comments, it's possible to do this change of making it non-abstract for JS only.

My default stance on this is that it's nicer if the predicates you're supposed to implement are abstract, since that means you won't forget to do it. However, there is also part of me that is annoyed at having to implement getBlockMode for hashing cryptographic operations...

So I guess now is a good time to think about whether we want to change this to have a default of none() for all languages or not. What are your own thoughts on this?

I've been uncertain on this as the need to implement this for hashing operations is irritating. I'd briefly considered creating separate EncryptionOperation and HashingOperation (and maybe also PasswordHashingOperation) classes and only including the getBlockMode() predicate in EncryptionOperation, but I think this is maybe overcomplicating things since getAlgorithm() already works to restrict the operation to a particular kind of operation.

On reflection I think this change to have a default none() is a mistake. It's too easy to just forget to implement this predicate entirely if it's not abstract. I also don't think there's a need to make this change for JS specifically, since this is already a breaking change which will require changes to external implementations of CryptographicOperation.

My plan here is to change this back to being abstract (for all languages) and to improve the documentation on this to make it clearer when this should be implemented with a non-none() result (i.e. only when this is an encryption operation using a block cipher).

sounds good to me 👍

alexrford · 2023-02-15T15:20:43Z

I'm wondering whether the changes in b968b59 to matchesName is important for anything, or it just seemed like a good idea? -- We've previously seen performance problems with the Python analysis when doing too much string manipulation, so that is why I'm interested in this bit in particular (and would at least like to see a performance evaluation before giving a final approval for this)

There were some specific cases that I was trying to cover here. In asmCrypto (JS) we have:

asmCrypto.AES_OFB.encrypt(input, key, iv)

where the version of matchesName before this PR would try to match against the string AESOFB (as \w includes underscores) and would not match against AES as it should. I started to address this in 1435ef1, but found that it meant that Cryptodome (Py) had ambiguity in cases like:

from Cryptodome.Hash import SHA3_224

hasher = SHA3_224.new()

which would match against both SHA3 alone (split before the underscore) and SHA3224 (full string). Both are "correct", but including both means that getAlgorithm() doesn't necessarily have a unique result and it seems redundant to return SHA3 when SHA3224 is more precise, hence b968b59.

I'll start some DCA runs for all languages after a few changes are finalized here.

…being an abstract predicate

…ut() predicate

javascript/ql/lib/semmle/javascript/Concepts.qll

erik-krogh

JS 👍

alexrford · 2023-02-17T11:58:28Z

@RasmusWL I finally got together 3 successful DCA runs - I didn't notice any performance differences, though admittedly the test suites don't have that many projects.

RasmusWL

LGTM 👍 Thanks for making the changes. Performance evaluation looked fine as well 👍

alexrford added 7 commits February 2, 2023 20:27

ConceptsShared: Add deprecated DataFlow::Node CryptographicOperation#…

61095b3

…getInput() predicate

ConceptsShared: Add BlockMode#matchesString(string) predicate

e5dfbe2

JS: Use shared CryptographicOperation concept and implement BlockMode…

983055b

… getBlockMode()

CryptoAlgorithms: make CryptographicAlgorithm#matchesName split on un…

1435ef1

…derscores

JS: add blockMode to CryptographicOperation tests

c25dc97

JS: adjust test whitespace

aa2c532

JS: add some CryptographicOperation#getBlockMode() tests

b0b8f87

alexrford added JS Python no-change-note-required This PR does not need a change note Ruby labels Feb 2, 2023

github-actions bot added the ATM label Feb 2, 2023

alexrford added 4 commits February 3, 2023 12:12

JS: update CryptographicKey.expected

6b2a92a

JS: pick up CryptographicKeys used in asmCrypto encrypt/decrypt calls

e17b3d9

CryptoAlgorithms: make CryptographicAlgorithm#matchesName hold only i…

b968b59

…f that algorithm is the most specific match

ConceptsShared: add a default implementation of BlockMode Cryptograph…

6c35fea

…icOperation#getBlockMode() for compatibility with external code

alexrford marked this pull request as ready for review February 3, 2023 15:18

alexrford requested a review from a team February 3, 2023 15:18

alexrford requested review from a team as code owners February 3, 2023 15:18

Merge branch 'main' into js-use-shared-cryptography

7768026

erik-krogh self-requested a review February 3, 2023 16:45

erik-krogh requested changes Feb 3, 2023

View reviewed changes

improve the CryptoJS model by using API::Node

ecafce8

calumgrant requested a review from RasmusWL February 6, 2023 10:29

RasmusWL reviewed Feb 14, 2023

View reviewed changes

JS: remove unused field

8d90c02

JS: avoid adding a deprecated CryptographicOperation#getInput to py/ruby

c7aaad9

alexrford added 4 commits February 15, 2023 16:23

dynamic: switch CryptographicOperation::Range#getBlockMode() back to …

d4d0b91

…being an abstract predicate

JS: improve documentation on deprecated CryptographicOperation#getInp…

925b4a3

…ut() predicate

JS: breaking change note for CryptographicOperation sync

e8cbf72

dynamic: more detailed qldoc for CryptographicOperation#getBlockMode()

43af306

github-actions bot added the documentation label Feb 15, 2023

alexrford and others added 2 commits February 15, 2023 16:59

JS: add missing qldoc

1958b9d

Merge branch 'main' into js-use-shared-cryptography

1556b1a

erik-krogh reviewed Feb 15, 2023

View reviewed changes

javascript/ql/lib/semmle/javascript/Concepts.qll Outdated Show resolved Hide resolved

JS: fix qldoc

9cfd0f5

erik-krogh approved these changes Feb 17, 2023

View reviewed changes

alexrford requested a review from RasmusWL February 17, 2023 11:52

alexrford added ready-for-internal-ci and removed ready-for-internal-ci labels Feb 20, 2023

RasmusWL approved these changes Feb 27, 2023

View reviewed changes

alexrford merged commit 7c85448 into github:main Feb 27, 2023

alexrford deleted the js-use-shared-cryptography branch February 27, 2023 12:26

		/** DEPRECATED. This predicate has been renamed to `getAnInput`. */
		deprecated final DataFlow::Node getInput() { result = super.getInput() }

JS: Use shared CryptographicOperation concept #12080

JS: Use shared CryptographicOperation concept #12080

Uh oh!

Conversation

alexrford commented Feb 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erik-krogh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RasmusWL left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexrford commented Feb 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

erik-krogh left a comment

Choose a reason for hiding this comment

Uh oh!

alexrford commented Feb 17, 2023

Uh oh!

RasmusWL left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

JS: Use shared `CryptographicOperation` concept #12080

JS: Use shared `CryptographicOperation` concept #12080

alexrford commented Feb 2, 2023 •

edited

Loading

alexrford commented Feb 15, 2023 •

edited

Loading