Swift: Risky or Broken Cryptographic Algorithm Query #13649

d10c · 2023-07-03T12:09:21Z

No description provided.

github-actions · 2023-07-03T12:10:20Z

QHelp previews:

swift/ql/src/queries/Security/CWE-327/BrokenCryptoAlgorithm.qhelp

Use of a broken or weak cryptographic algorithm

Using broken or weak cryptographic algorithms can leave data vulnerable to being decrypted or forged by an attacker.

Many cryptographic algorithms provided by cryptography libraries are known to be weak, or flawed. Using such an algorithm means that encrypted or hashed data is less secure than it appears to be.

This query alerts on any use of a weak cryptographic algorithm, that is not a hashing algorithm. Use of broken or weak cryptographic hash functions are handled by the swift/weak-sensitive-data-hashing query.

Recommendation

Ensure that you use a strong, modern cryptographic algorithm, such as AES-256 or RSA-2048.

Example

The following code uses the library to encrypt some secret data. When you create a cipher using you must specify the encryption algorithm to use. The first example uses , which is an older algorithm that is now considered weak. The second example uses , which is a stronger modern algorithm.

import CryptoKit
import Foundation

let SECRET_KEY = SymmetricKey(size: .bits256)

func encrypt(plaintext: Data) -> Data {
    return cipher.encrypt(plaintext) // BAD: weak encryption
}

func encrypt(plaintext: Data) -> Data {
    // GOOD: strong encryption
    let sealedData = try! AES.GCM.seal(plaintext, using: SECRET_KEY, nonce: AES.GCM.Nonce())
    let encryptedContent = try! sealedData.combined!
    return encryptedContent
}

References

NIST, FIPS 140 Annex a: Approved Security Functions.
NIST, SP 800-131A: Transitions: Recommendation for Transitioning the Use of Cryptographic Algorithms and Key Lengths.
OWASP: Cryptographic Storage Cheat Sheet - Algorithms.
Common Weakness Enumeration: CWE-327.

swift/ql/src/queries/Security/CWE-327/BrokenCryptoAlgorithm.ql

geoffw0

This looks great, obviously we need to figure out a couple of realistic test cases (and perhaps some less realistic ones as well).

swift/ql/src/queries/Security/CWE-327/BrokenCryptoAlgorithm.qhelp

swift/ql/src/queries/Security/CWE-327/BrokenCryptoAlgorithm.ql

geoffw0 · 2023-07-03T17:28:56Z

swift/ql/src/queries/Security/CWE-327/BrokenCryptoAlgorithm.ql

+    msgPrefix = "The cryptographic algorithm " + operation.getAlgorithm().getName()
+  )
+  or
+  operation.getBlockMode().isWeak() and msgPrefix = "The block mode " + operation.getBlockMode()


There may be some overlap in results here with the swift/ecb-encryption query - though that query currently only models a very narrow range of algorithms and does not use heuristics. We might want to sort this out later. But I suggest we make it a follow-up issue and don't let it distract from finishing the swift/weak-cryptographic-algorithm query.

swift/ql/src/queries/Security/CWE-327/BrokenCryptoAlgorithm.swift

geoffw0 · 2023-07-03T17:37:22Z

swift/ql/src/queries/Security/CWE-327/BrokenCryptoAlgorithm.swift

+let SECRET_KEY = SymmetricKey(size: .bits256)
+
+func sendEncrypted(plaintext: Data, to channel: Channel) {
+    channel.send(cipher.encrypt(plaintext)) // BAD: weak encryption


Is cipher placeholder?

Also is Channel a Swift class, third party library, or something we're just implying the software of the example has?

It's just a placeholder, there's nothing quite equivalent in Swift's standard library, which prefers Actors for cross-thread communication, so I'll just replace this with a return since this query does not care about where ciphertext ends up getting used.

geoffw0 · 2023-07-04T09:10:27Z

swift/ql/lib/codeql/swift/internal/ConceptsShared.qll

+     * Extend this class to model new APIs. If you want to refine existing API models,
+     * extend `CryptographicOperation` instead.
+     */
+    abstract class Range extends DataFlow::Node {


Are we missing one or more implementations extending CryptographicOperation::Range?

Yes, that's the extension point.

Btw, do you know if it's possible to specify them as Models-as-Data CSVs? I'm a bit unfamiliar with the API for those.

I guess you could define them as sources or sinks (by extending SourceModelCsv / SinkModelCsv) and then use the sourceNode / sinkNode predicate to select the corresponding DataFlow::Nodes. Morally I'm not sure they're quite the same thing as sources or sinks though.

geoffw0 · 2023-08-02T17:41:08Z

Tests for this query on the CommonCrypto library here: #13870

I'm hoping to extend the dataflow to go from algorithm constants to the outputs of CommonCrypto encryption/decryption functions, through the CSV summaries, but I need to write test cases to make sure this actually works. Next steps: - Declare en/decryption outputs as subtypes of CommonCryptoOutputNode - Add test cases - Add also block mode constants to the source, so that they may be returned by getBlockMode().

swift/ql/lib/codeql/swift/frameworks/Cryptography/CommonCrypto.qll

+  }
+}
+
+private module AlgorithmUse = DataFlow::Global<AlgorithmUseConfig>;


swift/ql/lib/codeql/swift/frameworks/Cryptography/CommonCrypto.qll

