[JS] cwe-327 (Weak or vulnerable cryptography usage) added by monkey-junkie · Pull Request #3977 · github/codeql

monkey-junkie · 2020-07-26T19:19:28Z

Hi team!
I've wrote detection of usage of weak block cipher algorithms like block ciphers in ECB mode (known as chosen plaintext attack) and for common crypto flaw - static IV for block ciphers.
Either of such flaws could led to sensitive data leak.

More information:

OWASP - https://owasp.org/www-community/vulnerabilities/Using_a_broken_or_risky_cryptographic_algorithm
OWASP cheatsheet - https://cheatsheetseries.owasp.org/cheatsheets/Cryptographic_Storage_Cheat_Sheet.html
Real world example - https://www.ambionics.io/blog/prestashop-privilege-escalation
CPA - https://en.wikipedia.org/wiki/Chosen-plaintext_attack

intrigus-lgtm · 2020-07-26T19:51:37Z

Is this similar to https://github.com/github/codeql/blob/master/javascript/ql/src/Security/CWE-327/BrokenCryptoAlgorithm.qhelp and https://github.com/github/codeql/blob/master/javascript/ql/src/Security/CWE-327/BrokenCryptoAlgorithm.ql ?

monkey-junkie · 2020-07-28T22:39:32Z

Is this similar to https://github.com/github/codeql/blob/master/javascript/ql/src/Security/CWE-327/BrokenCryptoAlgorithm.qhelp and https://github.com/github/codeql/blob/master/javascript/ql/src/Security/CWE-327/BrokenCryptoAlgorithm.ql ?

Hi!
Not exactly. Proposed query could be used for detection of incorrect usage of secure algorithms (for example, AES-128 ECB which could be vulnerable for CPA), when mentioned query is about known-weak algorithms.
Maybe, it should be more appropriate to merge proposed query with https://github.com/github/codeql/blob/master/javascript/ql/src/Security/CWE-327/BrokenCryptoAlgorithm.ql

asgerf

Hi @monkey-junkie, thanks for the contribution! This looks really interesting.

I'm not an expert in cryptography, but I think I understand the chosen-plaintext attack vector, and it would be great to have a query for that.

So far I just have a few initial comments and questions.

asgerf · 2020-07-30T11:33:28Z

javascript/ql/src/experimental/Security/CWE-327/BrokenSymmetricCrypto.ql

+      ivBlock = call.(InvokeNode).getArgument(2).getALocalSource().getBasicBlock() and
+      alg = call.(InvokeNode).getArgument(0).toString()
+    |
+      createCihperBlock.length() != ivBlock.length() and


This doesn't looks right. The BasicBlock class has nothing to do with cryptography; it refers to the concept from compilers (see wikipedia page). I don't see why its length should matter.

Do I understand correctly that the OFB and CTR modes of AES become susceptible to chosen-plaintext attacks if subsequent encryption requests are made with the same IV? If so, you probably want to use getContainer instead of getBasicBlock:

call.getContainer() != call.getArgument(2).getALocalSource().getContainer()

Hi!
Yes, exactly, OFB and CTR with the static IV are vulnerable to CPA. I tried to use .legnth() method of BasicBlock to distinguish blocks with IV initialization and its actual usage. But now I can see, that I was wrong and I should use getContainer instead.
Thanks!

I've updated pr according all your comments, @asgerf please review :)

javascript/ql/src/experimental/Security/CWE-327/BrokenSymmetricCrypto.ql

…cCrypto.ql Co-authored-by: Asger F <asgerf@github.com>

intrigus-lgtm · 2020-07-30T23:35:13Z

@monkey-junkie In case you didn't know:

^ Pro tip: in the "Files changed" view, you have the option of adding review suggestions to a batch and commit them all at once.

asgerf

After studying this a little more I start to wonder if this should be moved to BrokenCryptoAlgorithm. The chosen-plaintext vector seemed really interesting at first, but now it seems to me these crypto algorithms are insecure even if the attacker can't choose the plaintext.

Two questions:

Is there ever a valid reason to use ECB when an outsider can't control the plaintext?
Is there ever a valid reason to reuse a given IV for multiple plaintexts?

If not, I think we can move these sinks into BrokenCryptoAlgorithm and just flag them regardless of whether the plaintext is tainted.

If there are valid reasons (and we could thus get FPs from BrokenCryptoAlgorithm), I'd appreciate it if the documentation is more specific to chosen-plaintext attacks rather than the generic "incorrect usage" term. For example, the @description could be:

Encrypting user-controlled plaintexts with ECB or with a reused IV enables an attacker to recover the secret key via chosen-plaintext attack.

