From 41d3222c68fc48147aa4a8cfc01ea75299ce3589 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nora=20Dimitrijevi=C4=87?= Date: Wed, 28 Jun 2023 12:36:20 +0200 Subject: [PATCH 1/6] Swift: introduce shared Concepts and crypto algorithm libraries. --- config/identical-files.json | 9 +- .../codeql/swift/internal/ConceptsImports.qll | 7 + .../codeql/swift/internal/ConceptsShared.qll | 175 ++++++++++++++++++ .../swift/security/CryptoAlgorithms.qll | 117 ++++++++++++ .../internal/CryptoAlgorithmNames.qll | 84 +++++++++ 5 files changed, 389 insertions(+), 3 deletions(-) create mode 100644 swift/ql/lib/codeql/swift/internal/ConceptsImports.qll create mode 100644 swift/ql/lib/codeql/swift/internal/ConceptsShared.qll create mode 100644 swift/ql/lib/codeql/swift/security/CryptoAlgorithms.qll create mode 100644 swift/ql/lib/codeql/swift/security/internal/CryptoAlgorithmNames.qll diff --git a/config/identical-files.json b/config/identical-files.json index ceed02ba5d63..624f3fd55def 100644 --- a/config/identical-files.json +++ b/config/identical-files.json @@ -501,12 +501,14 @@ "CryptoAlgorithms Python/JS/Ruby": [ "javascript/ql/lib/semmle/javascript/security/CryptoAlgorithms.qll", "python/ql/lib/semmle/python/concepts/CryptoAlgorithms.qll", - "ruby/ql/lib/codeql/ruby/security/CryptoAlgorithms.qll" + "ruby/ql/lib/codeql/ruby/security/CryptoAlgorithms.qll", + "swift/ql/lib/codeql/swift/security/CryptoAlgorithms.qll" ], "CryptoAlgorithmNames Python/JS/Ruby": [ "javascript/ql/lib/semmle/javascript/security/internal/CryptoAlgorithmNames.qll", "python/ql/lib/semmle/python/concepts/internal/CryptoAlgorithmNames.qll", - "ruby/ql/lib/codeql/ruby/security/internal/CryptoAlgorithmNames.qll" + "ruby/ql/lib/codeql/ruby/security/internal/CryptoAlgorithmNames.qll", + "swift/ql/lib/codeql/swift/security/internal/CryptoAlgorithmNames.qll" ], "SensitiveDataHeuristics Python/JS": [ "javascript/ql/lib/semmle/javascript/security/internal/SensitiveDataHeuristics.qll", @@ -543,7 +545,8 @@ "Concepts Python/Ruby/JS": [ "python/ql/lib/semmle/python/internal/ConceptsShared.qll", "ruby/ql/lib/codeql/ruby/internal/ConceptsShared.qll", - "javascript/ql/lib/semmle/javascript/internal/ConceptsShared.qll" + "javascript/ql/lib/semmle/javascript/internal/ConceptsShared.qll", + "swift/ql/lib/codeql/swift/internal/ConceptsShared.qll" ], "ApiGraphModels": [ "javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModels.qll", diff --git a/swift/ql/lib/codeql/swift/internal/ConceptsImports.qll b/swift/ql/lib/codeql/swift/internal/ConceptsImports.qll new file mode 100644 index 000000000000..2878e636e178 --- /dev/null +++ b/swift/ql/lib/codeql/swift/internal/ConceptsImports.qll @@ -0,0 +1,7 @@ +/** + * This file contains imports required for the Swift version of `ConceptsShared.qll`. + * Since they are language-specific, they can't be placed directly in that file, as it is shared between languages. + */ + +import codeql.swift.dataflow.DataFlow +import codeql.swift.security.CryptoAlgorithms as CryptoAlgorithms diff --git a/swift/ql/lib/codeql/swift/internal/ConceptsShared.qll b/swift/ql/lib/codeql/swift/internal/ConceptsShared.qll new file mode 100644 index 000000000000..e86b156e2042 --- /dev/null +++ b/swift/ql/lib/codeql/swift/internal/ConceptsShared.qll @@ -0,0 +1,175 @@ +/** + * Provides Concepts which are shared across languages. + * + * Each language has a language specific `Concepts.qll` file that can import the + * shared concepts from this file. A language can either re-export the concept directly, + * or can add additional member-predicates that are needed for that language. + * + * Moving forward, `Concepts.qll` will be the staging ground for brand new concepts from + * each language, but we will maintain a discipline of moving those concepts to + * `ConceptsShared.qll` ASAP. + */ + +private import ConceptsImports + +/** + * Provides models for cryptographic concepts. + * + * Note: The `CryptographicAlgorithm` class currently doesn't take weak keys into + * consideration for the `isWeak` member predicate. So RSA is always considered + * secure, although using a low number of bits will actually make it insecure. We plan + * to improve our libraries in the future to more precisely capture this aspect. + */ +module Cryptography { + class CryptographicAlgorithm = CryptoAlgorithms::CryptographicAlgorithm; + + class EncryptionAlgorithm = CryptoAlgorithms::EncryptionAlgorithm; + + class HashingAlgorithm = CryptoAlgorithms::HashingAlgorithm; + + class PasswordHashingAlgorithm = CryptoAlgorithms::PasswordHashingAlgorithm; + + /** + * A data-flow node that is an application of a cryptographic algorithm. For example, + * encryption, decryption, signature-validation. + * + * Extend this class to refine existing API models. If you want to model new APIs, + * extend `CryptographicOperation::Range` instead. + */ + class CryptographicOperation extends DataFlow::Node instanceof CryptographicOperation::Range { + /** Gets the algorithm used, if it matches a known `CryptographicAlgorithm`. */ + CryptographicAlgorithm getAlgorithm() { result = super.getAlgorithm() } + + /** Gets an input the algorithm is used on, for example the plain text input to be encrypted. */ + DataFlow::Node getAnInput() { result = super.getAnInput() } + + /** + * Gets the block mode used to perform this cryptographic operation. + * + * This predicate is only expected to have a result if two conditions hold: + * 1. The operation is an encryption operation, i.e. the algorithm used is an `EncryptionAlgorithm`, and + * 2. The algorithm used is a block cipher (not a stream cipher). + * + * If either of these conditions do not hold, then this predicate should have no result. + */ + BlockMode getBlockMode() { result = super.getBlockMode() } + } + + /** Provides classes for modeling new applications of a cryptographic algorithms. */ + module CryptographicOperation { + /** + * A data-flow node that is an application of a cryptographic algorithm. For example, + * encryption, decryption, signature-validation. + * + * Extend this class to model new APIs. If you want to refine existing API models, + * extend `CryptographicOperation` instead. + */ + abstract class Range extends DataFlow::Node { + /** Gets the algorithm used, if it matches a known `CryptographicAlgorithm`. */ + abstract CryptographicAlgorithm getAlgorithm(); + + /** Gets an input the algorithm is used on, for example the plain text input to be encrypted. */ + abstract DataFlow::Node getAnInput(); + + /** + * Gets the block mode used to perform this cryptographic operation. + * + * This predicate is only expected to have a result if two conditions hold: + * 1. The operation is an encryption operation, i.e. the algorithm used is an `EncryptionAlgorithm`, and + * 2. The algorithm used is a block cipher (not a stream cipher). + * + * If either of these conditions do not hold, then this predicate should have no result. + */ + abstract BlockMode getBlockMode(); + } + } + + /** + * A cryptographic block cipher mode of operation. This can be used to encrypt + * data of arbitrary length using a block encryption algorithm. + */ + class BlockMode extends string { + BlockMode() { + this = + [ + "ECB", "CBC", "GCM", "CCM", "CFB", "OFB", "CTR", "OPENPGP", + "XTS", // https://csrc.nist.gov/publications/detail/sp/800-38e/final + "EAX" // https://en.wikipedia.org/wiki/EAX_mode + ] + } + + /** Holds if this block mode is considered to be insecure. */ + predicate isWeak() { this = "ECB" } + + /** Holds if the given string appears to match this block mode. */ + bindingset[s] + predicate matchesString(string s) { s.toUpperCase().matches("%" + this + "%") } + } +} + +/** Provides classes for modeling HTTP-related APIs. */ +module Http { + /** Provides classes for modeling HTTP clients. */ + module Client { + /** + * A data-flow node that makes an outgoing HTTP request. + * + * Extend this class to refine existing API models. If you want to model new APIs, + * extend `Http::Client::Request::Range` instead. + */ + class Request extends DataFlow::Node instanceof Request::Range { + /** + * Gets a data-flow node that contributes to the URL of the request. + * Depending on the framework, a request may have multiple nodes which contribute to the URL. + */ + DataFlow::Node getAUrlPart() { result = super.getAUrlPart() } + + /** Gets a string that identifies the framework used for this request. */ + string getFramework() { result = super.getFramework() } + + /** + * Holds if this request is made using a mode that disables SSL/TLS + * certificate validation, where `disablingNode` represents the point at + * which the validation was disabled, and `argumentOrigin` represents the origin + * of the argument that disabled the validation (which could be the same node as + * `disablingNode`). + */ + predicate disablesCertificateValidation( + DataFlow::Node disablingNode, DataFlow::Node argumentOrigin + ) { + super.disablesCertificateValidation(disablingNode, argumentOrigin) + } + } + + /** Provides a class for modeling new HTTP requests. */ + module Request { + /** + * A data-flow node that makes an outgoing HTTP request. + * + * Extend this class to model new APIs. If you want to refine existing API models, + * extend `Http::Client::Request` instead. + */ + abstract class Range extends DataFlow::Node { + /** + * Gets a data-flow node that contributes to the URL of the request. + * Depending on the framework, a request may have multiple nodes which contribute to the URL. + */ + abstract DataFlow::Node getAUrlPart(); + + /** Gets a string that identifies the framework used for this request. */ + abstract string getFramework(); + + /** + * Holds if this request is made using a mode that disables SSL/TLS + * certificate validation, where `disablingNode` represents the point at + * which the validation was disabled, and `argumentOrigin` represents the origin + * of the argument that disabled the validation (which could be the same node as + * `disablingNode`). + */ + abstract predicate disablesCertificateValidation( + DataFlow::Node disablingNode, DataFlow::Node argumentOrigin + ); + } + } + } +} diff --git a/swift/ql/lib/codeql/swift/security/CryptoAlgorithms.qll b/swift/ql/lib/codeql/swift/security/CryptoAlgorithms.qll new file mode 100644 index 000000000000..7176c666c573 --- /dev/null +++ b/swift/ql/lib/codeql/swift/security/CryptoAlgorithms.qll @@ -0,0 +1,117 @@ +/** + * Provides classes modeling cryptographic algorithms, separated into strong and weak variants. + * + * The classification into strong and weak are based on Wikipedia, OWASP and Google (2021). + */ + +private import internal.CryptoAlgorithmNames + +/** + * A cryptographic algorithm. + */ +private newtype TCryptographicAlgorithm = + MkHashingAlgorithm(string name, boolean isWeak) { + isStrongHashingAlgorithm(name) and isWeak = false + or + isWeakHashingAlgorithm(name) and isWeak = true + } or + MkEncryptionAlgorithm(string name, boolean isWeak) { + isStrongEncryptionAlgorithm(name) and isWeak = false + or + isWeakEncryptionAlgorithm(name) and isWeak = true + } or + MkPasswordHashingAlgorithm(string name, boolean isWeak) { + isStrongPasswordHashingAlgorithm(name) and isWeak = false + or + isWeakPasswordHashingAlgorithm(name) and isWeak = true + } + +/** + * Gets the most specific `CryptographicAlgorithm` that matches the given `name`. + * A matching algorithm is one where the name of the algorithm matches the start of name, with allowances made for different name formats. + * In the case that multiple `CryptographicAlgorithm`s match the given `name`, the algorithm(s) with the longest name will be selected. This is intended to select more specific versions of algorithms when multiple versions could match - for example "SHA3_224" matches against both "SHA3" and "SHA3224", but the latter is a more precise match. + */ +bindingset[name] +private CryptographicAlgorithm getBestAlgorithmForName(string name) { + result = + max(CryptographicAlgorithm algorithm | + algorithm.getName() = + [ + name.toUpperCase(), // the full name + name.toUpperCase().regexpCapture("^([\\w]+)(?:-.*)?$", 1), // the name prior to any dashes or spaces + name.toUpperCase().regexpCapture("^([A-Z0-9]+)(?:(-|_).*)?$", 1) // the name prior to any dashes, spaces, or underscores + ].regexpReplaceAll("[-_ ]", "") // strip dashes, underscores, and spaces + | + algorithm order by algorithm.getName().length() + ) +} + +/** + * A cryptographic algorithm. + */ +abstract class CryptographicAlgorithm extends TCryptographicAlgorithm { + /** Gets a textual representation of this element. */ + string toString() { result = this.getName() } + + /** + * Gets the normalized name of this algorithm (upper-case, no spaces, dashes or underscores). + */ + abstract string getName(); + + /** + * Holds if the name of this algorithm is the most specific match for `name`. + * This predicate matches quite liberally to account for different ways of formatting algorithm names, e.g. using dashes, underscores, or spaces as separators, including or not including block modes of operation, etc. + */ + bindingset[name] + predicate matchesName(string name) { this = getBestAlgorithmForName(name) } + + /** + * Holds if this algorithm is weak. + */ + abstract predicate isWeak(); +} + +/** + * A hashing algorithm such as `MD5` or `SHA512`. + */ +class HashingAlgorithm extends MkHashingAlgorithm, CryptographicAlgorithm { + string name; + boolean isWeak; + + HashingAlgorithm() { this = MkHashingAlgorithm(name, isWeak) } + + override string getName() { result = name } + + override predicate isWeak() { isWeak = true } +} + +/** + * An encryption algorithm such as `DES` or `AES512`. + */ +class EncryptionAlgorithm extends MkEncryptionAlgorithm, CryptographicAlgorithm { + string name; + boolean isWeak; + + EncryptionAlgorithm() { this = MkEncryptionAlgorithm(name, isWeak) } + + override string getName() { result = name } + + override predicate isWeak() { isWeak = true } + + /** Holds if this algorithm is a stream cipher. */ + predicate isStreamCipher() { isStreamCipher(name) } +} + +/** + * A password hashing algorithm such as `PBKDF2` or `SCRYPT`. + */ +class PasswordHashingAlgorithm extends MkPasswordHashingAlgorithm, CryptographicAlgorithm { + string name; + boolean isWeak; + + PasswordHashingAlgorithm() { this = MkPasswordHashingAlgorithm(name, isWeak) } + + override string getName() { result = name } + + override predicate isWeak() { isWeak = true } +} diff --git a/swift/ql/lib/codeql/swift/security/internal/CryptoAlgorithmNames.qll b/swift/ql/lib/codeql/swift/security/internal/CryptoAlgorithmNames.qll new file mode 100644 index 000000000000..8bb63d97876a --- /dev/null +++ b/swift/ql/lib/codeql/swift/security/internal/CryptoAlgorithmNames.qll @@ -0,0 +1,84 @@ +/** + * Names of cryptographic algorithms, separated into strong and weak variants. + * + * The names are normalized: upper-case, no spaces, dashes or underscores. + * + * The names are inspired by the names used in real world crypto libraries. + * + * The classification into strong and weak are based on Wikipedia, OWASP and Google (2021). + */ + +/** + * Holds if `name` corresponds to a strong hashing algorithm. + */ +predicate isStrongHashingAlgorithm(string name) { + name = + [ + // see https://cryptography.io/en/latest/hazmat/primitives/cryptographic-hashes/#blake2 + // and https://www.blake2.net/ + "BLAKE2", "BLAKE2B", "BLAKE2S", + // see https://github.com/BLAKE3-team/BLAKE3 + "BLAKE3", + // + "DSA", "ED25519", "ES256", "ECDSA256", "ES384", "ECDSA384", "ES512", "ECDSA512", "SHA2", + "SHA224", "SHA256", "SHA384", "SHA512", "SHA3", "SHA3224", "SHA3256", "SHA3384", "SHA3512", + // see https://cryptography.io/en/latest/hazmat/primitives/cryptographic-hashes/#cryptography.hazmat.primitives.hashes.SHAKE128 + "SHAKE128", "SHAKE256", + // see https://cryptography.io/en/latest/hazmat/primitives/cryptographic-hashes/#sm3 + "SM3", + // see https://security.stackexchange.com/a/216297 + "WHIRLPOOL", + ] +} + +/** + * Holds if `name` corresponds to a weak hashing algorithm. + */ +predicate isWeakHashingAlgorithm(string name) { + name = + [ + "HAVEL128", "MD2", "MD4", "MD5", "PANAMA", "RIPEMD", "RIPEMD128", "RIPEMD256", "RIPEMD160", + "RIPEMD320", "SHA0", "SHA1" + ] +} + +/** + * Holds if `name` corresponds to a strong encryption algorithm. + */ +predicate isStrongEncryptionAlgorithm(string name) { + name = + [ + "AES", "AES128", "AES192", "AES256", "AES512", "AES-128", "AES-192", "AES-256", "AES-512", + "ARIA", "BLOWFISH", "BF", "ECIES", "CAST", "CAST5", "CAMELLIA", "CAMELLIA128", "CAMELLIA192", + "CAMELLIA256", "CAMELLIA-128", "CAMELLIA-192", "CAMELLIA-256", "CHACHA", "GOST", "GOST89", + "IDEA", "RABBIT", "RSA", "SEED", "SM4" + ] +} + +/** + * Holds if `name` corresponds to a weak encryption algorithm. + */ +predicate isWeakEncryptionAlgorithm(string name) { + name = + [ + "DES", "3DES", "DES3", "TRIPLEDES", "DESX", "TDEA", "TRIPLEDEA", "ARC2", "RC2", "ARC4", "RC4", + "ARCFOUR", "ARC5", "RC5" + ] +} + +/** + * Holds if `name` corresponds to a strong password hashing algorithm. + */ +predicate isStrongPasswordHashingAlgorithm(string name) { + name = ["ARGON2", "PBKDF2", "BCRYPT", "SCRYPT"] +} + +/** + * Holds if `name` corresponds to a weak password hashing algorithm. + */ +predicate isWeakPasswordHashingAlgorithm(string name) { name = "EVPKDF" } + +/** + * Holds if `name` corresponds to a stream cipher. + */ +predicate isStreamCipher(string name) { name = ["CHACHA", "RC4", "ARC4", "ARCFOUR", "RABBIT"] } From 66f3f8ec104158eb5b729764b19fcd9ab198bdac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nora=20Dimitrijevi=C4=87?= Date: Mon, 3 Jul 2023 14:05:52 +0200 Subject: [PATCH 2/6] WIP: query ported from python. --- .../CWE-327/BrokenCryptoAlgorithm.qhelp | 58 +++++++++++++++++++ .../Security/CWE-327/BrokenCryptoAlgorithm.ql | 30 ++++++++++ .../CWE-327/BrokenCryptoAlgorithm.swift | 15 +++++ 3 files changed, 103 insertions(+) create mode 100644 swift/ql/src/queries/Security/CWE-327/BrokenCryptoAlgorithm.qhelp create mode 100644 swift/ql/src/queries/Security/CWE-327/BrokenCryptoAlgorithm.ql create mode 100644 swift/ql/src/queries/Security/CWE-327/BrokenCryptoAlgorithm.swift diff --git a/swift/ql/src/queries/Security/CWE-327/BrokenCryptoAlgorithm.qhelp b/swift/ql/src/queries/Security/CWE-327/BrokenCryptoAlgorithm.qhelp new file mode 100644 index 000000000000..0266920d05a0 --- /dev/null +++ b/swift/ql/src/queries/Security/CWE-327/BrokenCryptoAlgorithm.qhelp @@ -0,0 +1,58 @@ + + + +

+ 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. +

+ + + + +

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

+ + + + +

+ 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. +

+ + + + + + +
  • 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: Rule + - Use strong approved cryptographic algorithms. +
    • +     + +     diff --git a/swift/ql/src/queries/Security/CWE-327/BrokenCryptoAlgorithm.ql b/swift/ql/src/queries/Security/CWE-327/BrokenCryptoAlgorithm.ql new file mode 100644 index 000000000000..edf41970ad46 --- /dev/null +++ b/swift/ql/src/queries/Security/CWE-327/BrokenCryptoAlgorithm.ql @@ -0,0 +1,30 @@ +/** + * @name Use of a broken or weak cryptographic algorithm + * @description Using broken or weak cryptographic algorithms can compromise security. + * @kind problem + * @problem.severity warning + * @security-severity 7.5 + * @precision high + * @id swift/weak-cryptographic-algorithm + * @tags security + * external/cwe/cwe-327 + */ + +import swift +import codeql.swift.security.Cryptography // TODO: or use a megamodule like Concepts, containing a Cryptography module? + +from + Cryptography::CryptographicOperation operation, Cryptography::CryptographicAlgorithm algorithm, + string msgPrefix +where + algorithm = operation.getAlgorithm() and + // `Cryptography::HashingAlgorithm` and `Cryptography::PasswordHashingAlgorithm` are + // handled by `py/weak-sensitive-data-hashing` + algorithm instanceof Cryptography::EncryptionAlgorithm and + ( + algorithm.isWeak() and + msgPrefix = "The cryptographic algorithm " + operation.getAlgorithm().getName() + ) + or + operation.getBlockMode().isWeak() and msgPrefix = "The block mode " + operation.getBlockMode() +select operation, msgPrefix + " is broken or weak, and should not be used." diff --git a/swift/ql/src/queries/Security/CWE-327/BrokenCryptoAlgorithm.swift b/swift/ql/src/queries/Security/CWE-327/BrokenCryptoAlgorithm.swift new file mode 100644 index 000000000000..da06226708e0 --- /dev/null +++ b/swift/ql/src/queries/Security/CWE-327/BrokenCryptoAlgorithm.swift @@ -0,0 +1,15 @@ +import CryptoKit +import Foundation + +let SECRET_KEY = SymmetricKey(size: .bits256) + +func sendEncrypted(plaintext: Data, to channel: Channel) { + channel.send(cipher.encrypt(plaintext)) // BAD: weak encryption +} + +func sendEncrypted(plaintext: Data, to channel: Channel) { + // GOOD: strong encryption + let sealedData = try! AES.GCM.seal(plaintext, using: SECRET_KEY, nonce: AES.GCM.Nonce()) + let encryptedContent = try! sealedData.combined! + channel.send(encryptedContent) +} From bad17e3ba49903ee66f583d890321569eef33192 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nora=20Dimitrijevi=C4=87?= Date: Wed, 5 Jul 2023 12:48:39 +0200 Subject: [PATCH 3/6] Swift: respond to documentation/comment/QL-style reviews. --- .../Security/CWE-327/BrokenCryptoAlgorithm.qhelp | 6 ++---- .../Security/CWE-327/BrokenCryptoAlgorithm.ql | 13 ++++--------- .../Security/CWE-327/BrokenCryptoAlgorithm.swift | 8 ++++---- 3 files changed, 10 insertions(+), 17 deletions(-) diff --git a/swift/ql/src/queries/Security/CWE-327/BrokenCryptoAlgorithm.qhelp b/swift/ql/src/queries/Security/CWE-327/BrokenCryptoAlgorithm.qhelp index 0266920d05a0..93a8f10b29aa 100644 --- a/swift/ql/src/queries/Security/CWE-327/BrokenCryptoAlgorithm.qhelp +++ b/swift/ql/src/queries/Security/CWE-327/BrokenCryptoAlgorithm.qhelp @@ -27,7 +27,7 @@

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

    @@ -49,9 +49,7 @@
  • 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: Rule - - Use strong approved cryptographic algorithms. +
  • OWASP: Cryptographic Storage Cheat Sheet - Algorithms.
    • diff --git a/swift/ql/src/queries/Security/CWE-327/BrokenCryptoAlgorithm.ql b/swift/ql/src/queries/Security/CWE-327/BrokenCryptoAlgorithm.ql index edf41970ad46..fd47b50f1996 100644 --- a/swift/ql/src/queries/Security/CWE-327/BrokenCryptoAlgorithm.ql +++ b/swift/ql/src/queries/Security/CWE-327/BrokenCryptoAlgorithm.ql @@ -11,17 +11,12 @@ */ import swift -import codeql.swift.security.Cryptography // TODO: or use a megamodule like Concepts, containing a Cryptography module? +import codeql.swift.security.Cryptography -from - Cryptography::CryptographicOperation operation, Cryptography::CryptographicAlgorithm algorithm, - string msgPrefix +from Cryptography::CryptographicOperation operation, string msgPrefix where - algorithm = operation.getAlgorithm() and - // `Cryptography::HashingAlgorithm` and `Cryptography::PasswordHashingAlgorithm` are - // handled by `py/weak-sensitive-data-hashing` - algorithm instanceof Cryptography::EncryptionAlgorithm and - ( + // Hashing algorithms are handled by `swift/weak-sensitive-data-hashing` + exists(Cryptography::EncryptionAlgorithm algorithm | algorithm = operation.getAlgorithm() | algorithm.isWeak() and msgPrefix = "The cryptographic algorithm " + operation.getAlgorithm().getName() ) diff --git a/swift/ql/src/queries/Security/CWE-327/BrokenCryptoAlgorithm.swift b/swift/ql/src/queries/Security/CWE-327/BrokenCryptoAlgorithm.swift index da06226708e0..64f921b6074d 100644 --- a/swift/ql/src/queries/Security/CWE-327/BrokenCryptoAlgorithm.swift +++ b/swift/ql/src/queries/Security/CWE-327/BrokenCryptoAlgorithm.swift @@ -3,13 +3,13 @@ import Foundation let SECRET_KEY = SymmetricKey(size: .bits256) -func sendEncrypted(plaintext: Data, to channel: Channel) { - channel.send(cipher.encrypt(plaintext)) // BAD: weak encryption +func encrypt(plaintext: Data) -> Data { + return cipher.encrypt(plaintext) // BAD: weak encryption } -func sendEncrypted(plaintext: Data, to channel: Channel) { +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! - channel.send(encryptedContent) + return encryptedContent } From 256723a33a46296fa4e572c13c0ed99983289345 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nora=20Dimitrijevi=C4=87?= Date: Wed, 5 Jul 2023 14:49:23 +0200 Subject: [PATCH 4/6] Swift: Cryptography concept library --- swift/ql/lib/codeql/swift/Concepts.qll | 7 +++++++ swift/ql/lib/codeql/swift/security/Cryptography.qll | 10 ++++++++++ .../queries/Security/CWE-327/BrokenCryptoAlgorithm.ql | 2 +- 3 files changed, 18 insertions(+), 1 deletion(-) create mode 100644 swift/ql/lib/codeql/swift/Concepts.qll create mode 100644 swift/ql/lib/codeql/swift/security/Cryptography.qll diff --git a/swift/ql/lib/codeql/swift/Concepts.qll b/swift/ql/lib/codeql/swift/Concepts.qll new file mode 100644 index 000000000000..e1b6aae67973 --- /dev/null +++ b/swift/ql/lib/codeql/swift/Concepts.qll @@ -0,0 +1,7 @@ +/** + * Re-exports abstract classes representing generic concepts such as file system + * access or system command execution, for which individual framework libraries + * provide concrete subclasses. + */ + +import codeql.swift.security.Cryptography as Cryptography diff --git a/swift/ql/lib/codeql/swift/security/Cryptography.qll b/swift/ql/lib/codeql/swift/security/Cryptography.qll new file mode 100644 index 000000000000..72773ed41c53 --- /dev/null +++ b/swift/ql/lib/codeql/swift/security/Cryptography.qll @@ -0,0 +1,10 @@ +/** + * Provides models for cryptographic concepts. + * + * Note: The `CryptographicAlgorithm` class currently doesn't take weak keys into + * consideration for the `isWeak` member predicate. So RSA is always considered + * secure, although using a low number of bits will actually make it insecure. We plan + * to improve our libraries in the future to more precisely capture this aspect. + */ + +import codeql.swift.internal.ConceptsShared::Cryptography diff --git a/swift/ql/src/queries/Security/CWE-327/BrokenCryptoAlgorithm.ql b/swift/ql/src/queries/Security/CWE-327/BrokenCryptoAlgorithm.ql index fd47b50f1996..0ce5bfbf4f10 100644 --- a/swift/ql/src/queries/Security/CWE-327/BrokenCryptoAlgorithm.ql +++ b/swift/ql/src/queries/Security/CWE-327/BrokenCryptoAlgorithm.ql @@ -11,7 +11,7 @@ */ import swift -import codeql.swift.security.Cryptography +import codeql.swift.Concepts from Cryptography::CryptographicOperation operation, string msgPrefix where From bdba63f4d00a49d6809783b6899fe274a193bae7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nora=20Dimitrijevi=C4=87?= Date: Wed, 12 Jul 2023 16:05:39 +0200 Subject: [PATCH 5/6] Swift: add stubs for cryptography library models --- .../frameworks/Cryptography/CommonCrypto.qll | 15 +++++++++++++++ .../swift/frameworks/Cryptography/CryptoKit.qll | 15 +++++++++++++++ .../swift/frameworks/Cryptography/CryptoSwift.qll | 15 +++++++++++++++ .../frameworks/Cryptography/Cryptography.qll | 7 +++++++ .../ql/lib/codeql/swift/frameworks/Frameworks.qll | 1 + 5 files changed, 53 insertions(+) create mode 100644 swift/ql/lib/codeql/swift/frameworks/Cryptography/CommonCrypto.qll create mode 100644 swift/ql/lib/codeql/swift/frameworks/Cryptography/CryptoKit.qll create mode 100644 swift/ql/lib/codeql/swift/frameworks/Cryptography/CryptoSwift.qll create mode 100644 swift/ql/lib/codeql/swift/frameworks/Cryptography/Cryptography.qll diff --git a/swift/ql/lib/codeql/swift/frameworks/Cryptography/CommonCrypto.qll b/swift/ql/lib/codeql/swift/frameworks/Cryptography/CommonCrypto.qll new file mode 100644 index 000000000000..c00e7937af11 --- /dev/null +++ b/swift/ql/lib/codeql/swift/frameworks/Cryptography/CommonCrypto.qll @@ -0,0 +1,15 @@ +/** Provides models and concepts from the CommonCrypto library */ + +private import swift +private import codeql.swift.dataflow.DataFlow +private import codeql.swift.security.Cryptography + +private class CommonCryptoCallNode extends CryptographicOperation::Range { + CommonCryptoCallNode() { none() } + + override CryptographicAlgorithm getAlgorithm() { none() } + + override DataFlow::Node getAnInput() { none() } + + override BlockMode getBlockMode() { none() } +} diff --git a/swift/ql/lib/codeql/swift/frameworks/Cryptography/CryptoKit.qll b/swift/ql/lib/codeql/swift/frameworks/Cryptography/CryptoKit.qll new file mode 100644 index 000000000000..dfe0d5951e9b --- /dev/null +++ b/swift/ql/lib/codeql/swift/frameworks/Cryptography/CryptoKit.qll @@ -0,0 +1,15 @@ +/** Provides models and concepts from the CryptoKit library */ + +private import swift +private import codeql.swift.dataflow.DataFlow +private import codeql.swift.security.Cryptography + +private class CryptoKitCallNode extends CryptographicOperation::Range { + CryptoKitCallNode() { none() } + + override CryptographicAlgorithm getAlgorithm() { none() } + + override DataFlow::Node getAnInput() { none() } + + override BlockMode getBlockMode() { none() } +} diff --git a/swift/ql/lib/codeql/swift/frameworks/Cryptography/CryptoSwift.qll b/swift/ql/lib/codeql/swift/frameworks/Cryptography/CryptoSwift.qll new file mode 100644 index 000000000000..bd289abc6cc1 --- /dev/null +++ b/swift/ql/lib/codeql/swift/frameworks/Cryptography/CryptoSwift.qll @@ -0,0 +1,15 @@ +/** Provides models and concepts from the CryptoSwift library */ + +private import swift +private import codeql.swift.dataflow.DataFlow +private import codeql.swift.security.Cryptography + +private class CryptoSwiftCallNode extends CryptographicOperation::Range { + CryptoSwiftCallNode() { none() } + + override CryptographicAlgorithm getAlgorithm() { none() } + + override DataFlow::Node getAnInput() { none() } + + override BlockMode getBlockMode() { none() } +} diff --git a/swift/ql/lib/codeql/swift/frameworks/Cryptography/Cryptography.qll b/swift/ql/lib/codeql/swift/frameworks/Cryptography/Cryptography.qll new file mode 100644 index 000000000000..f3ce3f24c4bc --- /dev/null +++ b/swift/ql/lib/codeql/swift/frameworks/Cryptography/Cryptography.qll @@ -0,0 +1,7 @@ +/** + * This file imports all models of Cryptography-related frameworks and libraries. + */ + +import CryptoKit +import CryptoSwift +import CommonCrypto diff --git a/swift/ql/lib/codeql/swift/frameworks/Frameworks.qll b/swift/ql/lib/codeql/swift/frameworks/Frameworks.qll index 4d00ea7cbc9c..2c323a7beaa8 100644 --- a/swift/ql/lib/codeql/swift/frameworks/Frameworks.qll +++ b/swift/ql/lib/codeql/swift/frameworks/Frameworks.qll @@ -6,3 +6,4 @@ private import Alamofire.Alamofire private import StandardLibrary.StandardLibrary private import UIKit.UIKit private import Xml.Xml +private import Cryptography.Cryptography From 9ab5cef985411db81edfcc284733e7c92f205e8f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nora=20Dimitrijevi=C4=87?= Date: Mon, 7 Aug 2023 16:09:03 +0200 Subject: [PATCH 6/6] WIP 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(). --- .../frameworks/Cryptography/CommonCrypto.qll | 117 +++++++++++++++++- 1 file changed, 114 insertions(+), 3 deletions(-) diff --git a/swift/ql/lib/codeql/swift/frameworks/Cryptography/CommonCrypto.qll b/swift/ql/lib/codeql/swift/frameworks/Cryptography/CommonCrypto.qll index c00e7937af11..5b8b601c5f20 100644 --- a/swift/ql/lib/codeql/swift/frameworks/Cryptography/CommonCrypto.qll +++ b/swift/ql/lib/codeql/swift/frameworks/Cryptography/CommonCrypto.qll @@ -1,15 +1,126 @@ -/** Provides models and concepts from the CommonCrypto library */ +/** Provides models and concepts from the CommonCrypto C library */ private import swift private import codeql.swift.dataflow.DataFlow +private import codeql.swift.dataflow.ExternalFlow private import codeql.swift.security.Cryptography -private class CommonCryptoCallNode extends CryptographicOperation::Range { - CommonCryptoCallNode() { none() } +/** + * A model for CommonCrypto functions that permit taint flow. + */ +private class CommonCryptoSummaries extends SummaryModelCsv { + override predicate row(string row) { + row = + [ + // "namespace; type; subtypes; name; signature; ext; input; output; kind" + ";;false;CCCryptorCreate(_:_:_:_:_:_:_:);;;Argument[1];Argument[6];value", + ";;false;CCCryptorCreateFromData(_:_:_:_:_:_:_:_:_:_:);;;Argument[1];Argument[8];value", + ";;false;CCCryptorCreateWithMode(_:_:_:_:_:_:_:_:_:_:_:_:);;;Argument[1..2];Argument[10];value", + ] + } +} +/** A data flow node for the output of a CommonCrypto encryption/decryption operation */ +abstract private class CommonCryptoOutputNode extends CryptographicOperation::Range { override CryptographicAlgorithm getAlgorithm() { none() } override DataFlow::Node getAnInput() { none() } override BlockMode getBlockMode() { none() } } + +/** + * A declaration of a constant representing a value of the + * `CCAlgorithm` C enum, such as `kCCAlgorithmAES128`. + */ +private class AlgorithmConstantDecl extends VarDecl { + AlgorithmConstantDecl() { this.getName().matches("kCCAlgorithm%") } + + string getAlgorithmName() { this.getName() = "kCCAlgorithm" + result } + + CryptographicAlgorithm asAlgorithm() { result.matchesName(this.getAlgorithmName()) } +} + +/** + * Data flow configuration from a `CCAlgorithm` constant (or `CCAlgorithm`-bearing `CCCryptorRef`) + * to a call where it appears as an argument, e.g. `CCCryptorCreate(…)`, `CCCryptorUpdate(…)`, etc. + */ +private module AlgorithmUseConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node n) { + n.asExpr().(DeclRefExpr).getDecl() instanceof AlgorithmConstantDecl + } + + predicate isSink(DataFlow::Node n) { + exists(Argument arg | n.asExpr() = arg.getExpr() | + arg.getApplyExpr().getStaticTarget().getName().matches("CC%") + ) + } + + predicate isAdditionalFlowStep(DataFlow::Node n1, DataFlow::Node n2) { + // Flow through cast expressions like `CCAlgorithm(kCCAlgorithmAES128)`. + // TODO: turn this into a generally available flow step. + exists(NumericalCastExpr call | + n1.asExpr() = call.getArgument(0).getExpr() and + n2.asExpr() = call + ) + } +} + +private module AlgorithmUse = DataFlow::Global; + +/** + * A call expression that uses a `CCAlgorithm` constant through an argument, e.g. `CCryptorCreate(…)`. + */ +private class AlgorithmUser extends CallExpr { + AlgorithmConstantDecl alg; + + AlgorithmUser() { + exists(DataFlow::Node src, DataFlow::Node snk | AlgorithmUse::flow(src, snk) | + src.asExpr().(DeclRefExpr).getDecl() = alg and + snk.asExpr() = this.getAnArgument().getExpr() + ) + } + + CryptographicAlgorithm getAlgorithm() { result = alg.asAlgorithm() } +} + +/** + * A call to an auto-generated numerical cast initializer, e.g. `CCAlgorithm(kCCAlgorithmAES128)`. + */ +private class NumericalCastExpr extends CallExpr { + NumericalCastExpr() { this.getStaticTarget() instanceof NumericalCastInitializer } +} + +/** + * The declaration of an auto-generated numerical cast initializer. + */ +private class NumericalCastInitializer extends Initializer { + NumericalCastInitializer() { + this.getInterfaceType().getCanonicalType() instanceof NumericalCastType + } +} + +/** + * The type of an auto-generated numerical cast initializer, which is a generic + * function type of the form ` (Self.Type) -> (T) -> Self`. + */ +private class NumericalCastType extends Type { + NumericalCastType() { + exists( + GenericFunctionType fntySelf, FunctionType fntyT, GenericTypeParamType genericSelf, + GenericTypeParamType genericT, MetatypeType metaSelf + | + this = fntySelf and + fntySelf = fntySelf.getCanonicalType() and + fntySelf.getNumberOfGenericParams() = 2 and + fntySelf.getGenericParam(0) = genericSelf and + fntySelf.getGenericParam(1) = genericT and + fntySelf.getNumberOfParamTypes() = 1 and + fntySelf.getParamType(0) = metaSelf and + fntySelf.getResult() = fntyT and + fntyT.getNumberOfParamTypes() = 1 and + fntyT.getParamType(0) = genericT and + fntyT.getResult() = genericSelf + ) + } +}