+/**
+ * A call expression that uses a `CCAlgorithm` constant through an argument, e.g. `CCryptorCreate(…)`.
+ */
+private class AlgorithmUser extends CallExpr {


geoffw0 · 2023-08-07T18:46:00Z

swift/ql/lib/codeql/swift/frameworks/Cryptography/CommonCrypto.qll

+        ";;false;CCCryptorCreate(_:_:_:_:_:_:_:);;;Argument[1];Argument[6];value",
+        ";;false;CCCryptorCreateFromData(_:_:_:_:_:_:_:_:_:_:);;;Argument[1];Argument[8];value",
+        ";;false;CCCryptorCreateWithMode(_:_:_:_:_:_:_:_:_:_:_:_:);;;Argument[1..2];Argument[10];value",


Suggested change

";;false;CCCryptorCreate(_:_:_:_:_:_:_:);;;Argument[1];Argument[6];value",

";;false;CCCryptorCreateFromData(_:_:_:_:_:_:_:_:_:_:);;;Argument[1];Argument[8];value",

";;false;CCCryptorCreateWithMode(_:_:_:_:_:_:_:_:_:_:_:_:);;;Argument[1..2];Argument[10];value",

";;false;CCCryptorCreate(_:_:_:_:_:_:_:);;;Argument[1];Argument[6];taint",

";;false;CCCryptorCreateFromData(_:_:_:_:_:_:_:_:_:_:);;;Argument[1];Argument[8];taint",

";;false;CCCryptorCreateWithMode(_:_:_:_:_:_:_:_:_:_:_:_:);;;Argument[1..2];Argument[11];taint",

I believe this should be (at most) taint flow, because someone whoever controls the algorithm choice substantially controls cryptorRef - but cryptorRef is not itself just a copy of the algorithm argument (which would be value flow).

You'll have to make AlgorithmUse a TaintTracking::Global flow however.

geoffw0 · 2023-08-07T18:56:35Z

swift/ql/lib/codeql/swift/frameworks/Cryptography/CommonCrypto.qll

+
+  predicate isAdditionalFlowStep(DataFlow::Node n1, DataFlow::Node n2) {
+    // Flow through cast expressions like `CCAlgorithm(kCCAlgorithmAES128)`.
+    // TODO: turn this into a generally available flow step.


As I understand it there's a slight risk that NumericalCastType matches something that isn't truly a cast. But I imagine this will be very worthwhile overall. 👍

The trouble is it really should have it's own tests, so it might be better done as a separate PR.

... do you mind if I grab your implementation of NumericalCastType and do the generally available step myself? It looks like having this might fix an issue elsewhere (in the constant salt query tests).

Inspired by this and some similar problems we've run into, I've created #13946 . It does not contain your code, just a number of specific models for Numeric and similar classes (which may cover your needs, as really you're just dealing with Int / UInt32). In any case we could add your model to Numeric.qll quite easily now.

Note to future developer working on this query: the isAdditionalFlowStep and associated code should be removed from this PR. We've evolved this code and got these cases by default now.

geoffw0 · 2023-08-07T19:05:01Z

swift/ql/lib/codeql/swift/frameworks/Cryptography/CommonCrypto.qll

+}
+
+/** A data flow node for the output of a CommonCrypto encryption/decryption operation */
+abstract private class CommonCryptoOutputNode extends CryptographicOperation::Range {


I think this should just explicitly model CCCrypt and CCCryptorUpdate? It looks like you have everything you need to figure out which algorithm is used (at which point you should begin to get results on the tests), and I guess similar logic for the block mode.

d10c added 2 commits June 28, 2023 13:42

Swift: introduce shared Concepts and crypto algorithm libraries.

41d3222

WIP: query ported from python.

66f3f8e

github-actions bot added documentation Swift labels Jul 3, 2023

github-advanced-security bot found potential problems Jul 3, 2023

View reviewed changes

swift/ql/src/queries/Security/CWE-327/BrokenCryptoAlgorithm.ql Fixed Show fixed Hide fixed

geoffw0 reviewed Jul 3, 2023

View reviewed changes

geoffw0 reviewed Jul 4, 2023

View reviewed changes

d10c added 3 commits July 5, 2023 12:48

Swift: respond to documentation/comment/QL-style reviews.

bad17e3

Swift: Cryptography concept library

256723a

Swift: add stubs for cryptography library models

bdba63f

geoffw0 mentioned this pull request Aug 2, 2023

Swift: CommonCrypto test cases for the BrokenCryptoAlgorithm query #13870

Merged

github-advanced-security bot found potential problems Aug 7, 2023

View reviewed changes

geoffw0 reviewed Aug 7, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Swift: Risky or Broken Cryptographic Algorithm Query #13649

Swift: Risky or Broken Cryptographic Algorithm Query #13649

d10c commented Jul 3, 2023

github-actions bot commented Jul 3, 2023 •

edited

Loading

Use of a broken or weak cryptographic algorithm

Recommendation

Example

References

geoffw0 left a comment

geoffw0 Jul 3, 2023

geoffw0 Jul 3, 2023

d10c Jul 5, 2023

geoffw0 Jul 4, 2023

d10c Jul 4, 2023

geoffw0 Jul 4, 2023

geoffw0 commented Aug 2, 2023

geoffw0 Aug 7, 2023

geoffw0 Aug 7, 2023

geoffw0 Aug 9, 2023

geoffw0 Aug 10, 2023

geoffw0 Oct 13, 2023

geoffw0 Aug 7, 2023

Swift: Risky or Broken Cryptographic Algorithm Query #13649

Are you sure you want to change the base?

Swift: Risky or Broken Cryptographic Algorithm Query #13649

Conversation

d10c commented Jul 3, 2023

github-actions bot commented Jul 3, 2023 • edited Loading

Use of a broken or weak cryptographic algorithm

Recommendation

Example

References

geoffw0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geoffw0 commented Aug 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 3, 2023 •

edited

Loading