(keep in mind I'm not a cryptographer so the above description may be wrong, in particular the part about actually recovering the key)

javascript/ql/src/experimental/Security/CWE-327/BrokenSymmetricCrypto.ql

erik-krogh · 2020-08-07T10:20:24Z

Is there ever a valid reason to reuse a given IV for multiple plaintexts?

I think this is the mistake that Sony made in the Playstation 3 (article).
I'm not sure if this was a chosen chosen-plaintext attack.

…cCrypto.ql Co-authored-by: Asger F <asgerf@github.com>

monkey-junkie · 2020-08-08T22:56:10Z

After studying this a little more I start to wonder if this should be moved to BrokenCryptoAlgorithm. The chosen-plaintext vector seemed really interesting at first, but now it seems to me these crypto algorithms are insecure even if the attacker can't choose the plaintext.

Two questions:

Is there ever a valid reason to use ECB when an outsider can't control the plaintext?

Is there ever a valid reason to reuse a given IV for multiple plaintexts?

If not, I think we can move these sinks into BrokenCryptoAlgorithm and just flag them regardless of whether the plaintext is tainted.

If there are valid reasons (and we could thus get FPs from BrokenCryptoAlgorithm), I'd appreciate it if the documentation is more specific to chosen-plaintext attacks rather than the generic "incorrect usage" term. For example, the @description could be:

Encrypting user-controlled plaintexts with ECB or with a reused IV enables an attacker to recover the secret key via chosen-plaintext attack.

(keep in mind I'm not a cryptographer so the above description may be wrong, in particular the part about actually recovering the key)

Hi!

Is there ever a valid reason to use ECB when an outsider can't control the plaintext?

No.

Is there ever a valid reason to reuse a given IV for multiple plaintexts?

According to RFC for CTR mode, "The same IV and key combination MUST NOT be used more than once" so, technically, it is safe to use static IV with unique encryption key per message, but unique encryption key is very rare case.

In sum, I think you are right and whe should move query to BrokenCryptoAlgorithm with additional check for key uniqueness (based on getContainer, like for unique IV in my ConstIVSink class).
So I'm going to add my updated pr to BrokenCryptoAlgorithm, if you dont mind.

asgerf · 2020-08-10T08:40:50Z

So I'm going to add my updated pr to BrokenCryptoAlgorithm, if you dont mind.

Thanks. Please go ahead.

asgerf · 2022-05-09T13:27:46Z

Closing the PR due to inactivity, but feel free to reopen if you wish to continue work on it.

Overall I think the ECB and reused-IV stuff could be useful to have in query suite in some form, so I'd be happy to see it re-opened. In retrospect I wish I had not suggested moving it into the non-experimental BrokenCryptoAlgorithm query because the requirements are higher for such queries, and it's just more work to integrate with the abstraction we use internally. Merging as an experimental query would have been better than closing the PR unmerged like this.

[JS] cwe-327 added

8070900

monkey-junkie requested a review from a team as a code owner July 26, 2020 19:19

asgerf reviewed Jul 30, 2020

View reviewed changes

monkey-junkie and others added 3 commits July 31, 2020 03:02

Update javascript/ql/src/experimental/Security/CWE-327/BrokenSymmetri…

e45d9b4

…cCrypto.ql Co-authored-by: Asger F <asgerf@github.com>

Update javascript/ql/src/experimental/Security/CWE-327/BrokenSymmetri…

c2335c4

…cCrypto.ql Co-authored-by: Asger F <asgerf@github.com>

Update javascript/ql/src/experimental/Security/CWE-327/BrokenSymmetri…

d772991

…cCrypto.ql Co-authored-by: Asger F <asgerf@github.com>

johndoe12312 added 2 commits July 31, 2020 02:37

cwe-327 updates for pr

89fe5e7

unused BasicBlocks variables removed

ff636df

esbena added the JS label Aug 3, 2020

asgerf reviewed Aug 7, 2020

View reviewed changes

javascript/ql/src/experimental/Security/CWE-327/BrokenSymmetricCrypto.ql Outdated Show resolved Hide resolved

javascript/ql/src/experimental/Security/CWE-327/BrokenSymmetricCrypto.ql Outdated Show resolved Hide resolved

monkey-junkie and others added 2 commits August 9, 2020 01:56

Update javascript/ql/src/experimental/Security/CWE-327/BrokenSymmetri…

5837594

…cCrypto.ql Co-authored-by: Asger F <asgerf@github.com>

Update javascript/ql/src/experimental/Security/CWE-327/BrokenSymmetri…

a3e1468

…cCrypto.ql Co-authored-by: Asger F <asgerf@github.com>

adityasharad changed the base branch from master to main August 14, 2020 18:33

asgerf closed this May 9, 2022

Feb	MAR	Apr
	21
2025	2026	2027

Conversation

monkey-junkie commented Jul 26, 2020

Uh oh!

intrigus-lgtm commented Jul 26, 2020

Uh oh!

monkey-junkie commented Jul 28, 2020

Uh oh!

asgerf left a comment

Choose a reason for hiding this comment

Uh oh!

asgerf Jul 30, 2020

Choose a reason for hiding this comment

Uh oh!

monkey-junkie Jul 30, 2020

Choose a reason for hiding this comment

Uh oh!

monkey-junkie Jul 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

intrigus-lgtm commented Jul 30, 2020

Uh oh!

asgerf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

erik-krogh commented Aug 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

monkey-junkie commented Aug 8, 2020

Uh oh!

asgerf commented Aug 10, 2020

Uh oh!

asgerf commented May 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

monkey-junkie Jul 30, 2020 •

edited

Loading

erik-krogh commented Aug 7, 2020 •

edited

Loading