From 1efaedee330f1aa70aaf403e70261ec7a41d02b4 Mon Sep 17 00:00:00 2001 From: Matt Schwager Date: Thu, 18 Dec 2025 14:32:21 -0500 Subject: [PATCH 1/4] Add CodeQL query format check to CI --- .github/workflows/test.yml | 6 ++---- Makefile | 21 +++++++++++++++++++++ README.md | 17 +++++++++++------ scripts/install_all.sh | 9 --------- 4 files changed, 34 insertions(+), 19 deletions(-) create mode 100644 Makefile delete mode 100644 scripts/install_all.sh diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 058aef0..e64326e 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -16,7 +16,5 @@ jobs: version: '2.23.8' platform: 'linux64' checksum: 'e61bc8aa8d86d45acd9d1c36629a12bbfb3365cd07a31666a2ebc91c6a1940b2' - - run: | - codeql test run --threads=0 ./cpp/test/ - codeql test run --threads=0 ./go/test/ - codeql test run --threads=0 ./java/test/ + - run: make format-check + - run: make test diff --git a/Makefile b/Makefile new file mode 100644 index 0000000..88679fe --- /dev/null +++ b/Makefile @@ -0,0 +1,21 @@ +test: + find . -iname "test" -type d -maxdepth 2 -mindepth 2 -print0 | \ + xargs -0 codeql test run --threads=0 + +format: + find . \( -iname '*.ql' -o -iname '*.qll' \) -print0 | \ + xargs -0 codeql query format --in-place + +format-check: + find . \( -iname '*.ql' -o -iname '*.qll' \) -print0 | \ + xargs -0 codeql query format --check-only + +pack-install: + find . -iname "qlpack.yml" -exec \ + sh -c 'codeql pack install $$(dirname "$$1")' sh {} \; + +pack-upgrade: + find . -iname "qlpack.yml" -exec \ + sh -c 'codeql pack upgrade $$(dirname "$$1")' sh {} \; + +.PHONY: test format format-check pack-install pack-upgrade diff --git a/README.md b/README.md index 559efba..660a339 100644 --- a/README.md +++ b/README.md @@ -103,16 +103,21 @@ codeql resolve packs | grep trailofbits #### Before committing Run tests: + +```sh +make test +``` + +Format queries: + ```sh -cd codeql-queries -codeql test run ./cpp/test -codeql test run ./go/test -codeql test run ./java/test +make format ``` -Update dependencies: +Install dependencies: + ```sh -bash ./scripts/install_all.sh +make install ``` Generate query tables and copy-paste it to README.md file diff --git a/scripts/install_all.sh b/scripts/install_all.sh deleted file mode 100644 index e959963..0000000 --- a/scripts/install_all.sh +++ /dev/null @@ -1,9 +0,0 @@ -#!/bin/bash - -cmd='QP="{}"; QPD="${QP%/*}"; echo "$QPD"; cd "$QPD"; codeql pack install' - -echo "Installing queries and libraries" -find . -name 'qlpack.yml' -path '*/src/*' -exec sh -c "$cmd" \; - -echo "Installing tests" -find . -name 'qlpack.yml' -path '*/test/*' -exec sh -c "$cmd" \; From b3b8f8f95bf1a149fdca9a8e16593caa34f4678b Mon Sep 17 00:00:00 2001 From: Matt Schwager Date: Thu, 18 Dec 2025 14:33:15 -0500 Subject: [PATCH 2/4] Autoformat all .ql and .qll files --- cpp/lib/trailofbits/crypto/common.qll | 1 - cpp/lib/trailofbits/crypto/common/Csprng.qll | 6 +- .../crypto/common/CsprngInitializer.qll | 15 +- .../crypto/common/CustomAllocator.qll | 56 +-- .../trailofbits/crypto/common/ErrorCode.qll | 1 - .../crypto/common/HashFunction.qll | 55 +-- .../crypto/common/InitUpdateFinalPattern.qll | 52 +-- .../crypto/common/KeyDerivationFunction.qll | 24 +- .../crypto/common/StaticCryptoVariable.qll | 48 +- .../crypto/common/SymmetricCipherContext.qll | 31 +- cpp/lib/trailofbits/crypto/libraries.qll | 2 +- cpp/lib/trailofbits/crypto/mbedtls/bignum.qll | 12 +- cpp/lib/trailofbits/crypto/openssl/evp.qll | 425 +++++------------- cpp/lib/trailofbits/crypto/openssl/hash.qll | 41 +- cpp/lib/trailofbits/crypto/openssl/sha.qll | 81 +--- cpp/src/crypto/InvalidKeySize.ql | 44 +- cpp/src/crypto/MissingErrorHandling.ql | 46 +- cpp/src/crypto/RandomBufferTooSmall.ql | 14 +- cpp/src/crypto/StaticKeyFlow.ql | 4 +- cpp/src/crypto/StaticPasswordFlow.ql | 4 +- cpp/src/crypto/UnbalancedBnCtx.ql | 17 +- cpp/src/crypto/UseOfLegacyAlgorithm.ql | 58 +-- .../AsyncUnsafeSignalHandler.ql | 84 ++-- cpp/src/security/CStrnFinder/CStrnFinder.ql | 267 ++++++----- .../NoNullTerminator/NoNullTerminator.ql | 98 ++-- .../CustomAllocator/CustomAllocatorCall.ql | 11 +- .../InitUpdateFinalTuple.ql | 7 +- .../crypto/StaticKeyAccess/StaticKeyAccess.ql | 4 +- .../StaticKeyLiteral/StaticKeyLiteral.ql | 7 +- .../MsgNotHashedBeforeSigVerfication.ql | 232 +++++----- .../security/FilePermsFlaws/FilePermsFlaws.ql | 44 +- .../MissingMinVersionTLS.ql | 171 ++++--- go/src/security/TrimMisuse/TrimMisuse.ql | 46 +- 33 files changed, 793 insertions(+), 1215 deletions(-) diff --git a/cpp/lib/trailofbits/crypto/common.qll b/cpp/lib/trailofbits/crypto/common.qll index 335c075..7116441 100644 --- a/cpp/lib/trailofbits/crypto/common.qll +++ b/cpp/lib/trailofbits/crypto/common.qll @@ -1,6 +1,5 @@ import common.InitUpdateFinalPattern import common.SymmetricCipherContext - import common.Csprng import common.ErrorCode import common.HashFunction diff --git a/cpp/lib/trailofbits/crypto/common/Csprng.qll b/cpp/lib/trailofbits/crypto/common/Csprng.qll index 36fc23b..b72ed36 100644 --- a/cpp/lib/trailofbits/crypto/common/Csprng.qll +++ b/cpp/lib/trailofbits/crypto/common/Csprng.qll @@ -1,20 +1,20 @@ import cpp - abstract class Csprng extends Function { abstract int getAStrongRandomnessSource(); abstract int getRequestedBytes(); } - class CsprngCall extends FunctionCall { Csprng csprng; // Ensures that the call target is an instance of Csprng. CsprngCall() { csprng = this.getTarget() } - Expr getAStrongRandomnessSource() { result = this.getArgument(csprng.getAStrongRandomnessSource()) } + Expr getAStrongRandomnessSource() { + result = this.getArgument(csprng.getAStrongRandomnessSource()) + } Expr getRequestedBytes() { result = this.getArgument(csprng.getRequestedBytes()) } } diff --git a/cpp/lib/trailofbits/crypto/common/CsprngInitializer.qll b/cpp/lib/trailofbits/crypto/common/CsprngInitializer.qll index 3931583..b66fc18 100644 --- a/cpp/lib/trailofbits/crypto/common/CsprngInitializer.qll +++ b/cpp/lib/trailofbits/crypto/common/CsprngInitializer.qll @@ -1,7 +1,6 @@ import cpp import StrongRandomnessSink - abstract class CsprngInitializer extends Function { // Returns the index of the seed argument (if it exists). abstract int getAStrongRandomnessSink(); @@ -9,24 +8,18 @@ abstract class CsprngInitializer extends Function { class CsprngInitializerCall extends FunctionCall { CsprngInitializer init; - - CsprngInitializerCall() { - this.getTarget() = init - } + + CsprngInitializerCall() { this.getTarget() = init } // Returns the seed argument. - Expr getAStrongRandomnessSink() { - result = this.getArgument(init.getAStrongRandomnessSink()) - } + Expr getAStrongRandomnessSink() { result = this.getArgument(init.getAStrongRandomnessSink()) } } // A type representing the seed argument of a CSPRNG initializer function. class CsprngInitializerSink extends StrongRandomnessSink { CsprngInitializerCall init; - CsprngInitializerSink() { - this = init.getAStrongRandomnessSink() - } + CsprngInitializerSink() { this = init.getAStrongRandomnessSink() } override string getDescription() { result = init.getTarget().getQualifiedName() + " parameter " + this.toString() diff --git a/cpp/lib/trailofbits/crypto/common/CustomAllocator.qll b/cpp/lib/trailofbits/crypto/common/CustomAllocator.qll index f95cdb2..df4ac66 100644 --- a/cpp/lib/trailofbits/crypto/common/CustomAllocator.qll +++ b/cpp/lib/trailofbits/crypto/common/CustomAllocator.qll @@ -1,45 +1,31 @@ import cpp private import semmle.code.cpp.controlflow.StackVariableReachability - /** * A custom allocator which returns a pointer to the allocated object. */ abstract class CustomAllocator extends Function { CustomDeallocator dealloc; - CustomDeallocator getDeallocator() { - result = dealloc - } + CustomDeallocator getDeallocator() { result = dealloc } // Override this and return the index of the pointer argument if the // allocated pointer is passed as an argument. - int getPointer() { - result = -1 - } + int getPointer() { result = -1 } } class CustomAllocatorCall extends FunctionCall { CustomAllocator alloc; - CustomAllocatorCall() { - this = alloc.getACallToThisFunction() - } + CustomAllocatorCall() { this = alloc.getACallToThisFunction() } Expr getPointer() { - if alloc.getPointer() < 0 then - result = this - else - result = this.getArgument(alloc.getPointer()) + if alloc.getPointer() < 0 then result = this else result = this.getArgument(alloc.getPointer()) } - CustomAllocator getAllocator() { - result = alloc - } + CustomAllocator getAllocator() { result = alloc } - CustomDeallocatorCall getADeallocatorCall() { - result.getTarget() = alloc.getDeallocator() - } + CustomDeallocatorCall getADeallocatorCall() { result.getTarget() = alloc.getDeallocator() } } /** @@ -52,27 +38,23 @@ abstract class CustomDeallocator extends Function { class CustomDeallocatorCall extends FunctionCall { CustomDeallocator dealloc; - CustomDeallocatorCall() { - this = dealloc.getACallToThisFunction() - } + CustomDeallocatorCall() { this = dealloc.getACallToThisFunction() } - Expr getPointer() { - result = this.getArgument(dealloc.getPointer()) - } + Expr getPointer() { result = this.getArgument(dealloc.getPointer()) } } class CustomAllocatorLeak extends StackVariableReachabilityWithReassignment { CustomAllocatorCall alloc; - CustomAllocatorLeak() { - this = "CustomAllocatorLeak" - } + CustomAllocatorLeak() { this = "CustomAllocatorLeak" } override predicate isSourceActual(ControlFlowNode node, StackVariable var) { // A source is an allocation of the variable. - node = alloc.getPointer() and ( + node = alloc.getPointer() and + ( // When alloc returns allocated object. - alloc.getPointer() = var.getAnAssignedValue() or + alloc.getPointer() = var.getAnAssignedValue() + or // When alloc takes a pointer as an argument the variable can be either a // pointer or an object v, in which case &v is passed to the function and // we need to invoke getAChild() to get a variable access. @@ -87,15 +69,13 @@ class CustomAllocatorLeak extends StackVariableReachabilityWithReassignment { override predicate isSinkActual(ControlFlowNode node, StackVariable var) { // A sink is a return statement not returning the allocated variable. - var.getFunction() = node.(ReturnStmt).getEnclosingFunction() - and not node.(ReturnStmt).getExpr() = var.getAnAccess() + var.getFunction() = node.(ReturnStmt).getEnclosingFunction() and + not node.(ReturnStmt).getExpr() = var.getAnAccess() } } class CustomAllocatorUseAfterFree extends StackVariableReachabilityWithReassignment { - CustomAllocatorUseAfterFree() { - this = "CustomAllocatorUseAfterFree" - } + CustomAllocatorUseAfterFree() { this = "CustomAllocatorUseAfterFree" } override predicate isSourceActual(ControlFlowNode node, StackVariable var) { node.(CustomDeallocatorCall).getPointer() = var.getAnAccess() @@ -103,8 +83,8 @@ class CustomAllocatorUseAfterFree extends StackVariableReachabilityWithReassignm override predicate isSinkActual(ControlFlowNode node, StackVariable var) { // A use is an access which is not a reassignment. - node = var.getAnAccess() and not - this.isAnAssignment(node, var) + node = var.getAnAccess() and + not this.isAnAssignment(node, var) } override predicate isBarrier(ControlFlowNode node, StackVariable var) { diff --git a/cpp/lib/trailofbits/crypto/common/ErrorCode.qll b/cpp/lib/trailofbits/crypto/common/ErrorCode.qll index 03e29e1..6e4eef7 100644 --- a/cpp/lib/trailofbits/crypto/common/ErrorCode.qll +++ b/cpp/lib/trailofbits/crypto/common/ErrorCode.qll @@ -5,7 +5,6 @@ import cpp */ abstract class ReturnsErrorCode extends Function { } - /** * A type modelling the return value from a function that returns an error or status code. */ diff --git a/cpp/lib/trailofbits/crypto/common/HashFunction.qll b/cpp/lib/trailofbits/crypto/common/HashFunction.qll index e3f57f8..5ba728e 100644 --- a/cpp/lib/trailofbits/crypto/common/HashFunction.qll +++ b/cpp/lib/trailofbits/crypto/common/HashFunction.qll @@ -1,6 +1,5 @@ import cpp - // A type modelling a (taint propagating) hash function. abstract class HashFunction extends TaintFunction { // Returns the index of the input data argument. @@ -16,15 +15,15 @@ abstract class HashFunction extends TaintFunction { override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) { input.isParameterDeref(this.getInput()) and ( - output.isParameterDeref(this.getAnOutput()) or - (this.returnsOutput() = true and output.isReturnValueDeref()) + output.isParameterDeref(this.getAnOutput()) + or + this.returnsOutput() = true and output.isReturnValueDeref() ) } } // A type modelling a (taint propagating) hash context. -class HashContext extends Expr { -} +class HashContext extends Expr { } // A type modelling a hash context initializer. abstract class HashContextInitializer extends Function { @@ -40,14 +39,13 @@ class HashContextInitializerCall extends FunctionCall { // Returns the hash context argument. HashContext getContext() { - (init.returnsContext() = true and result = this) or - (result = this.getArgument(init.getAContext())) + init.returnsContext() = true and result = this + or + result = this.getArgument(init.getAContext()) } // Returns the corresponding initializer function. - HashContextInitializer getInitializer() { - result = init - } + HashContextInitializer getInitializer() { result = init } } // A type modelling a hash context update function. @@ -70,40 +68,35 @@ class HashContextUpdaterCall extends FunctionCall { HashContextUpdater update; // Returns the hash context argument. - HashContext getContext() { - result = this.getArgument(update.getContext()) - } + HashContext getContext() { result = this.getArgument(update.getContext()) } // Returns the corresponding update function. - HashContextInitializer getUpdater() { - result = update - } - + HashContextInitializer getUpdater() { result = update } + // Returns an input data argument. - Expr getAnInput() { - result = this.getArgument(update.getAnInput()) - } + Expr getAnInput() { result = this.getArgument(update.getAnInput()) } } // A type modelling a hash context finalizer function. abstract class HashContextFinalizer extends TaintFunction { // Returns the index of the context argument. abstract int getContext(); - + // Returns the output digest argument (if it exists). abstract int getAnOutput(); // True iff the finalizer function returns a pointer or reference to the // output digest. abstract boolean returnsOutput(); - + // Since C++ does not have AdditionalTaintFlow we use TaintFunction to model // taint from input to output through the hash context. override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) { input.isParameterDeref(this.getContext()) and ( - output.isParameterDeref(this.getAnOutput()) or - (this.returnsOutput() = true and output.isReturnValueDeref()) + output.isParameterDeref(this.getAnOutput()) + or + this.returnsOutput() = true and output.isReturnValueDeref() ) } } @@ -112,19 +105,15 @@ class HashContextFinalizerCall extends FunctionCall { HashContextFinalizer final; // Returns the hash context argument. - HashContext getContext() { - result = this.getArgument(final.getContext()) - } + HashContext getContext() { result = this.getArgument(final.getContext()) } // Returns the corresponding finalizer function. - HashContextInitializer getFinalizer() { - result = final - } + HashContextInitializer getFinalizer() { result = final } // Returns an expression representing the output digest. Expr getAnOutput() { - (final.returnsOutput() = true and result = this) or - (result = this.getArgument(final.getAnOutput())) + final.returnsOutput() = true and result = this + or + result = this.getArgument(final.getAnOutput()) } } - diff --git a/cpp/lib/trailofbits/crypto/common/InitUpdateFinalPattern.qll b/cpp/lib/trailofbits/crypto/common/InitUpdateFinalPattern.qll index a6bbbda..08fc98f 100644 --- a/cpp/lib/trailofbits/crypto/common/InitUpdateFinalPattern.qll +++ b/cpp/lib/trailofbits/crypto/common/InitUpdateFinalPattern.qll @@ -8,22 +8,19 @@ abstract class InitFunction extends Function { abstract int getAContextArg(); // Override and return true if the function returns the initialized context. - predicate returnsContext() { - 1 = 0 - } + predicate returnsContext() { 1 = 0 } } // This type represents a call to the corresponding init function. class InitCall extends FunctionCall { InitFunction init; - InitCall() { - init = this.getTarget() - } + InitCall() { init = this.getTarget() } Expr getAContext() { - result = this.getArgument(init.getAContextArg()) or - (init.returnsContext() and result = this) + result = this.getArgument(init.getAContextArg()) + or + init.returnsContext() and result = this } } @@ -35,22 +32,19 @@ abstract class UpdateFunction extends Function { abstract int getAContextArg(); // Override and return true if the function returns the updated context. - predicate returnsContext() { - 1 = 0 - } + predicate returnsContext() { 1 = 0 } } // This type represents a call to the corresponding update function. class UpdateCall extends FunctionCall { UpdateFunction update; - UpdateCall() { - update = this.getTarget() - } + UpdateCall() { update = this.getTarget() } Expr getAContext() { - result = this.getArgument(update.getAContextArg()) or - (update.returnsContext() and result = this) + result = this.getArgument(update.getAContextArg()) + or + update.returnsContext() and result = this } } @@ -62,21 +56,18 @@ abstract class FinalFunction extends Function { abstract int getAContextArg(); // Override and return true if the function returns the initialized context. - predicate returnsContext() { - 1 = 0 - } + predicate returnsContext() { 1 = 0 } } class FinalCall extends FunctionCall { FinalFunction final; - FinalCall() { - final = this.getTarget() - } + FinalCall() { final = this.getTarget() } Expr getAContext() { - result = this.getArgument(final.getAContextArg()) or - (final.returnsContext() and result = this) + result = this.getArgument(final.getAContextArg()) + or + final.returnsContext() and result = this } } @@ -88,7 +79,6 @@ class FinalCall extends FunctionCall { // be multi-valued. I.e. a priori there may be more than one valid update or // finalizer for a given initialier.) class InitUpdateFinalTuple extends string { - bindingset[this] InitUpdateFinalTuple() { any() } @@ -98,15 +88,9 @@ class InitUpdateFinalTuple extends string { abstract FinalFunction getFinal(); - InitCall getAnInitCall() { - result = this.getInit().getACallToThisFunction() - } + InitCall getAnInitCall() { result = this.getInit().getACallToThisFunction() } - UpdateCall getAnUpdateCall() { - result = this.getUpdate().getACallToThisFunction() - } + UpdateCall getAnUpdateCall() { result = this.getUpdate().getACallToThisFunction() } - FinalCall getAFinalCall() { - result = this.getFinal().getACallToThisFunction() - } + FinalCall getAFinalCall() { result = this.getFinal().getACallToThisFunction() } } diff --git a/cpp/lib/trailofbits/crypto/common/KeyDerivationFunction.qll b/cpp/lib/trailofbits/crypto/common/KeyDerivationFunction.qll index 05c39e9..5d3b625 100644 --- a/cpp/lib/trailofbits/crypto/common/KeyDerivationFunction.qll +++ b/cpp/lib/trailofbits/crypto/common/KeyDerivationFunction.qll @@ -10,9 +10,7 @@ abstract class KeyDerivationFunctionWithPassword extends KeyDerivationFunction { // Returns the index of the password argument. abstract int getPassword(); - override int getAStrongPasswordSink() { - result = this.getPassword() - } + override int getAStrongPasswordSink() { result = this.getPassword() } } abstract class KeyDerivationFunctionWithSalt extends KeyDerivationFunction { @@ -20,31 +18,23 @@ abstract class KeyDerivationFunctionWithSalt extends KeyDerivationFunction { abstract int getSalt(); } -abstract class KeyDerivationFunctionWithPasswordAndSalt extends - KeyDerivationFunctionWithPassword, - KeyDerivationFunctionWithSalt { - -} +abstract class KeyDerivationFunctionWithPasswordAndSalt extends KeyDerivationFunctionWithPassword, + KeyDerivationFunctionWithSalt +{ } class KeyDerivationFunctionWithPasswordCall extends FunctionCall { KeyDerivationFunctionWithPassword kdf; - KeyDerivationFunctionWithPasswordCall() { - this.getTarget() = kdf - } + KeyDerivationFunctionWithPasswordCall() { this.getTarget() = kdf } - Expr getAStrongPasswordSink() { - result = this.getArgument(kdf.getAStrongPasswordSink()) - } + Expr getAStrongPasswordSink() { result = this.getArgument(kdf.getAStrongPasswordSink()) } } // A type representing the password argument of a key derivation function. class KeyDerivationFunctionSink extends StrongPasswordSink { KeyDerivationFunctionWithPasswordCall kdf; - KeyDerivationFunctionSink() { - this = kdf.getAStrongPasswordSink() - } + KeyDerivationFunctionSink() { this = kdf.getAStrongPasswordSink() } override string getDescription() { result = kdf.getTarget().getQualifiedName() + " parameter " + this.toString() diff --git a/cpp/lib/trailofbits/crypto/common/StaticCryptoVariable.qll b/cpp/lib/trailofbits/crypto/common/StaticCryptoVariable.qll index 373f4f8..cb683c5 100644 --- a/cpp/lib/trailofbits/crypto/common/StaticCryptoVariable.qll +++ b/cpp/lib/trailofbits/crypto/common/StaticCryptoVariable.qll @@ -6,69 +6,45 @@ private import semmle.code.cpp.dataflow.new.DataFlow class StaticKeyLiteral extends ArrayOrVectorAggregateLiteral { Variable var; - StaticKeyLiteral() { - this = var.getInitializer().getExpr() - } + StaticKeyLiteral() { this = var.getInitializer().getExpr() } - int getSize() { - result = this.getArraySize() - } + int getSize() { result = this.getArraySize() } - VariableAccess getAnAccess() { - result = var.getAnAccess() - } + VariableAccess getAnAccess() { result = var.getAnAccess() } } class StaticKeyAccess extends VariableAccess { StaticKeyLiteral key; - StaticKeyAccess() { - this = key.getAnAccess() - } + StaticKeyAccess() { this = key.getAnAccess() } - StaticKeyLiteral getKey() { - result = key - } + StaticKeyLiteral getKey() { result = key } } class StaticPasswordLiteral extends StringLiteral { - VariableAccess getAnAccess() { - result = this.getEnclosingVariable().getAnAccess() - } + VariableAccess getAnAccess() { result = this.getEnclosingVariable().getAnAccess() } } class StaticPasswordAccess extends VariableAccess { StaticPasswordLiteral password; - StaticPasswordAccess() { - this = password.getAnAccess() - } + StaticPasswordAccess() { this = password.getAnAccess() } - StaticPasswordLiteral getPassword() { - result = password - } + StaticPasswordLiteral getPassword() { result = password } } module StaticKeyConfig implements DataFlow::ConfigSig { - predicate isSource(DataFlow::Node source) { - source.asExpr() instanceof StaticKeyAccess - } + predicate isSource(DataFlow::Node source) { source.asExpr() instanceof StaticKeyAccess } - predicate isSink(DataFlow::Node sink) { - sink.asExpr() instanceof StrongRandomnessSink - } + predicate isSink(DataFlow::Node sink) { sink.asExpr() instanceof StrongRandomnessSink } } module StaticKeyFlow = DataFlow::Global; module StaticPasswordConfig implements DataFlow::ConfigSig { - predicate isSource(DataFlow::Node source) { - source.asExpr() instanceof StaticPasswordAccess - } + predicate isSource(DataFlow::Node source) { source.asExpr() instanceof StaticPasswordAccess } - predicate isSink(DataFlow::Node sink) { - sink.asExpr() instanceof StrongPasswordSink - } + predicate isSink(DataFlow::Node sink) { sink.asExpr() instanceof StrongPasswordSink } } module StaticPasswordFlow = DataFlow::Global; diff --git a/cpp/lib/trailofbits/crypto/common/SymmetricCipherContext.qll b/cpp/lib/trailofbits/crypto/common/SymmetricCipherContext.qll index 4756d3a..e03a871 100644 --- a/cpp/lib/trailofbits/crypto/common/SymmetricCipherContext.qll +++ b/cpp/lib/trailofbits/crypto/common/SymmetricCipherContext.qll @@ -13,41 +13,33 @@ abstract class SymmetricCipherInitWithKey extends SymmetricCipherInit { // Returns the index of the key argument. abstract int getKey(); - override int getAStrongRandomnessSink() { - result = this.getKey() - } + override int getAStrongRandomnessSink() { result = this.getKey() } } abstract class SymmetricCipherInitWithIv extends SymmetricCipherInit { // Returns the index of the IV argument. abstract int getIv(); - override int getAStrongRandomnessSink() { - result = this.getIv() - } + override int getAStrongRandomnessSink() { result = this.getIv() } } -abstract class SymmetricCipherInitWithKeyAndIv extends SymmetricCipherInitWithKey, SymmetricCipherInitWithIv { - override int getAStrongRandomnessSink() { - result = this.getKey() or result = this.getIv() - } +abstract class SymmetricCipherInitWithKeyAndIv extends SymmetricCipherInitWithKey, + SymmetricCipherInitWithIv +{ + override int getAStrongRandomnessSink() { result = this.getKey() or result = this.getIv() } } class SymmetricCipherInitCall extends InitCall { override SymmetricCipherInit init; // Returns an argument requiring strong randomness. - Expr getAStrongRandomnessSink() { - result = this.getArgument(init.getAStrongRandomnessSink()) - } + Expr getAStrongRandomnessSink() { result = this.getArgument(init.getAStrongRandomnessSink()) } } class SymmetricCipherInitSink extends StrongRandomnessSink { SymmetricCipherInitCall init; - SymmetricCipherInitSink() { - this = init.getAStrongRandomnessSink() - } + SymmetricCipherInitSink() { this = init.getAStrongRandomnessSink() } override string getDescription() { result = init.getTarget().getQualifiedName() + " parameter " + this.toString() @@ -56,8 +48,7 @@ class SymmetricCipherInitSink extends StrongRandomnessSink { // This abstract class represent a function which takes an initialized key or // cipher context as argument and updates it with more data. -abstract class SymmetricCipherUpdate extends UpdateFunction { -} +abstract class SymmetricCipherUpdate extends UpdateFunction { } class SymmetricCipherUpdaterCall extends UpdateCall { override SymmetricCipherUpdate update; @@ -65,10 +56,8 @@ class SymmetricCipherUpdaterCall extends UpdateCall { // This abstract class represent a function which takes an initialized key or // cipher context as argument and updates it with more data. -abstract class SymmetricCipherFinal extends FinalFunction { -} +abstract class SymmetricCipherFinal extends FinalFunction { } class SymmetricCipherFinalCall extends FinalCall { override SymmetricCipherFinal final; } - diff --git a/cpp/lib/trailofbits/crypto/libraries.qll b/cpp/lib/trailofbits/crypto/libraries.qll index 7c23f0a..f2eb0ec 100644 --- a/cpp/lib/trailofbits/crypto/libraries.qll +++ b/cpp/lib/trailofbits/crypto/libraries.qll @@ -1,2 +1,2 @@ import mbedtls -import openssl \ No newline at end of file +import openssl diff --git a/cpp/lib/trailofbits/crypto/mbedtls/bignum.qll b/cpp/lib/trailofbits/crypto/mbedtls/bignum.qll index b88e915..7efd696 100644 --- a/cpp/lib/trailofbits/crypto/mbedtls/bignum.qll +++ b/cpp/lib/trailofbits/crypto/mbedtls/bignum.qll @@ -8,18 +8,12 @@ class Mbedtls_mpi_init extends CustomAllocator { dealloc instanceof Mbedtls_mpi_free } - override int getPointer() { - result = 0 - } + override int getPointer() { result = 0 } } // void mbedtls_mpi_free( mbedtls_mpi *X ); class Mbedtls_mpi_free extends CustomDeallocator { - Mbedtls_mpi_free() { - this.getQualifiedName() = "mbedtls_mpi_free" - } + Mbedtls_mpi_free() { this.getQualifiedName() = "mbedtls_mpi_free" } - override int getPointer() { - result = 0 - } + override int getPointer() { result = 0 } } diff --git a/cpp/lib/trailofbits/crypto/openssl/evp.qll b/cpp/lib/trailofbits/crypto/openssl/evp.qll index 95b7861..e5edad1 100644 --- a/cpp/lib/trailofbits/crypto/openssl/evp.qll +++ b/cpp/lib/trailofbits/crypto/openssl/evp.qll @@ -3,283 +3,174 @@ import trailofbits.crypto.common // This file contains CodeQL models for the OpenSSL EVP API defined in // openssl/evp.h. - // int EVP_EncryptInit(EVP_CIPHER_CTX *ctx, const EVP_CIPHER *type, // const unsigned char *key, const unsigned char *iv); class EVP_EncryptInit extends SymmetricCipherInitWithKeyAndIv, ReturnsErrorCode { - EVP_EncryptInit() { - this.getQualifiedName() = "EVP_EncryptInit" - } + EVP_EncryptInit() { this.getQualifiedName() = "EVP_EncryptInit" } - int getCipher() { - result = 1 - } + int getCipher() { result = 1 } + override int getAContextArg() { result = 0 } - override int getAContextArg() { - result = 0 - } + override int getKey() { result = 2 } - override int getKey() { - result = 2 - } - - override int getIv() { - result = 3 - } + override int getIv() { result = 3 } } // int EVP_DecryptInit(EVP_CIPHER_CTX *ctx, const EVP_CIPHER *type, // const unsigned char *key, const unsigned char *iv); class EVP_DecryptInit extends SymmetricCipherInitWithKeyAndIv, ReturnsErrorCode { - EVP_DecryptInit() { - this.getQualifiedName() = "EVP_DecryptInit" - } + EVP_DecryptInit() { this.getQualifiedName() = "EVP_DecryptInit" } - override int getAContextArg() { - result = 0 - } + override int getAContextArg() { result = 0 } - override int getKey() { - result = 2 - } + override int getKey() { result = 2 } - override int getIv() { - result = 3 - } + override int getIv() { result = 3 } } // int EVP_EncryptInit_ex(EVP_CIPHER_CTX *ctx, const EVP_CIPHER *type, // ENGINE *impl, const unsigned char *key, const unsigned char *iv); class EVP_EncryptInit_ex extends SymmetricCipherInitWithKeyAndIv, ReturnsErrorCode { - EVP_EncryptInit_ex() { - this.getQualifiedName() = "EVP_EncryptInit_ex" - } + EVP_EncryptInit_ex() { this.getQualifiedName() = "EVP_EncryptInit_ex" } - int getCipher(){ - result = 1 - } + int getCipher() { result = 1 } - override int getAContextArg() { - result = 0 - } + override int getAContextArg() { result = 0 } - override int getKey() { - result = 3 - } + override int getKey() { result = 3 } - override int getIv() { - result = 4 - } + override int getIv() { result = 4 } } // int EVP_DecryptInit_ex(EVP_CIPHER_CTX *ctx, const EVP_CIPHER *type, // ENGINE *impl, const unsigned char *key, const unsigned char *iv); class EVP_DecryptInit_ex extends SymmetricCipherInitWithKeyAndIv, ReturnsErrorCode { - EVP_DecryptInit_ex() { - this.getQualifiedName() = "EVP_DecryptInit_ex" - } + EVP_DecryptInit_ex() { this.getQualifiedName() = "EVP_DecryptInit_ex" } - override int getAContextArg() { - result = 0 - } + override int getAContextArg() { result = 0 } - override int getKey() { - result = 3 - } + override int getKey() { result = 3 } - override int getIv() { - result = 4 - } + override int getIv() { result = 4 } } // int EVP_CipherInit(EVP_CIPHER_CTX *ctx, const EVP_CIPHER *type, // const unsigned char *key, const unsigned char *iv, int enc); class EVP_CipherInit extends SymmetricCipherInitWithKeyAndIv, ReturnsErrorCode { - EVP_CipherInit() { - this.getQualifiedName() = "EVP_CipherInit" - } + EVP_CipherInit() { this.getQualifiedName() = "EVP_CipherInit" } - override int getAContextArg() { - result = 0 - } + override int getAContextArg() { result = 0 } - override int getKey() { - result = 2 - } + override int getKey() { result = 2 } - override int getIv() { - result = 3 - } + override int getIv() { result = 3 } - int getMode() { - result = 4 - } + int getMode() { result = 4 } } // int EVP_CipherInit_ex(EVP_CIPHER_CTX *ctx, const EVP_CIPHER *type, // ENGINE *impl, const unsigned char *key, const unsigned char *iv, int enc); class EVP_CipherInit_ex extends SymmetricCipherInitWithKeyAndIv, ReturnsErrorCode { - EVP_CipherInit_ex() { - this.getQualifiedName() = "EVP_CipherInit_ex" - } + EVP_CipherInit_ex() { this.getQualifiedName() = "EVP_CipherInit_ex" } - override int getAContextArg() { - result = 0 - } + override int getAContextArg() { result = 0 } - override int getKey() { - result = 3 - } + override int getKey() { result = 3 } - override int getIv() { - result = 4 - } + override int getIv() { result = 4 } - int getMode() { - result = 5 - } + int getMode() { result = 5 } } - class BIO_set_cipher extends SymmetricCipherInitWithKeyAndIv, ReturnsErrorCode { - BIO_set_cipher() { - this.getQualifiedName() = "BIO_set_cipher" - } + BIO_set_cipher() { this.getQualifiedName() = "BIO_set_cipher" } - override int getAContextArg() { - result = 0 - } + override int getAContextArg() { result = 0 } - override int getKey() { - result = 2 - } + override int getKey() { result = 2 } - override int getIv() { - result = 3 - } + override int getIv() { result = 3 } - int getMode() { - result = 4 - } + int getMode() { result = 4 } } // int EVP_EncryptUpdate(EVP_CIPHER_CTX *ctx, unsigned char *out, // int *outl, const unsigned char *in, int inl); class EVP_EncryptUpdate extends SymmetricCipherUpdate, ReturnsErrorCode { - EVP_EncryptUpdate() { - this.getQualifiedName() = "EVP_EncryptUpdate" - } + EVP_EncryptUpdate() { this.getQualifiedName() = "EVP_EncryptUpdate" } - override int getAContextArg() { - result = 0 - } + override int getAContextArg() { result = 0 } } // int EVP_DecryptUpdate(EVP_CIPHER_CTX *ctx, unsigned char *out, // int *outl, const unsigned char *in, int inl); class EVP_DecryptUpdate extends SymmetricCipherUpdate, ReturnsErrorCode { - EVP_DecryptUpdate() { - this.getQualifiedName() = "EVP_DecryptUpdate" - } + EVP_DecryptUpdate() { this.getQualifiedName() = "EVP_DecryptUpdate" } - override int getAContextArg() { - result = 0 - } + override int getAContextArg() { result = 0 } } // int EVP_CipherUpdate(EVP_CIPHER_CTX *ctx, unsigned char *out, // int *outl, const unsigned char *in, int inl); class EVP_CipherUpdate extends SymmetricCipherUpdate, ReturnsErrorCode { - EVP_CipherUpdate() { - this.getQualifiedName() = "EVP_CipherUpdate" - } + EVP_CipherUpdate() { this.getQualifiedName() = "EVP_CipherUpdate" } - override int getAContextArg() { - result = 0 - } + override int getAContextArg() { result = 0 } } // int EVP_EncryptFinal(EVP_CIPHER_CTX *ctx, unsigned char *out, int *outl); class EVP_EncryptFinal extends SymmetricCipherFinal, ReturnsErrorCode { - EVP_EncryptFinal() { - this.getQualifiedName() = "EVP_EncryptFinal" - } + EVP_EncryptFinal() { this.getQualifiedName() = "EVP_EncryptFinal" } - override int getAContextArg() { - result = 0 - } + override int getAContextArg() { result = 0 } } // int EVP_DecryptFinal(EVP_CIPHER_CTX *ctx, unsigned char *outm, int *outl); class EVP_DecryptFinal extends SymmetricCipherFinal, ReturnsErrorCode { - EVP_DecryptFinal() { - this.getQualifiedName() = "EVP_DecryptFinal" - } + EVP_DecryptFinal() { this.getQualifiedName() = "EVP_DecryptFinal" } - override int getAContextArg() { - result = 0 - } + override int getAContextArg() { result = 0 } } // int EVP_EncryptFinal_ex(EVP_CIPHER_CTX *ctx, unsigned char *out, int *outl); class EVP_EncryptFinal_ex extends SymmetricCipherFinal, ReturnsErrorCode { - EVP_EncryptFinal_ex() { - this.getQualifiedName() = "EVP_EncryptFinal_ex" - } + EVP_EncryptFinal_ex() { this.getQualifiedName() = "EVP_EncryptFinal_ex" } - override int getAContextArg() { - result = 0 - } + override int getAContextArg() { result = 0 } } - // int EVP_DecryptFinal_ex(EVP_CIPHER_CTX *ctx, unsigned char *outm, int *outl); +// int EVP_DecryptFinal_ex(EVP_CIPHER_CTX *ctx, unsigned char *outm, int *outl); class EVP_DecryptFinal_ex extends SymmetricCipherFinal, ReturnsErrorCode { - EVP_DecryptFinal_ex() { - this.getQualifiedName() = "EVP_DecryptFinal_ex" - } + EVP_DecryptFinal_ex() { this.getQualifiedName() = "EVP_DecryptFinal_ex" } - override int getAContextArg() { - result = 0 - } + override int getAContextArg() { result = 0 } } // int EVP_CipherFinal(EVP_CIPHER_CTX *ctx, unsigned char *outm, int *outl); class EVP_CipherFinal extends SymmetricCipherFinal, ReturnsErrorCode { - EVP_CipherFinal() { - this.getQualifiedName() = "EVP_CipherFinal" - } + EVP_CipherFinal() { this.getQualifiedName() = "EVP_CipherFinal" } - override int getAContextArg() { - result = 0 - } + override int getAContextArg() { result = 0 } } // int EVP_CipherFinal(EVP_CIPHER_CTX *ctx, unsigned char *outm, int *outl); class EVP_CipherFinal_ex extends SymmetricCipherFinal, ReturnsErrorCode { - EVP_CipherFinal_ex() { - this.getQualifiedName() = "EVP_CipherFinal_ex" - } + EVP_CipherFinal_ex() { this.getQualifiedName() = "EVP_CipherFinal_ex" } - override int getAContextArg() { - result = 0 - } + override int getAContextArg() { result = 0 } } // Used by the MissingInitializer query. class EVP_EncryptTuple extends InitUpdateFinalTuple { - EVP_EncryptTuple() { - this = "EVP_EncryptTuple" - } + EVP_EncryptTuple() { this = "EVP_EncryptTuple" } override InitFunction getInit() { result instanceof EVP_EncryptInit or result instanceof EVP_EncryptInit_ex } - override UpdateFunction getUpdate() { - result instanceof EVP_EncryptUpdate - } + override UpdateFunction getUpdate() { result instanceof EVP_EncryptUpdate } override FinalFunction getFinal() { result instanceof EVP_EncryptFinal or @@ -289,18 +180,14 @@ class EVP_EncryptTuple extends InitUpdateFinalTuple { // Used by the MissingInitializer query. class EVP_DecryptTuple extends InitUpdateFinalTuple { - EVP_DecryptTuple() { - this = "EVP_DecryptTuple" - } + EVP_DecryptTuple() { this = "EVP_DecryptTuple" } override InitFunction getInit() { result instanceof EVP_DecryptInit or result instanceof EVP_DecryptInit_ex } - override UpdateFunction getUpdate() { - result instanceof EVP_DecryptUpdate - } + override UpdateFunction getUpdate() { result instanceof EVP_DecryptUpdate } override FinalFunction getFinal() { result instanceof EVP_DecryptFinal or @@ -310,18 +197,14 @@ class EVP_DecryptTuple extends InitUpdateFinalTuple { // Used by the MissingInitializer query. class EVP_CipherTuple extends InitUpdateFinalTuple { - EVP_CipherTuple() { - this = "EVP_CipherTuple" - } + EVP_CipherTuple() { this = "EVP_CipherTuple" } override InitFunction getInit() { result instanceof EVP_CipherInit or result instanceof EVP_CipherInit_ex } - override UpdateFunction getUpdate() { - result instanceof EVP_CipherUpdate - } + override UpdateFunction getUpdate() { result instanceof EVP_CipherUpdate } override FinalFunction getFinal() { result instanceof EVP_CipherFinal or @@ -338,17 +221,11 @@ class EVP_CipherTuple extends InitUpdateFinalTuple { // unsigned char *key, // unsigned char *iv); class EVP_BytesToKey extends KeyDerivationFunctionWithPasswordAndSalt, ReturnsErrorCode { - EVP_BytesToKey() { - this.getQualifiedName() = "EVP_BytesToKey" - } + EVP_BytesToKey() { this.getQualifiedName() = "EVP_BytesToKey" } - override int getPassword() { - result = 3 - } + override int getPassword() { result = 3 } - override int getSalt() { - result = 2 - } + override int getSalt() { result = 2 } } // int PKCS5_PBE_keyivgen(EVP_CIPHER_CTX *ctx, @@ -359,17 +236,11 @@ class EVP_BytesToKey extends KeyDerivationFunctionWithPasswordAndSalt, ReturnsEr // const EVP_MD *md, // int en_de); class PKCS5_PBE_keyivgen extends KeyDerivationFunctionWithPassword, ReturnsErrorCode { - PKCS5_PBE_keyivgen() { - this.getQualifiedName() = "PKCS5_PBE_keyivgen" - } + PKCS5_PBE_keyivgen() { this.getQualifiedName() = "PKCS5_PBE_keyivgen" } - override int getPassword() { - result = 1 - } + override int getPassword() { result = 1 } - int getMode() { - result = 6 - } + int getMode() { result = 6 } } // int PKCS5_PBKDF2_HMAC_SHA1(const char *pass, @@ -380,17 +251,11 @@ class PKCS5_PBE_keyivgen extends KeyDerivationFunctionWithPassword, ReturnsError // int keylen, // unsigned char *out); class PKCS5_PBKDF2_HMAC_SHA1 extends KeyDerivationFunctionWithPasswordAndSalt, ReturnsErrorCode { - PKCS5_PBKDF2_HMAC_SHA1() { - this.getQualifiedName() = "PKCS5_PBKDF2_HMAC_SHA1" - } + PKCS5_PBKDF2_HMAC_SHA1() { this.getQualifiedName() = "PKCS5_PBKDF2_HMAC_SHA1" } - override int getPassword() { - result = 0 - } + override int getPassword() { result = 0 } - override int getSalt() { - result = 2 - } + override int getSalt() { result = 2 } } // int PKCS5_PBKDF2_HMAC(const char *pass, @@ -402,17 +267,11 @@ class PKCS5_PBKDF2_HMAC_SHA1 extends KeyDerivationFunctionWithPasswordAndSalt, R // int keylen, // unsigned char *out); class PKCS5_PBKDF2_HMAC extends KeyDerivationFunctionWithPasswordAndSalt, ReturnsErrorCode { - PKCS5_PBKDF2_HMAC() { - this.getQualifiedName() = "PKCS5_PBKDF2_HMAC" - } + PKCS5_PBKDF2_HMAC() { this.getQualifiedName() = "PKCS5_PBKDF2_HMAC" } - override int getPassword() { - result = 0 - } + override int getPassword() { result = 0 } - override int getSalt() { - result = 2 - } + override int getSalt() { result = 2 } } // int PKCS5_v2_PBE_keyivgen(EVP_CIPHER_CTX *ctx, @@ -423,17 +282,11 @@ class PKCS5_PBKDF2_HMAC extends KeyDerivationFunctionWithPasswordAndSalt, Return // const EVP_MD *md, // int en_de); class PKCS5_v2_PBE_keyivgen extends KeyDerivationFunctionWithPassword, ReturnsErrorCode { - PKCS5_v2_PBE_keyivgen() { - this.getQualifiedName() = "PKCS5_v2_PBE_keyivgen" - } + PKCS5_v2_PBE_keyivgen() { this.getQualifiedName() = "PKCS5_v2_PBE_keyivgen" } - override int getPassword() { - result = 1 - } + override int getPassword() { result = 1 } - int getMode() { - result = 6 - } + int getMode() { result = 6 } } // int PKCS5_v2_PBE_keyivgen_ex(EVP_CIPHER_CTX *ctx, @@ -446,17 +299,11 @@ class PKCS5_v2_PBE_keyivgen extends KeyDerivationFunctionWithPassword, ReturnsEr // OSSL_LIB_CTX *libctx, // const char *propq); class PKCS5_v2_PBE_keyivgen_ex extends KeyDerivationFunctionWithPassword, ReturnsErrorCode { - PKCS5_v2_PBE_keyivgen_ex() { - this.getQualifiedName() = "PKCS5_v2_PBE_keyivgen_ex" - } + PKCS5_v2_PBE_keyivgen_ex() { this.getQualifiedName() = "PKCS5_v2_PBE_keyivgen_ex" } - override int getPassword() { - result = 1 - } + override int getPassword() { result = 1 } - int getMode() { - result = 6 - } + int getMode() { result = 6 } } // int EVP_PBE_scrypt(const char *pass, @@ -470,17 +317,11 @@ class PKCS5_v2_PBE_keyivgen_ex extends KeyDerivationFunctionWithPassword, Return // unsigned char *key, // size_t keylen); class EVP_PBE_scrypt extends KeyDerivationFunctionWithPasswordAndSalt, ReturnsErrorCode { - EVP_PBE_scrypt() { - this.getQualifiedName() = "EVP_PBE_scrypt" - } + EVP_PBE_scrypt() { this.getQualifiedName() = "EVP_PBE_scrypt" } - override int getPassword() { - result = 0 - } + override int getPassword() { result = 0 } - override int getSalt() { - result = 2 - } + override int getSalt() { result = 2 } } // int EVP_PBE_scrypt_ex(const char *pass, @@ -496,17 +337,11 @@ class EVP_PBE_scrypt extends KeyDerivationFunctionWithPasswordAndSalt, ReturnsEr // OSSL_LIB_CTX *ctx, // const char *propq); class EVP_PBE_scrypt_ex extends KeyDerivationFunctionWithPasswordAndSalt, ReturnsErrorCode { - EVP_PBE_scrypt_ex() { - this.getQualifiedName() = "EVP_PBE_scrypt" - } + EVP_PBE_scrypt_ex() { this.getQualifiedName() = "EVP_PBE_scrypt" } - override int getPassword() { - result = 0 - } + override int getPassword() { result = 0 } - override int getSalt() { - result = 2 - } + override int getSalt() { result = 2 } } // int PKCS5_v2_scrypt_keyivgen(EVP_CIPHER_CTX *ctx, @@ -517,17 +352,11 @@ class EVP_PBE_scrypt_ex extends KeyDerivationFunctionWithPasswordAndSalt, Return // const EVP_MD *md, // int en_de); class PKCS5_v2_scrypt_keyivgen extends KeyDerivationFunctionWithPassword, ReturnsErrorCode { - PKCS5_v2_scrypt_keyivgen() { - this.getQualifiedName() = "PKCS5_v2_scrypt_keyivgen" - } + PKCS5_v2_scrypt_keyivgen() { this.getQualifiedName() = "PKCS5_v2_scrypt_keyivgen" } - override int getPassword() { - result = 1 - } + override int getPassword() { result = 1 } - int getMode() { - result = 6 - } + int getMode() { result = 6 } } // int PKCS5_v2_scrypt_keyivgen_ex(EVP_CIPHER_CTX *ctx, @@ -540,17 +369,11 @@ class PKCS5_v2_scrypt_keyivgen extends KeyDerivationFunctionWithPassword, Return // OSSL_LIB_CTX *ctx, // const char *propq); class PKCS5_v2_scrypt_keyivgen_ex extends KeyDerivationFunctionWithPassword, ReturnsErrorCode { - PKCS5_v2_scrypt_keyivgen_ex() { - this.getQualifiedName() = "PKCS5_v2_scrypt_keyivgen" - } + PKCS5_v2_scrypt_keyivgen_ex() { this.getQualifiedName() = "PKCS5_v2_scrypt_keyivgen" } - override int getPassword() { - result = 1 - } + override int getPassword() { result = 1 } - int getMode() { - result = 6 - } + int getMode() { result = 6 } } // int EVP_PBE_CipherInit(ASN1_OBJECT *pbe_obj, @@ -560,17 +383,11 @@ class PKCS5_v2_scrypt_keyivgen_ex extends KeyDerivationFunctionWithPassword, Ret // EVP_CIPHER_CTX *ctx, // int en_de); class EVP_PBE_CipherInit extends KeyDerivationFunctionWithPassword, ReturnsErrorCode { - EVP_PBE_CipherInit() { - this.getQualifiedName() = "EVP_PBE_CipherInit" - } + EVP_PBE_CipherInit() { this.getQualifiedName() = "EVP_PBE_CipherInit" } - override int getPassword() { - result = 1 - } + override int getPassword() { result = 1 } - int getMode() { - result = 5 - } + int getMode() { result = 5 } } // int EVP_PBE_CipherInit_ex(ASN1_OBJECT *pbe_obj, @@ -582,44 +399,48 @@ class EVP_PBE_CipherInit extends KeyDerivationFunctionWithPassword, ReturnsError // OSSL_LIB_CTX *ctx, // const char *propq); class EVP_PBE_CipherInit_ex extends KeyDerivationFunctionWithPassword, ReturnsErrorCode { - EVP_PBE_CipherInit_ex() { - this.getQualifiedName() = "EVP_PBE_CipherInit" - } + EVP_PBE_CipherInit_ex() { this.getQualifiedName() = "EVP_PBE_CipherInit" } - override int getPassword() { - result = 1 - } + override int getPassword() { result = 1 } - int getMode() { - result = 5 - } + int getMode() { result = 5 } } // TODO: Implement support for `EVP_CIPHER_fetch`. class EVP_CIPHER extends FunctionCall { - int keySize; - EVP_CIPHER () { + EVP_CIPHER() { // AES variants. - (this.getTarget().getName().matches("%EVP_aes_256_cbc%") and keySize = 32) or - (this.getTarget().getName().matches("%EVP_aes_256_cfb%") and keySize = 32) or - (this.getTarget().getName().matches("%EVP_aes_256_ctr%") and keySize = 32) or - (this.getTarget().getName().matches("%EVP_aes_256_ecb%") and keySize = 32) or - (this.getTarget().getName().matches("%EVP_aes_256_ofb%") and keySize = 32) or - (this.getTarget().getName().matches("%EVP_aes_256_xts%") and keySize = 64) or - (this.getTarget().getName().matches("%EVP_aes_192%") and keySize = 24) or - (this.getTarget().getName().matches("%EVP_aes_128_cbc%") and keySize = 16) or - (this.getTarget().getName().matches("%EVP_aes_128_cfb%") and keySize = 16) or - (this.getTarget().getName().matches("%EVP_aes_128_ctr%") and keySize = 16) or - (this.getTarget().getName().matches("%EVP_aes_128_ecb%") and keySize = 16) or - (this.getTarget().getName().matches("%EVP_aes_128_ofb%") and keySize = 16) or - (this.getTarget().getName().matches("%EVP_aes_128_xts%") and keySize = 32) or + this.getTarget().getName().matches("%EVP_aes_256_cbc%") and keySize = 32 + or + this.getTarget().getName().matches("%EVP_aes_256_cfb%") and keySize = 32 + or + this.getTarget().getName().matches("%EVP_aes_256_ctr%") and keySize = 32 + or + this.getTarget().getName().matches("%EVP_aes_256_ecb%") and keySize = 32 + or + this.getTarget().getName().matches("%EVP_aes_256_ofb%") and keySize = 32 + or + this.getTarget().getName().matches("%EVP_aes_256_xts%") and keySize = 64 + or + this.getTarget().getName().matches("%EVP_aes_192%") and keySize = 24 + or + this.getTarget().getName().matches("%EVP_aes_128_cbc%") and keySize = 16 + or + this.getTarget().getName().matches("%EVP_aes_128_cfb%") and keySize = 16 + or + this.getTarget().getName().matches("%EVP_aes_128_ctr%") and keySize = 16 + or + this.getTarget().getName().matches("%EVP_aes_128_ecb%") and keySize = 16 + or + this.getTarget().getName().matches("%EVP_aes_128_ofb%") and keySize = 16 + or + this.getTarget().getName().matches("%EVP_aes_128_xts%") and keySize = 32 + or // Chacha20 variants. - (this.getTarget().getName().matches("%EVP_chacha20%") and keySize = 32) + this.getTarget().getName().matches("%EVP_chacha20%") and keySize = 32 } - int getKeySize() { - result = keySize - } + int getKeySize() { result = keySize } } diff --git a/cpp/lib/trailofbits/crypto/openssl/hash.qll b/cpp/lib/trailofbits/crypto/openssl/hash.qll index 217a1e7..ba640a2 100644 --- a/cpp/lib/trailofbits/crypto/openssl/hash.qll +++ b/cpp/lib/trailofbits/crypto/openssl/hash.qll @@ -6,63 +6,42 @@ module Hash { // int SHA1_Init(SHA_CTX *c); abstract class Init extends HashContextInitializer { // Returns the index of the context argument (if there is one). - override int getAContext() { - result = 0 - } + override int getAContext() { result = 0 } // True iff the initializer returns the context. - override boolean returnsContext() { - result = false - } + override boolean returnsContext() { result = false } } // int SHA1_Update(SHA_CTX *c, const void *data, size_t len); abstract class Update extends HashContextUpdater { // Returns the index of the context argument. - override int getContext() { - result = 0 - } + override int getContext() { result = 0 } // Returns an index of an input data argument. - override int getAnInput() { - result = 1 - } + override int getAnInput() { result = 1 } } // int SHA1_Final(unsigned char *md, SHA_CTX *c); abstract class Final extends HashContextFinalizer { // Returns the index of the context argument. - override int getContext() { - result = 1 - } + override int getContext() { result = 1 } // Returns the output digest argument (if it exists). - override int getAnOutput() { - result = 0 - } + override int getAnOutput() { result = 0 } // True iff the finalizer function returns a pointer or reference to the // output digest. - override boolean returnsOutput() { - result = false - } + override boolean returnsOutput() { result = false } } abstract class Digest extends HashFunction { // Returns the index of the input data argument. - override int getInput() { - result = 0 - } + override int getInput() { result = 0 } // Returns an the index of an output data argument (if there is one). - override int getAnOutput() { - result = 2 - } + override int getAnOutput() { result = 2 } // True if the function returns a reference or pointer to the output. - override boolean returnsOutput() { - result = true - } + override boolean returnsOutput() { result = true } } } - diff --git a/cpp/lib/trailofbits/crypto/openssl/sha.qll b/cpp/lib/trailofbits/crypto/openssl/sha.qll index 57b8ab6..14cbdd8 100644 --- a/cpp/lib/trailofbits/crypto/openssl/sha.qll +++ b/cpp/lib/trailofbits/crypto/openssl/sha.qll @@ -3,143 +3,102 @@ private import hash // This file contains CodeQL models for the OpenSSL SHA functions defined in // openssl/sha.h. - // int SHA1_Init(SHA_CTX *c); class SHA1_Init extends Hash::Init, ReturnsErrorCode { - SHA1_Init() { - this.getQualifiedName() = "SHA1_Init" - } + SHA1_Init() { this.getQualifiedName() = "SHA1_Init" } } // int SHA1_Update(SHA_CTX *c, const void *data, size_t len); class SHA1_Update extends Hash::Update, ReturnsErrorCode { - SHA1_Update() { - this.getQualifiedName() = "SHA1_Update" - } + SHA1_Update() { this.getQualifiedName() = "SHA1_Update" } } // int SHA1_Final(unsigned char *md, SHA_CTX *c); class SHA1_Final extends Hash::Final, ReturnsErrorCode { - SHA1_Final() { - this.getQualifiedName() = "SHA1_Final" - } + SHA1_Final() { this.getQualifiedName() = "SHA1_Final" } } // unsigned char *SHA1(const unsigned char *d, size_t n, unsigned char *md); class SHA1 extends Hash::Digest { - SHA1() { - this.getQualifiedName() = "SHA1" - } + SHA1() { this.getQualifiedName() = "SHA1" } } // int SHA224_Init(SHA_CTX *c); class SHA224_Init extends Hash::Init, ReturnsErrorCode { - SHA224_Init() { - this.getQualifiedName() = "SHA224_Init" - } + SHA224_Init() { this.getQualifiedName() = "SHA224_Init" } } // int SHA224_Update(SHA_CTX *c, const void *data, size_t len); class SHA224_Update extends Hash::Update, ReturnsErrorCode { - SHA224_Update() { - this.getQualifiedName() = "SHA224_Update" - } + SHA224_Update() { this.getQualifiedName() = "SHA224_Update" } } // int SHA224_Final(unsigned char *md, SHA_CTX *c); class SHA224_Final extends Hash::Final, ReturnsErrorCode { - SHA224_Final() { - this.getQualifiedName() = "SHA224_Final" - } + SHA224_Final() { this.getQualifiedName() = "SHA224_Final" } } // unsigned char *SHA224(const unsigned char *d, size_t n, unsigned char *md); class SHA224 extends Hash::Digest { - SHA224() { - this.getQualifiedName() = "SHA224" - } + SHA224() { this.getQualifiedName() = "SHA224" } } // int SHA256_Init(SHA_CTX *c); class SHA256_Init extends Hash::Init, ReturnsErrorCode { - SHA256_Init() { - this.getQualifiedName() = "SHA256_Init" - } + SHA256_Init() { this.getQualifiedName() = "SHA256_Init" } } // int SHA256_Update(SHA_CTX *c, const void *data, size_t len); class SHA256_Update extends Hash::Update, ReturnsErrorCode { - SHA256_Update() { - this.getQualifiedName() = "SHA256_Update" - } + SHA256_Update() { this.getQualifiedName() = "SHA256_Update" } } // int SHA256_Final(unsigned char *md, SHA_CTX *c); class SHA256_Final extends Hash::Final, ReturnsErrorCode { - SHA256_Final() { - this.getQualifiedName() = "SHA256_Final" - } + SHA256_Final() { this.getQualifiedName() = "SHA256_Final" } } // unsigned char *SHA256(const unsigned char *d, size_t n, unsigned char *md); class SHA256 extends Hash::Digest { - SHA256() { - this.getQualifiedName() = "SHA256" - } + SHA256() { this.getQualifiedName() = "SHA256" } } // int SHA384_Init(SHA_CTX *c); class SHA384_Init extends Hash::Init, ReturnsErrorCode { - SHA384_Init() { - this.getQualifiedName() = "SHA384_Init" - } + SHA384_Init() { this.getQualifiedName() = "SHA384_Init" } } // int SHA384_Update(SHA_CTX *c, const void *data, size_t len); class SHA384_Update extends Hash::Update, ReturnsErrorCode { - SHA384_Update() { - this.getQualifiedName() = "SHA384_Update" - } + SHA384_Update() { this.getQualifiedName() = "SHA384_Update" } } // int SHA384_Final(unsigned char *md, SHA_CTX *c); class SHA384_Final extends Hash::Final, ReturnsErrorCode { - SHA384_Final() { - this.getQualifiedName() = "SHA384_Final" - } + SHA384_Final() { this.getQualifiedName() = "SHA384_Final" } } // unsigned char *SHA384(const unsigned char *d, size_t n, unsigned char *md); class SHA384 extends Hash::Digest { - SHA384() { - this.getQualifiedName() = "SHA384" - } + SHA384() { this.getQualifiedName() = "SHA384" } } // int SHA512_Init(SHA_CTX *c); class SHA512_Init extends Hash::Init, ReturnsErrorCode { - SHA512_Init() { - this.getQualifiedName() = "SHA512_Init" - } + SHA512_Init() { this.getQualifiedName() = "SHA512_Init" } } // int SHA512_Update(SHA_CTX *c, const void *data, size_t len); class SHA512_Update extends Hash::Update, ReturnsErrorCode { - SHA512_Update() { - this.getQualifiedName() = "SHA512_Update" - } + SHA512_Update() { this.getQualifiedName() = "SHA512_Update" } } // int SHA512_Final(unsigned char *md, SHA_CTX *c); class SHA512_Final extends Hash::Final, ReturnsErrorCode { - SHA512_Final() { - this.getQualifiedName() = "SHA512_Final" - } + SHA512_Final() { this.getQualifiedName() = "SHA512_Final" } } // unsigned char *SHA512(const unsigned char *d, size_t n, unsigned char *md); class SHA512 extends Hash::Digest { - SHA512() { - this.getQualifiedName() = "SHA512" - } + SHA512() { this.getQualifiedName() = "SHA512" } } diff --git a/cpp/src/crypto/InvalidKeySize.ql b/cpp/src/crypto/InvalidKeySize.ql index ff55062..f3b350f 100644 --- a/cpp/src/crypto/InvalidKeySize.ql +++ b/cpp/src/crypto/InvalidKeySize.ql @@ -14,48 +14,38 @@ import semmle.code.cpp.dataflow.new.DataFlow import trailofbits.crypto.libraries class Key extends Variable { - FunctionCall init; Key() { - DataFlow::localFlow( - DataFlow::exprNode(this.getAnAccess()), - DataFlow::exprNode(init.getArgument(init.getTarget().(EVP_EncryptInit).getKey())) - ) or - DataFlow::localFlow( - DataFlow::exprNode(this.getAnAccess()), - DataFlow::exprNode(init.getArgument(init.getTarget().(EVP_EncryptInit_ex).getKey())) - ) + DataFlow::localFlow(DataFlow::exprNode(this.getAnAccess()), + DataFlow::exprNode(init.getArgument(init.getTarget().(EVP_EncryptInit).getKey()))) or + DataFlow::localFlow(DataFlow::exprNode(this.getAnAccess()), + DataFlow::exprNode(init.getArgument(init.getTarget().(EVP_EncryptInit_ex).getKey()))) } EVP_CIPHER getACipher() { - DataFlow::localFlow(DataFlow::exprNode(result), DataFlow::exprNode(init.getArgument(init.getTarget().(EVP_EncryptInit).getCipher()))) or - DataFlow::localFlow(DataFlow::exprNode(result), DataFlow::exprNode(init.getArgument(init.getTarget().(EVP_EncryptInit_ex).getCipher()))) + DataFlow::localFlow(DataFlow::exprNode(result), + DataFlow::exprNode(init.getArgument(init.getTarget().(EVP_EncryptInit).getCipher()))) or + DataFlow::localFlow(DataFlow::exprNode(result), + DataFlow::exprNode(init.getArgument(init.getTarget().(EVP_EncryptInit_ex).getCipher()))) } FunctionCall getInitCall() { result = init } int getSize() { result = this.getUnderlyingType().getSize() } - predicate correctKeySize(EVP_CIPHER cipher) { - cipher.getKeySize() = this.getSize() - } + predicate correctKeySize(EVP_CIPHER cipher) { cipher.getKeySize() = this.getSize() } // Avoid matching on pointers where the key size is not known. - predicate isArray() { - this.getType() instanceof ArrayType - } + predicate isArray() { this.getType() instanceof ArrayType } } - - -from - Key key, - EVP_CIPHER cipher -where - cipher = key.getACipher() and +from Key key, EVP_CIPHER cipher +where + cipher = key.getACipher() and key.isArray() and not key.correctKeySize(cipher) -select - key.getInitCall().getLocation(), - "Key size (" + key.getSize() + " bytes) does not match the expected key size for the encryption algorithm (" + cipher.getKeySize() + " bytes)" +select key.getInitCall().getLocation(), + "Key size (" + key.getSize() + + " bytes) does not match the expected key size for the encryption algorithm (" + + cipher.getKeySize() + " bytes)" diff --git a/cpp/src/crypto/MissingErrorHandling.ql b/cpp/src/crypto/MissingErrorHandling.ql index 573077f..3cd716c 100644 --- a/cpp/src/crypto/MissingErrorHandling.ql +++ b/cpp/src/crypto/MissingErrorHandling.ql @@ -15,33 +15,25 @@ import trailofbits.crypto.libraries import semmle.code.cpp.dataflow.new.DataFlow predicate isChecked(Expr value) { - // The return value flows into the condition of an if-statement. - exists (IfStmt is | - DataFlow::localFlow( - DataFlow::exprNode(value), - DataFlow::exprNode(is.getCondition().getAChild*()) - ) - ) or - // The return value flows into the condition of a while-statement. - exists (WhileStmt ws | - DataFlow::localFlow( - DataFlow::exprNode(value), - DataFlow::exprNode(ws.getCondition().getAChild*()) - ) - ) or - // The return value flows into the condition of a switch-statement. - exists (SwitchStmt ss | - DataFlow::localFlow( - DataFlow::exprNode(value), - DataFlow::exprNode(ss.getExpr().getAChild*()) - ) - ) + // The return value flows into the condition of an if-statement. + exists(IfStmt is | + DataFlow::localFlow(DataFlow::exprNode(value), + DataFlow::exprNode(is.getCondition().getAChild*())) + ) + or + // The return value flows into the condition of a while-statement. + exists(WhileStmt ws | + DataFlow::localFlow(DataFlow::exprNode(value), + DataFlow::exprNode(ws.getCondition().getAChild*())) + ) + or + // The return value flows into the condition of a switch-statement. + exists(SwitchStmt ss | + DataFlow::localFlow(DataFlow::exprNode(value), DataFlow::exprNode(ss.getExpr().getAChild*())) + ) } -from - ErrorCode res -where - not isChecked(res) -select - res.getLocation(), +from ErrorCode res +where not isChecked(res) +select res.getLocation(), "The function fails to check the return value of '" + res.getTarget().getName() + "'" diff --git a/cpp/src/crypto/RandomBufferTooSmall.ql b/cpp/src/crypto/RandomBufferTooSmall.ql index 497d4e4..99704d4 100644 --- a/cpp/src/crypto/RandomBufferTooSmall.ql +++ b/cpp/src/crypto/RandomBufferTooSmall.ql @@ -15,25 +15,21 @@ import trailofbits.crypto.libraries // Exclude pointer arguments where we cannot determine the buffer size. predicate sourceIsPointer(CsprngCall call) { - call.getAStrongRandomnessSource().getType().getPointerIndirectionLevel() > 0 + call.getAStrongRandomnessSource().getType().getPointerIndirectionLevel() > 0 } int getBufferSize(CsprngCall call) { result = call.getAStrongRandomnessSource().getUnderlyingType().getSize() } -int getRequestedBytes(CsprngCall call) { - result = call.getRequestedBytes().getValue().toInt() -} +int getRequestedBytes(CsprngCall call) { result = call.getRequestedBytes().getValue().toInt() } -from - CsprngCall call, - int bufferSize, - int requestedSize +from CsprngCall call, int bufferSize, int requestedSize where not sourceIsPointer(call) and bufferSize = getBufferSize(call) and requestedSize = getRequestedBytes(call) and requestedSize > bufferSize select call.getLocation(), - "Buffer size (" + bufferSize + ") is smaller than the number of requested bytes (" + requestedSize + ") in call to '" + call.getTarget().getName() + "'." + "Buffer size (" + bufferSize + ") is smaller than the number of requested bytes (" + requestedSize + + ") in call to '" + call.getTarget().getName() + "'." diff --git a/cpp/src/crypto/StaticKeyFlow.ql b/cpp/src/crypto/StaticKeyFlow.ql index 5c7806d..23b5982 100644 --- a/cpp/src/crypto/StaticKeyFlow.ql +++ b/cpp/src/crypto/StaticKeyFlow.ql @@ -13,9 +13,7 @@ import cpp import trailofbits.crypto.libraries import semmle.code.cpp.dataflow.new.DataFlow -from - DataFlow::Node source, DataFlow::Node sink, - StrongRandomnessSink random +from DataFlow::Node source, DataFlow::Node sink, StrongRandomnessSink random where StaticKeyFlow::flow(source, sink) and random = sink.asExpr() diff --git a/cpp/src/crypto/StaticPasswordFlow.ql b/cpp/src/crypto/StaticPasswordFlow.ql index d57958f..e5f5dba 100644 --- a/cpp/src/crypto/StaticPasswordFlow.ql +++ b/cpp/src/crypto/StaticPasswordFlow.ql @@ -13,9 +13,7 @@ import cpp import trailofbits.crypto.libraries import semmle.code.cpp.dataflow.new.DataFlow -from - DataFlow::Node source, DataFlow::Node sink, - StrongPasswordSink random +from DataFlow::Node source, DataFlow::Node sink, StrongPasswordSink random where StaticPasswordFlow::flow(source, sink) and random = sink.asExpr() diff --git a/cpp/src/crypto/UnbalancedBnCtx.ql b/cpp/src/crypto/UnbalancedBnCtx.ql index e13f2b4..124c3ff 100644 --- a/cpp/src/crypto/UnbalancedBnCtx.ql +++ b/cpp/src/crypto/UnbalancedBnCtx.ql @@ -32,14 +32,11 @@ predicate hasMatchingStart(BN_CTX_end end) { from FunctionCall call, string message where - ( - call instanceof BN_CTX_start and - not hasMatchingEnd(call) and - message = "BN_CTX_start called without corresponding BN_CTX_end" - ) or - ( - call instanceof BN_CTX_end and - not hasMatchingStart(call) and - message = "BN_CTX_end called without corresponding BN_CTX_start" - ) + call instanceof BN_CTX_start and + not hasMatchingEnd(call) and + message = "BN_CTX_start called without corresponding BN_CTX_end" + or + call instanceof BN_CTX_end and + not hasMatchingStart(call) and + message = "BN_CTX_end called without corresponding BN_CTX_start" select call.getLocation(), message diff --git a/cpp/src/crypto/UseOfLegacyAlgorithm.ql b/cpp/src/crypto/UseOfLegacyAlgorithm.ql index e3e2bc3..a26f42f 100644 --- a/cpp/src/crypto/UseOfLegacyAlgorithm.ql +++ b/cpp/src/crypto/UseOfLegacyAlgorithm.ql @@ -13,33 +13,33 @@ import cpp from FunctionCall call, string functionName, string cipherName where - functionName = call.getTarget() - .getQualifiedName() - .toLowerCase() - and - ( - exists(string cn | - cn in [ - "MD2", "MD4", "MD5", "RIPEMD", "SHA1", "Whirlpool", "Streebog", - "PBKDF1", - "ArcFour", "Blowfish", "CAST", "IDEA", "Kasumi", - "Magma", "RC2", "RC4", "TDEA" - ] - and cipherName = cn - and functionName.matches("%" + cn.toLowerCase() + "%") - ) - /* match DES, but avoid false positives by not matching common terms containing it: - nodes - modes - codes - describe - description - descriptor - design - descend - destroy - */ - or cipherName = "DES" and functionName.regexpMatch(".*(?; /** * signal(SIGX, signalHandler) - */ + */ predicate isSignal(FunctionCall signalCall, Function signalHandler) { - signalCall.getTarget().getName() = "signal" - and exists(DataFlow::Node source, DataFlow::Node sink | - HandlerToSignal::flow(source, sink) - and source.asExpr() = signalHandler.getAnAccess() - and sink.asExpr() = signalCall.getArgument(1) + signalCall.getTarget().getName() = "signal" and + exists(DataFlow::Node source, DataFlow::Node sink | + HandlerToSignal::flow(source, sink) and + source.asExpr() = signalHandler.getAnAccess() and + sink.asExpr() = signalCall.getArgument(1) ) } @@ -80,36 +78,34 @@ predicate isSignal(FunctionCall signalCall, Function signalHandler) { */ predicate isSigaction(FunctionCall sigactionCall, Function signalHandler, Variable sigactVar) { exists(Struct sigactStruct, Field handlerField, DataFlow::Node source, DataFlow::Node sink | - sigactionCall.getTarget().getName() = "sigaction" - and sigactionCall.getArgument(1).getAChild*() = sigactVar.getAnAccess() - + sigactionCall.getTarget().getName() = "sigaction" and + sigactionCall.getArgument(1).getAChild*() = sigactVar.getAnAccess() and // struct sigaction with the handler func - and sigactStruct.getName() = "sigaction" - and handlerField.getName() = ["sa_handler", "sa_sigaction", "__sa_handler", "__sa_sigaction", "__sigaction_u"] - and ( + sigactStruct.getName() = "sigaction" and + handlerField.getName() = + ["sa_handler", "sa_sigaction", "__sa_handler", "__sa_sigaction", "__sigaction_u"] and + ( handlerField = sigactStruct.getAField() or exists(Union u | u.getAField() = handlerField and u = sigactStruct.getAField().getType()) - ) - - and ( + ) and + ( // sigactVar.sa_handler = &signalHandler exists(Assignment a, ValueFieldAccess vfa | - vfa.getTarget() = handlerField - and vfa.getQualifier+() = sigactVar.getAnAccess() - and a.getLValue() = vfa.getAChild*() - - and source.asExpr() = signalHandler.getAnAccess() - and sink.asExpr() = a.getRValue() - and HandlerToSignal::flow(source, sink) + vfa.getTarget() = handlerField and + vfa.getQualifier+() = sigactVar.getAnAccess() and + a.getLValue() = vfa.getAChild*() and + source.asExpr() = signalHandler.getAnAccess() and + sink.asExpr() = a.getRValue() and + HandlerToSignal::flow(source, sink) ) or // struct sigaction sigactVar = {.sa_sigaction = signalHandler}; exists(ClassAggregateLiteral initLit | - sigactVar.getInitializer().getExpr() = initLit - and source.asExpr() = signalHandler.getAnAccess() - and sink.asExpr() = initLit.getAFieldExpr(handlerField).getAChild*() - and HandlerToSignal::flow(source, sink) + sigactVar.getInitializer().getExpr() = initLit and + source.asExpr() = signalHandler.getAnAccess() and + sink.asExpr() = initLit.getAFieldExpr(handlerField).getAChild*() and + HandlerToSignal::flow(source, sink) ) ) ) @@ -126,28 +122,24 @@ predicate isSignalDeliveryBlocked(Variable sigactVar) { ) or exists(Field mask | - mask.getName() = "sa_mask" - and exists(sigactVar.getInitializer().getExpr().(ClassAggregateLiteral).getAFieldExpr(mask)) + mask.getName() = "sa_mask" and + exists(sigactVar.getInitializer().getExpr().(ClassAggregateLiteral).getAFieldExpr(mask)) ) } -string deliveryNotBlockedMsg() { - result = "Moreover, delivery of new signals may be not blocked. " -} +string deliveryNotBlockedMsg() { result = "Moreover, delivery of new signals may be not blocked. " } from FunctionCall fc, Function signalHandler, string msg where - isAsyncUnsafe(signalHandler) - and ( - (isSignal(fc, signalHandler) and msg = deliveryNotBlockedMsg()) + isAsyncUnsafe(signalHandler) and + ( + isSignal(fc, signalHandler) and msg = deliveryNotBlockedMsg() or exists(Variable sigactVar | - isSigaction(fc, signalHandler, sigactVar) - and if isSignalDeliveryBlocked(sigactVar) then - msg = "" - else - msg = deliveryNotBlockedMsg() + isSigaction(fc, signalHandler, sigactVar) and + if isSignalDeliveryBlocked(sigactVar) then msg = "" else msg = deliveryNotBlockedMsg() ) ) -select signalHandler, "$@ is a non-trivial signal handler that uses not async-safe functions. " + msg + - "Handler is registered by $@.", signalHandler, signalHandler.toString(), fc, fc.toString() +select signalHandler, + "$@ is a non-trivial signal handler that uses not async-safe functions. " + msg + + "Handler is registered by $@.", signalHandler, signalHandler.toString(), fc, fc.toString() diff --git a/cpp/src/security/CStrnFinder/CStrnFinder.ql b/cpp/src/security/CStrnFinder/CStrnFinder.ql index 8f2f4aa..3cfdd8a 100644 --- a/cpp/src/security/CStrnFinder/CStrnFinder.ql +++ b/cpp/src/security/CStrnFinder/CStrnFinder.ql @@ -14,39 +14,41 @@ import cpp import semmle.code.cpp.security.BufferWrite import semmle.code.cpp.dataflow.new.DataFlow -/* - * Class for all functions interesting for us +/* + * Class for all functions interesting for us */ + abstract class StrNFunction extends Function { abstract int getSourceArgs(); + abstract int getSizeArg(); } -/* +/* * Class for functions with two input (source) arguments */ + class StrNFunctionCallTwoSources extends StrNFunction { StrNFunctionCallTwoSources() { this.getName() in [ - // STRNCMP like - "strncmp", // strncmp(src_a, src_b, count) - "strnicmp", // strnicmp(src_a, src_b, count) - "strncasecmp", // strncasecmp(src_a, src_b, count) - "bcmp", // bcmp(src_a, src_b, count) - "_mbsnbcmp", // _mbsnbcmp(src_a, src_b, count) multibyte - "_mbsnbcmp_l", // _mbsnbcmp_l(src_a, src_b, count, locale) multibyte - "_mbsncmp", // _mbsncmp(src_a, src_b, count) multibyte - "_mbsncmp_l", // _mbsncmp_l(src_a, src_b, count, locale) multibyte - "wcsncmp", // wcsncmp(src_a, src_b, count) widechar - "wcsncasecmp", // wcsncasecmp(src_a, src_b, count) widechar - "wcsncasecmp_l", // wcsncasecmp(src_a, src_b, count, locale) widechar - - // MEMCMP like - "memcmp", // memcmp(src_a, src_b, count) - "_memicmp", // _memicmp(src_a, src_b, count) - "_memicmp_l", // _memicmp_l(src_a, src_b, count, locale) - "wmemcmp" // wmemcmp(src_a, src_b, count) widechar - ] + // STRNCMP like + "strncmp", // strncmp(src_a, src_b, count) + "strnicmp", // strnicmp(src_a, src_b, count) + "strncasecmp", // strncasecmp(src_a, src_b, count) + "bcmp", // bcmp(src_a, src_b, count) + "_mbsnbcmp", // _mbsnbcmp(src_a, src_b, count) multibyte + "_mbsnbcmp_l", // _mbsnbcmp_l(src_a, src_b, count, locale) multibyte + "_mbsncmp", // _mbsncmp(src_a, src_b, count) multibyte + "_mbsncmp_l", // _mbsncmp_l(src_a, src_b, count, locale) multibyte + "wcsncmp", // wcsncmp(src_a, src_b, count) widechar + "wcsncasecmp", // wcsncasecmp(src_a, src_b, count) widechar + "wcsncasecmp_l", // wcsncasecmp(src_a, src_b, count, locale) widechar + // MEMCMP like + "memcmp", // memcmp(src_a, src_b, count) + "_memicmp", // _memicmp(src_a, src_b, count) + "_memicmp_l", // _memicmp_l(src_a, src_b, count, locale) + "wmemcmp" // wmemcmp(src_a, src_b, count) widechar + ] } override int getSourceArgs() { @@ -55,183 +57,166 @@ class StrNFunctionCallTwoSources extends StrNFunction { result = 1 } - override int getSizeArg() { - result = 2 - } + override int getSizeArg() { result = 2 } } -/* +/* * Class for functions with single input (source) argument */ + class StrNFunctionCallSingleSource extends StrNFunction { // https://github.com/gcc-mirror/gcc/blob/f3fc9b804a4e552a173e2d4071b2adec33178161/gcc/testsuite/gcc.c-torture/execute/builtins/chk.h // https://www.ibm.com/docs/ko/xl-c-and-cpp-linux/16.1.0?topic=functions-builtin-chk // https://github.com/llvm/llvm-project/blob/53e5cd4d3e39dad47312a48d4c6c71318bb2c283/clang/lib/Sema/SemaChecking.cpp#L1261-L1293 - StrNFunctionCallSingleSource() { // STRNCPY like this.hasGlobalOrStdOrBslName([ - "strncpy", // strncpy(dst, src, max_copy) - "__builtin___strncpy_chk", // __builtin___strncpy_chk (dst, src, max_copy, os) - "__builtin_strncpy", // __builtin_strncpy (dst, src, max_copy) - "wcsncpy", // wcsncpy(dst, src, max_copy) widechar - "strxfrm", // strxfrm(dst, src, max_copy) - "wcsxfrm" // wcsxfrm(dst, src, max_copy) widechar - // should be safe - // "wcsncpy_s" // wcsncpy_s(dst, dst_size, src, max_copy) - // "wcslcpy" // wcslcpy(dst, src, dst_size) - ]) + "strncpy", // strncpy(dst, src, max_copy) + "__builtin___strncpy_chk", // __builtin___strncpy_chk (dst, src, max_copy, os) + "__builtin_strncpy", // __builtin_strncpy (dst, src, max_copy) + "wcsncpy", // wcsncpy(dst, src, max_copy) widechar + "strxfrm", // strxfrm(dst, src, max_copy) + "wcsxfrm" // wcsxfrm(dst, src, max_copy) widechar + // should be safe + // "wcsncpy_s" // wcsncpy_s(dst, dst_size, src, max_copy) + // "wcslcpy" // wcslcpy(dst, src, dst_size) + ]) or this.hasGlobalName([ - "_strncpy_l", // _strncpy_l(dst, src, max_copy, locale) - "_wcsncpy_l", // _wcsncpy_l(dst, src, max_copy, locale) widechar - "_mbsncpy", // _mbsncpy(dst, src, max_copy) multibyte - "_mbsncpy_l", // _mbsncpy_l(dst, src, max_copy, locale) multibyte - "_strxfrm_l", // _strxfrm_l(dst, src, max_copy, locale) - "wcsxfrm_l", // _strxfrm_l(dst, src, max_copy, locale) widechar - "_mbsnbcpy", // _mbsnbcpy(dst, src, max_copy) multibyte - "_mbsnbcpy_l", // _mbsnbcpy_l(dst, src, max_copy, locale) multibyte - // should be safe - // "_mbsnbcpy_s", // _mbsnbcpy_s(dst, dst_size, src) - // "_mbsnbcpy_s_l", // _mbsnbcpy_s_l(dst, dst_size, src, locale) - // "_mbscpy_s", // _mbscpy_s(dst, dst_size, src) - "stpncpy", // stpncpy(dst, src, max_copy) - "__builtin___stpncpy_chk", // __builtin___stpncpy_chk(dst, src, max_copy, os) - "__builtin_stpncpy", // __builtin_stpncpy(dst, src, max_copy) - "strlcpy", // strlcpy(dst, src, max_copy_minus_one) - "__builtin___strlcpy_chk" // __builtin___strlcpy_chk(dst, src, max_copy_minus_one) - ]) - + "_strncpy_l", // _strncpy_l(dst, src, max_copy, locale) + "_wcsncpy_l", // _wcsncpy_l(dst, src, max_copy, locale) widechar + "_mbsncpy", // _mbsncpy(dst, src, max_copy) multibyte + "_mbsncpy_l", // _mbsncpy_l(dst, src, max_copy, locale) multibyte + "_strxfrm_l", // _strxfrm_l(dst, src, max_copy, locale) + "wcsxfrm_l", // _strxfrm_l(dst, src, max_copy, locale) widechar + "_mbsnbcpy", // _mbsnbcpy(dst, src, max_copy) multibyte + "_mbsnbcpy_l", // _mbsnbcpy_l(dst, src, max_copy, locale) multibyte + // should be safe + // "_mbsnbcpy_s", // _mbsnbcpy_s(dst, dst_size, src) + // "_mbsnbcpy_s_l", // _mbsnbcpy_s_l(dst, dst_size, src, locale) + // "_mbscpy_s", // _mbscpy_s(dst, dst_size, src) + "stpncpy", // stpncpy(dst, src, max_copy) + "__builtin___stpncpy_chk", // __builtin___stpncpy_chk(dst, src, max_copy, os) + "__builtin_stpncpy", // __builtin_stpncpy(dst, src, max_copy) + "strlcpy", // strlcpy(dst, src, max_copy_minus_one) + "__builtin___strlcpy_chk" // __builtin___strlcpy_chk(dst, src, max_copy_minus_one) + ]) + or // should be safe // or // this.hasGlobalOrStdName([ // "strcpy_s", // strcpy_s(dst, dst_size, src) // "wcscpy_s" // wcscpy_s(dst, dst_size, src) widechar // ]) - // STRNCAT like - or this.hasGlobalOrStdOrBslName([ - "strncat", // strncat(dst, src, max_copy) - "__builtin___strncat_chk", // __builtin___strncat_chk(dst, src, max_copy, os) - "__builtin_strncat", // __builtin_strncat(dst, src, max_copy) - "wcsncat", // wcsncat(dst, src, max_copy) widechar - // should be safe - // "wcsncat_s" // wcsncat_s(dst, dst_size, src, max_copy) - // "wcslcat" // wcslcat(dst, src, dst_size) - ]) + "strncat", // strncat(dst, src, max_copy) + "__builtin___strncat_chk", // __builtin___strncat_chk(dst, src, max_copy, os) + "__builtin_strncat", // __builtin_strncat(dst, src, max_copy) + "wcsncat", // wcsncat(dst, src, max_copy) widechar + // should be safe + // "wcsncat_s" // wcsncat_s(dst, dst_size, src, max_copy) + // "wcslcat" // wcslcat(dst, src, dst_size) + ]) or this.hasGlobalName([ - "_mbsncat", // _mbsncat(dst, src, max_copy) multibyte - "_mbsncat_l", // _mbsncat_l(dst, src, max_copy, locale) multibyte - "_mbsnbcat", // _mbsnbcat(dst, src, max_copy) - "_mbsnbcat_l" // _mbsnbcat_l(dst, src, max_copy, locale) - // should be safe - // "_mbsnbcat_s" - // "_mbsnbcat_s_l" - - // STRL should be safe - // "strlcat", // strlcat(dst, src, dst_size) - // "__builtin___strlcat_chk", // __builtin___strlcat_chk(dst, src, dst_size) - ]) - - // MEMCPY like + "_mbsncat", // _mbsncat(dst, src, max_copy) multibyte + "_mbsncat_l", // _mbsncat_l(dst, src, max_copy, locale) multibyte + "_mbsnbcat", // _mbsnbcat(dst, src, max_copy) + "_mbsnbcat_l" // _mbsnbcat_l(dst, src, max_copy, locale) + // should be safe + // "_mbsnbcat_s" + // "_mbsnbcat_s_l" + // STRL should be safe + // "strlcat", // strlcat(dst, src, dst_size) + // "__builtin___strlcat_chk", // __builtin___strlcat_chk(dst, src, dst_size) + ]) or + // MEMCPY like this.hasGlobalOrStdOrBslName([ - "memcpy", // memcpy(dst, src, max_copy) - "__builtin___memcpy_chk", // __builtin___memcpy_chk (dst, src, max_copy, os) - "__builtin_memcpy", // __builtin_memcpy(dst, src, max_copy, os) - "memmove", // memmove(dst, src, max_copy) - " __builtin___memmove_chk", // __builtin___memmove_chk(dst, src, max_copy, os) - "__builtin_memmove", // __builtin_memmove(dst, src, max_copy) - "wmemmove" // wmemmove(dst, src, max_copy) widechar - ]) + "memcpy", // memcpy(dst, src, max_copy) + "__builtin___memcpy_chk", // __builtin___memcpy_chk (dst, src, max_copy, os) + "__builtin_memcpy", // __builtin_memcpy(dst, src, max_copy, os) + "memmove", // memmove(dst, src, max_copy) + " __builtin___memmove_chk", // __builtin___memmove_chk(dst, src, max_copy, os) + "__builtin_memmove", // __builtin_memmove(dst, src, max_copy) + "wmemmove" // wmemmove(dst, src, max_copy) widechar + ]) or this.hasGlobalName([ - "bcopy", // bcopy(src, dst, max_copy) - "mempcpy", // mempcpy(dst, src, max_copy) - " __builtin___mempcpy_chk", // __builtin___mempcpy_chk(dst, src, max_copy, os) - "__builtin_mempcpy", // __builtin_mempcpy(dst, src, max_copy) - "memccpy", // memccpy(dst, src, c, max_copy) - "__builtin___memccpy_chk", // __builtin___memccpy_chk() TODO - args - "wmemcpy", // wmemcpy(dst, src, max_copy) widechar - "wmempcpy", // wmempcpy(dst, src, max_copy) widechar - "wmemccpy", // wmemccpy(dst, src, max_copy) widechar - "wcpncpy", // wcpncpy(dst, src, max_copy) widechar - "RtlCopyMemoryNonTemporal" // RtlCopyMemoryNonTemporal(dst, src, max_copy) - ]) + "bcopy", // bcopy(src, dst, max_copy) + "mempcpy", // mempcpy(dst, src, max_copy) + " __builtin___mempcpy_chk", // __builtin___mempcpy_chk(dst, src, max_copy, os) + "__builtin_mempcpy", // __builtin_mempcpy(dst, src, max_copy) + "memccpy", // memccpy(dst, src, c, max_copy) + "__builtin___memccpy_chk", // __builtin___memccpy_chk() TODO - args + "wmemcpy", // wmemcpy(dst, src, max_copy) widechar + "wmempcpy", // wmempcpy(dst, src, max_copy) widechar + "wmemccpy", // wmemccpy(dst, src, max_copy) widechar + "wcpncpy", // wcpncpy(dst, src, max_copy) widechar + "RtlCopyMemoryNonTemporal" // RtlCopyMemoryNonTemporal(dst, src, max_copy) + ]) } - override int getSourceArgs() { - if this.getName() in ["bcopy"] then - result = 0 - else - result = 1 - } + override int getSourceArgs() { if this.getName() in ["bcopy"] then result = 0 else result = 1 } override int getSizeArg() { - if this.getName() in ["memccpy", "__builtin___memccpy_chk"] then - result = 3 - else - result = 2 + if this.getName() in ["memccpy", "__builtin___memccpy_chk"] then result = 3 else result = 2 } } /* * Finds constant numeric value of the given expression, if possible */ -int getKnownInt(Expr argument) { - result = argument.getValue().toInt() -} + +int getKnownInt(Expr argument) { result = argument.getValue().toInt() } /* * Finds length of given string, if possible * The length does not count terminating nullbyte - */ + */ + int getKnownLen(Expr sourceArg) { result = sourceArg.(StringLiteral).getOriginalLength() - 1 or exists(LocalScopeVariable v, SsaDefinition ssaDef | - sourceArg = ssaDef.getAUse(v) - and result = ssaDef.getAnUltimateDefiningValue(v).(StringLiteral).getOriginalLength() - 1 + sourceArg = ssaDef.getAUse(v) and + result = ssaDef.getAnUltimateDefiningValue(v).(StringLiteral).getOriginalLength() - 1 ) } // TODO: review against widechars and multibytes -from StrNFunction f, FunctionCall fc, int sourceSize, int sizeArgValue, string msg +from StrNFunction f, FunctionCall fc, int sourceSize, int sizeArgValue, string msg where // find calls to interesting functions - f = fc.getTarget() - + f = fc.getTarget() and // consider only the shorter argument for two-arg functions (e.g., strncmp) - and sourceSize = min(int sourceSizeTmp | - sourceSizeTmp = getKnownLen(fc.getArgument(f.getSourceArgs())) | - sourceSizeTmp - ) - + sourceSize = + min(int sourceSizeTmp | + sourceSizeTmp = getKnownLen(fc.getArgument(f.getSourceArgs())) + | + sourceSizeTmp + ) and // get size argument - and sizeArgValue = getKnownInt(fc.getArgument(f.getSizeArg())) - - and + sizeArgValue = getKnownInt(fc.getArgument(f.getSizeArg())) and ( // typo bug // for "testabc" report sizes 6 and 5; 7 and greater sizes are fine; greater typos are probably intended - ( - sourceSize - sizeArgValue = [1, 2] - and msg = "Not whole source buffer is consumed, which may be a bug." - ) + sourceSize - sizeArgValue = [1, 2] and + msg = "Not whole source buffer is consumed, which may be a bug." or ( - sizeArgValue - 1 > sourceSize - and - if f.getName().matches("%mem%") or f.getName() in ["RtlCopyMemoryNonTemporal", "bcopy"] then - msg = "This is a buffer overread vulnerability." - else if f.getName().matches("%cat%") then - msg = "This indicates that the destination buffer may overflow, as the size argument limits amount of bytes copied, and not total length." + sizeArgValue - 1 > sourceSize and + if f.getName().matches("%mem%") or f.getName() in ["RtlCopyMemoryNonTemporal", "bcopy"] + then msg = "This is a buffer overread vulnerability." else - none() + if f.getName().matches("%cat%") + then + msg = + "This indicates that the destination buffer may overflow, as the size argument limits amount of bytes copied, and not total length." + else none() ) ) - -select fc, "Call to " + f.getName() + " receives an input string of size " + sourceSize + - ", but is passed size argument of " + sizeArgValue + ". " + msg +select fc, + "Call to " + f.getName() + " receives an input string of size " + sourceSize + + ", but is passed size argument of " + sizeArgValue + ". " + msg diff --git a/cpp/src/security/NoNullTerminator/NoNullTerminator.ql b/cpp/src/security/NoNullTerminator/NoNullTerminator.ql index 69d04c4..b197cab 100644 --- a/cpp/src/security/NoNullTerminator/NoNullTerminator.ql +++ b/cpp/src/security/NoNullTerminator/NoNullTerminator.ql @@ -16,41 +16,34 @@ import semmle.code.cpp.models.implementations.Strcpy import semmle.code.cpp.commons.Printf import semmle.code.cpp.commons.StringAnalysis - class NullStrFunction extends Function { int strPosition; NullStrFunction() { - (this instanceof StrcpyFunction and strPosition = this.(StrcpyFunction).getParamSrc()) + this instanceof StrcpyFunction and strPosition = this.(StrcpyFunction).getParamSrc() or - (this instanceof FormattingFunction and strPosition = [0 .. this.getNumberOfParameters()]) + this instanceof FormattingFunction and strPosition = [0 .. this.getNumberOfParameters()] or - ( - this.getName() = [ - "strcasecmp", "strstr", "strcasecmp_l", "strcasestr", "strcasestr_l", - "strcoll_l", "strlcpy", "strlcat", "strtok_r", "strcat", "strcmp", "strcoll", - "strspn", "strcspn", "strtok", "strpbrk" - ] - and strPosition = [0, 1] - ) + this.getName() = + [ + "strcasecmp", "strstr", "strcasecmp_l", "strcasestr", "strcasestr_l", "strcoll_l", + "strlcpy", "strlcat", "strtok_r", "strcat", "strcmp", "strcoll", "strspn", "strcspn", + "strtok", "strpbrk" + ] and + strPosition = [0, 1] or - ( - this.getName() = [ - "strchr", "strrchr", "strlen", "wcslen", "_mbslen", "_mbslen_l", - "_mbstrlen", "_mbstrlen_l", "strdup", "perror" - ] - and strPosition = 0 - ) + this.getName() = + [ + "strchr", "strrchr", "strlen", "wcslen", "_mbslen", "_mbslen_l", "_mbstrlen", "_mbstrlen_l", + "strdup", "perror" + ] and + strPosition = 0 or - ( - this.getName() = ["strvis", "stravis"] - and strPosition = 1 - ) + this.getName() = ["strvis", "stravis"] and + strPosition = 1 } - int getStrPosition() { - result = strPosition - } + int getStrPosition() { result = strPosition } } module StrFlowConfig implements DataFlow::ConfigSig { @@ -60,8 +53,8 @@ module StrFlowConfig implements DataFlow::ConfigSig { predicate isSink(DataFlow::Node sink) { exists(FunctionCall fc, NullStrFunction f | - fc.getTarget() = f - and sink.asExpr() = fc.getArgument(f.getStrPosition()) + fc.getTarget() = f and + sink.asExpr() = fc.getArgument(f.getStrPosition()) ) } } @@ -71,36 +64,41 @@ module StrFlow = DataFlow::Global; // based on https://github.com/github/codeql/blob/main/cpp/ql/src/Security/CWE/CWE-676/DangerousUseOfCin.ql class AnyCharArrayType extends ArrayType { AnyCharArrayType() { - (this.getBaseType().getUnderlyingType() instanceof CharType - or - this.getBaseType() instanceof Wchar_t) - and this.hasArraySize() + ( + this.getBaseType().getUnderlyingType() instanceof CharType + or + this.getBaseType() instanceof Wchar_t + ) and + this.hasArraySize() } } - -from Variable var, StringLiteral varInit, string msg, - int varSize, int varInitSize, FunctionCall fc, DataFlow::Node sink +from + Variable var, StringLiteral varInit, string msg, int varSize, int varInitSize, FunctionCall fc, + DataFlow::Node sink where - var.getType() instanceof AnyCharArrayType - and varSize = var.getType().getSize() - - and varInit = var.getInitializer().getExpr().getFullyConverted() - and varInitSize = varInit.getValue().length() * var.getType().(ArrayType).getBaseType().getSize() - - and ( + var.getType() instanceof AnyCharArrayType and + varSize = var.getType().getSize() and + varInit = var.getInitializer().getExpr().getFullyConverted() and + varInitSize = varInit.getValue().length() * var.getType().(ArrayType).getBaseType().getSize() and + ( // array won't have terminating null byte - (varSize = varInitSize and msg = ", as the arary size is equal to initialization string's length (" + varInitSize + " bytes).") + varSize = varInitSize and + msg = + ", as the arary size is equal to initialization string's length (" + varInitSize + " bytes)." or // should not be possible - (varSize < varInitSize and msg = ", as initialization string's length (" + varInitSize + ") is greater than the arary size (" + varSize + ").") - ) - + varSize < varInitSize and + msg = + ", as initialization string's length (" + varInitSize + ") is greater than the arary size (" + + varSize + ")." + ) and // TODO: there is no explicit nullbyte in the string // and not exists(int i | varInit.getValue().charAt(i) = "\u0000" ) - // the string flows to a str function (eg strcpy, strcat, printf) - and StrFlow::flow(DataFlow::exprNode(var.getAnAccess()), sink) - and fc.getAnArgument() = sink.asExpr() - -select var, "String is initialized without terminating null byte" + msg + ". String is then used as an argument to a function requiring null-terminated strings: $@", fc, fc.toString() \ No newline at end of file + StrFlow::flow(DataFlow::exprNode(var.getAnAccess()), sink) and + fc.getAnArgument() = sink.asExpr() +select var, + "String is initialized without terminating null byte" + msg + + ". String is then used as an argument to a function requiring null-terminated strings: $@", fc, + fc.toString() diff --git a/cpp/test/library-tests/crypto/CustomAllocator/CustomAllocatorCall.ql b/cpp/test/library-tests/crypto/CustomAllocator/CustomAllocatorCall.ql index 8dcf795..e33a317 100644 --- a/cpp/test/library-tests/crypto/CustomAllocator/CustomAllocatorCall.ql +++ b/cpp/test/library-tests/crypto/CustomAllocator/CustomAllocatorCall.ql @@ -1,11 +1,6 @@ import cpp import trailofbits.crypto.libraries - -from - CustomAllocatorCall alloc, - CustomDeallocatorCall dealloc -where - dealloc = alloc.getADeallocatorCall() -select - alloc, dealloc, dealloc.getPointer() +from CustomAllocatorCall alloc, CustomDeallocatorCall dealloc +where dealloc = alloc.getADeallocatorCall() +select alloc, dealloc, dealloc.getPointer() diff --git a/cpp/test/library-tests/crypto/InitUpdateFinalTuple/InitUpdateFinalTuple.ql b/cpp/test/library-tests/crypto/InitUpdateFinalTuple/InitUpdateFinalTuple.ql index 90448db..ee9d283 100644 --- a/cpp/test/library-tests/crypto/InitUpdateFinalTuple/InitUpdateFinalTuple.ql +++ b/cpp/test/library-tests/crypto/InitUpdateFinalTuple/InitUpdateFinalTuple.ql @@ -1,8 +1,5 @@ import cpp import trailofbits.crypto.libraries - -from - InitUpdateFinalTuple tuple -select - tuple.getAnInitCall(), tuple.getAnUpdateCall(), tuple.getAFinalCall() +from InitUpdateFinalTuple tuple +select tuple.getAnInitCall(), tuple.getAnUpdateCall(), tuple.getAFinalCall() diff --git a/cpp/test/library-tests/crypto/StaticKeyAccess/StaticKeyAccess.ql b/cpp/test/library-tests/crypto/StaticKeyAccess/StaticKeyAccess.ql index 6688b62..fd5e698 100644 --- a/cpp/test/library-tests/crypto/StaticKeyAccess/StaticKeyAccess.ql +++ b/cpp/test/library-tests/crypto/StaticKeyAccess/StaticKeyAccess.ql @@ -1,5 +1,5 @@ import cpp import trailofbits.crypto.libraries - -from StaticKeyAccess key select key +from StaticKeyAccess key +select key diff --git a/cpp/test/library-tests/crypto/StaticKeyLiteral/StaticKeyLiteral.ql b/cpp/test/library-tests/crypto/StaticKeyLiteral/StaticKeyLiteral.ql index 975c90b..2a84fc0 100644 --- a/cpp/test/library-tests/crypto/StaticKeyLiteral/StaticKeyLiteral.ql +++ b/cpp/test/library-tests/crypto/StaticKeyLiteral/StaticKeyLiteral.ql @@ -1,8 +1,5 @@ import cpp import trailofbits.crypto.libraries - -from - StaticKeyLiteral key -select - key +from StaticKeyLiteral key +select key diff --git a/go/src/crypto/MsgNotHashedBeforeSigVerfication/MsgNotHashedBeforeSigVerfication.ql b/go/src/crypto/MsgNotHashedBeforeSigVerfication/MsgNotHashedBeforeSigVerfication.ql index 2e8314b..0b35b55 100644 --- a/go/src/crypto/MsgNotHashedBeforeSigVerfication/MsgNotHashedBeforeSigVerfication.ql +++ b/go/src/crypto/MsgNotHashedBeforeSigVerfication/MsgNotHashedBeforeSigVerfication.ql @@ -6,7 +6,7 @@ * @tags security * @problem.severity error * @precision medium - * @security-severity 8.0 + * @security-severity 8.0 * @group cryptography */ @@ -17,141 +17,145 @@ import go * and silently truncates the input hash if it is longer than expected */ class SignatureMsgTruncationFunction extends Function { - int msgHashPosition; - - SignatureMsgTruncationFunction() { - // https://cs.opensource.google/go/go/+/refs/tags/go1.20.2:src/crypto/ecdsa/ecdsa.go;l=376-382;drc=457fd1d52d17fc8e73d4890150eadab3128de64d - (this.hasQualifiedName("crypto/ecdsa", ["VerifyASN1", "Verify"]) and msgHashPosition = 1) - or - (this.hasQualifiedName("crypto/ecdsa", ["SignASN1", "Sign"]) and msgHashPosition = 2) - or - - // doesn't truncate, but also doesn't hash internally - // https://cs.opensource.google/go/go/+/refs/tags/go1.20.2:src/crypto/dsa/dsa.go;l=296 - (this.hasQualifiedName("crypto/dsa", "Verify") and msgHashPosition = 1) - or - (this.hasQualifiedName("crypto/dsa", "Sign") and msgHashPosition = 2) - or - - // https://github.com/btcsuite/btcd/blob/2f508b3f86ed9ef87bcf3426b87b6c0dc0d3632c/btcec/signature.go#L256-L263 - (this.hasQualifiedName("github.com/btcsuite/btcd/btcec", "SignCompact") and msgHashPosition = 2) - or - (this.hasQualifiedName("github.com/btcsuite/btcd/btcec.PrivateKey", "Sign") and msgHashPosition = 0) - or - (this.hasQualifiedName("github.com/btcsuite/btcd/btcec.PublicKey", "RecoverCompact") and msgHashPosition = 2) - or - (this.hasQualifiedName("github.com/btcsuite/btcd/btcec.Signature", "Verify") and msgHashPosition = 0) - or - - // https://github.com/btcsuite/btcd/blob/2f508b3f86ed9ef87bcf3426b87b6c0dc0d3632c/btcec/signature.go#L256-L263 - (this.hasQualifiedName("github.com/umbracle/ethgo/wallet", "Ecrecover") and msgHashPosition = 0) - or - (this.hasQualifiedName("github.com/umbracle/ethgo/wallet", "RecoverPubkey") and msgHashPosition = 1) - or - (this.hasQualifiedName("github.com/umbracle/ethgo/wallet.Key", "Sign") and msgHashPosition = 0) - or - - // https://github.com/decred/dcrd/blob/3bc18a3be50822100773789cb3ecff08cb17f938/dcrec/secp256k1/modnscalar.go#L345-L356 - ( - exists(string dcrdVersion | dcrdVersion = ["", "/v2", "/v3", "/v4"] | - (this.hasQualifiedName("github.com/decred/dcrd/dcrec/secp256k1" + dcrdVersion, ["RecoverCompact", "RecoverCompact", "Sign"]) and msgHashPosition = 1) - or - (this.hasQualifiedName("github.com/decred/dcrd/dcrec/secp256k1" + dcrdVersion + ".Signature", "Verify") and msgHashPosition = 0) - ) - ) - - // examples of secure implementations: - // https://github.com/ethereum/go-ethereum/blob/v1.11.3/crypto/secp256k1/secp256.go#L127 - // https://cs.opensource.google/go/go/+/refs/tags/go1.20.2:src/crypto/ed25519/ed25519.go;l=322;drc=457fd1d52d17fc8e73d4890150eadab3128de64d - // https://github.com/btccom/secp256k1-go/blob/master/secp256k1/secp256k1.go#L270-L274 - } + int msgHashPosition; + + SignatureMsgTruncationFunction() { + // https://cs.opensource.google/go/go/+/refs/tags/go1.20.2:src/crypto/ecdsa/ecdsa.go;l=376-382;drc=457fd1d52d17fc8e73d4890150eadab3128de64d + this.hasQualifiedName("crypto/ecdsa", ["VerifyASN1", "Verify"]) and msgHashPosition = 1 + or + this.hasQualifiedName("crypto/ecdsa", ["SignASN1", "Sign"]) and msgHashPosition = 2 + or + // doesn't truncate, but also doesn't hash internally + // https://cs.opensource.google/go/go/+/refs/tags/go1.20.2:src/crypto/dsa/dsa.go;l=296 + this.hasQualifiedName("crypto/dsa", "Verify") and msgHashPosition = 1 + or + this.hasQualifiedName("crypto/dsa", "Sign") and msgHashPosition = 2 + or + // https://github.com/btcsuite/btcd/blob/2f508b3f86ed9ef87bcf3426b87b6c0dc0d3632c/btcec/signature.go#L256-L263 + this.hasQualifiedName("github.com/btcsuite/btcd/btcec", "SignCompact") and msgHashPosition = 2 + or + this.hasQualifiedName("github.com/btcsuite/btcd/btcec.PrivateKey", "Sign") and + msgHashPosition = 0 + or + this.hasQualifiedName("github.com/btcsuite/btcd/btcec.PublicKey", "RecoverCompact") and + msgHashPosition = 2 + or + this.hasQualifiedName("github.com/btcsuite/btcd/btcec.Signature", "Verify") and + msgHashPosition = 0 + or + // https://github.com/btcsuite/btcd/blob/2f508b3f86ed9ef87bcf3426b87b6c0dc0d3632c/btcec/signature.go#L256-L263 + this.hasQualifiedName("github.com/umbracle/ethgo/wallet", "Ecrecover") and msgHashPosition = 0 + or + this.hasQualifiedName("github.com/umbracle/ethgo/wallet", "RecoverPubkey") and + msgHashPosition = 1 + or + this.hasQualifiedName("github.com/umbracle/ethgo/wallet.Key", "Sign") and msgHashPosition = 0 + or + // https://github.com/decred/dcrd/blob/3bc18a3be50822100773789cb3ecff08cb17f938/dcrec/secp256k1/modnscalar.go#L345-L356 + exists(string dcrdVersion | dcrdVersion = ["", "/v2", "/v3", "/v4"] | + this.hasQualifiedName("github.com/decred/dcrd/dcrec/secp256k1" + dcrdVersion, + ["RecoverCompact", "RecoverCompact", "Sign"]) and + msgHashPosition = 1 + or + this.hasQualifiedName("github.com/decred/dcrd/dcrec/secp256k1" + dcrdVersion + ".Signature", + "Verify") and + msgHashPosition = 0 + ) + // examples of secure implementations: + // https://github.com/ethereum/go-ethereum/blob/v1.11.3/crypto/secp256k1/secp256.go#L127 + // https://cs.opensource.google/go/go/+/refs/tags/go1.20.2:src/crypto/ed25519/ed25519.go;l=322;drc=457fd1d52d17fc8e73d4890150eadab3128de64d + // https://github.com/btccom/secp256k1-go/blob/master/secp256k1/secp256k1.go#L270-L274 + } - predicate hashArgPosition(int position) { - msgHashPosition = position - } + predicate hashArgPosition(int position) { msgHashPosition = position } } /** * Models a hash function - * The list is extended from (private) experimental/CWE-327/CryptoLibraries.qll + * The list is extended from (private) experimental/CWE-327/CryptoLibraries.qll * TODO: received methods support, e.g., package.Hash(msg).Sum() */ class HashFunction extends Function { - HashFunction() { - this.getName().toUpperCase().matches([ + HashFunction() { + this.getName() + .toUpperCase() + .matches([ "DSA", "ED25519", "SHA256", "SHA384", "SHA512", "SHA3", "ES256", "ECDSA256", "ES384", - "ECDSA384", "ES512", "ECDSA512", "SHA2", "SHA224", "RIPEMD320", "SHA0", - "HAVEL128", "MD2", "SHA1", "MD4", "MD5", "PANAMA", "RIPEMD", "RIPEMD128", "RIPEMD256", - "ARGON2", "PBKDF2", "BCRYPT", "SCRYPT", - "KECCAK256", "KECCAK256RLP", "HASH" - ]) - } + "ECDSA384", "ES512", "ECDSA512", "SHA2", "SHA224", "RIPEMD320", "SHA0", "HAVEL128", + "MD2", "SHA1", "MD4", "MD5", "PANAMA", "RIPEMD", "RIPEMD128", "RIPEMD256", "ARGON2", + "PBKDF2", "BCRYPT", "SCRYPT", "KECCAK256", "KECCAK256RLP", "HASH" + ]) + } } private module LongestFlowConfig implements DataFlow::ConfigSig { - predicate isSource(DataFlow::Node source) { source = source } - predicate isSink(DataFlow::Node sink) { sink = sink } + predicate isSource(DataFlow::Node source) { source = source } + + predicate isSink(DataFlow::Node sink) { sink = sink } } + module LongestFlowFlow = DataFlow::Global; /** * Flows from anything to SignatureMsgTruncationFunction * that do not cross a hash function or slicing expression */ -module AnythingToSignatureMsgTrunFuncConfig implements DataFlow::ConfigSig { - // anything that is not a function's argument - // TODO: alternatively, set sources to be ExternalInputs - predicate isSource(DataFlow::Node source) { - not isSink(source) - and not source.asInstruction() instanceof IR::ReadArgumentInstruction - } - - predicate isSink(DataFlow::Node sink) { - exists(SignatureMsgTruncationFunction sigUseF, CallExpr sigUseCall, int position | - sigUseCall.getTarget() = sigUseF - and sigUseF.hashArgPosition(position) - and sink.asExpr() = sigUseCall.getArgument(position) - ) - } - - // sanitize if - // * data goes through a hash function - // * data is truncated with a hardcoded value - // * TODO: data is of type Hash - predicate isBarrier(DataFlow::Node node) { - // direct hash function call - exists(HashFunction hf | hf.getACall().getResult(_) = node or hf.getACall().getArgument(_) = node) - or - // hash function call via .Sum() - node.(DataFlow::CallNode).getTarget().hasQualifiedName("hash.Hash" + ["", "32", "64"], "Sum") - or - // slicing - exists(SliceExpr e | - e = node.asExpr() - and - (e.getHigh().isConst() or e.getLow().isConst()) - ) - // note that not hashing message may be bad even for short messages; 66 is max truncation size - or - node.asExpr().(SliceExpr).getBase().getType().getUnderlyingType().(ArrayType).getLength() <= 66 - or - node.asExpr().getType().getUnderlyingType().(ArrayType).getLength() <= 66 - } +module AnythingToSignatureMsgTrunFuncConfig implements DataFlow::ConfigSig { + // anything that is not a function's argument + // TODO: alternatively, set sources to be ExternalInputs + predicate isSource(DataFlow::Node source) { + not isSink(source) and + not source.asInstruction() instanceof IR::ReadArgumentInstruction + } + + predicate isSink(DataFlow::Node sink) { + exists(SignatureMsgTruncationFunction sigUseF, CallExpr sigUseCall, int position | + sigUseCall.getTarget() = sigUseF and + sigUseF.hashArgPosition(position) and + sink.asExpr() = sigUseCall.getArgument(position) + ) + } + + // sanitize if + // * data goes through a hash function + // * data is truncated with a hardcoded value + // * TODO: data is of type Hash + predicate isBarrier(DataFlow::Node node) { + // direct hash function call + exists(HashFunction hf | + hf.getACall().getResult(_) = node or hf.getACall().getArgument(_) = node + ) + or + // hash function call via .Sum() + node.(DataFlow::CallNode).getTarget().hasQualifiedName("hash.Hash" + ["", "32", "64"], "Sum") + or + // slicing + exists(SliceExpr e | + e = node.asExpr() and + (e.getHigh().isConst() or e.getLow().isConst()) + ) + or + // note that not hashing message may be bad even for short messages; 66 is max truncation size + node.asExpr().(SliceExpr).getBase().getType().getUnderlyingType().(ArrayType).getLength() <= 66 + or + node.asExpr().getType().getUnderlyingType().(ArrayType).getLength() <= 66 + } } + module AnythingToSignatureMsgTrunFuncFlow = DataFlow::Global; + import AnythingToSignatureMsgTrunFuncFlow::PathGraph -from AnythingToSignatureMsgTrunFuncFlow::PathNode source, AnythingToSignatureMsgTrunFuncFlow::PathNode sink +from + AnythingToSignatureMsgTrunFuncFlow::PathNode source, + AnythingToSignatureMsgTrunFuncFlow::PathNode sink where - AnythingToSignatureMsgTrunFuncFlow::flowPath(source, sink) - - // only the longest flow - // TODO: find only flows originating from user input - and not exists(DataFlow::Node source2 | - LongestFlowFlow::flow(source2, source.getNode()) - and source2 != source.getNode() - ) - - // TODO: ignore if conditionally hashed - + AnythingToSignatureMsgTrunFuncFlow::flowPath(source, sink) and + // only the longest flow + // TODO: find only flows originating from user input + not exists(DataFlow::Node source2 | + LongestFlowFlow::flow(source2, source.getNode()) and + source2 != source.getNode() + ) +// TODO: ignore if conditionally hashed select sink.getNode(), source, sink, "Message must be hashed before signing/veryfing operation" diff --git a/go/src/security/FilePermsFlaws/FilePermsFlaws.ql b/go/src/security/FilePermsFlaws/FilePermsFlaws.ql index 9247367..bdba885 100644 --- a/go/src/security/FilePermsFlaws/FilePermsFlaws.ql +++ b/go/src/security/FilePermsFlaws/FilePermsFlaws.ql @@ -15,6 +15,7 @@ import go /* * From https://github.com/github/codeql/blob/ef0ea247c40da805efa427a37f5213457d18f714/cpp/ql/src/Security/CWE/CWE-732/FilePermissions.qll#L4-L21 */ + bindingset[n, digit] private string octalDigit(int n, int digit) { result = n.bitShiftRight(digit * 3).bitAnd(7).toString() @@ -47,47 +48,46 @@ class PermChangingMethod extends CallExpr { /* * TODO: on windows OS, only '0400' and '0600' should be used https://golang.org/pkg/os/#Chmod */ + from Expr fileModeExpr, IntLit fileModeLit, int fileModeInt, string fileModeLitStr, string message where // get a FileMode - fileModeExpr.getType().getName() = "FileMode" - + fileModeExpr.getType().getName() = "FileMode" and // go over all numeric constants in expressions like `013 | 37 & os.ModeSticky` - and fileModeLit.getParent*() = fileModeExpr - + fileModeLit.getParent*() = fileModeExpr and // get integer and string values of the mode - and fileModeLit.getIntValue() = fileModeInt - and fileModeLitStr = fileModeLit.toString() - + fileModeLit.getIntValue() = fileModeInt and + fileModeLitStr = fileModeLit.toString() and // skip reporting if a popular decimal constant is used - and not isKnownValidConstant(fileModeLitStr) - + not isKnownValidConstant(fileModeLitStr) and // report early if special bits are set in modes for function calls // Go uses only 9 permission bits for Mkdir, Chmod etc // and support for other bits is platform specific // for details see https://github.com/golang/go/issues/25539#issuecomment-394484403 // TODO: use only permission-changing methods // chmod - and if fileModeInt > 511 /* 0o777 */ and exists(PermChangingMethod use | use.getAnArgument() = fileModeExpr) + if + fileModeInt > 511 and + /* 0o777 */ exists(PermChangingMethod use | use.getAnArgument() = fileModeExpr) then - message = "- only 9 permission bits are supported in Mkdir/MkdirAll/Chmod methods on all platforms" + message = + "- only 9 permission bits are supported in Mkdir/MkdirAll/Chmod methods on all platforms" else ( // skip reporting if the mode is large - probably a mask - not fileModeInt > 1911 /* 0x777 */ - + not fileModeInt > 1911 and + /* 0x777 */ // the most interesting case of number encoding typos - and exists(string fileModeAsOctal, string fileModeAsSeen | + exists(string fileModeAsOctal, string fileModeAsSeen | // what you see; left-pad with three zeros - fileModeAsSeen = ("000" + fileModeLitStr.replaceAll("_", "").regexpCapture("(0o|0x|0b)?(.+)", 2)).regexpCapture("0*(.{3,})", 1) - + fileModeAsSeen = + ("000" + fileModeLitStr.replaceAll("_", "").regexpCapture("(0o|0x|0b)?(.+)", 2)) + .regexpCapture("0*(.{3,})", 1) and // what you get - and fileModeAsOctal = octalFileMode(fileModeInt) - + fileModeAsOctal = octalFileMode(fileModeInt) and // what you see != what you get - and fileModeAsSeen != fileModeAsOctal - and message = "will evaluate to 0o" + fileModeAsOctal + fileModeAsSeen != fileModeAsOctal and + message = "will evaluate to 0o" + fileModeAsOctal ) ) - - // TODO: skip numbers used in arithmetic operations and bitshifts +// TODO: skip numbers used in arithmetic operations and bitshifts select fileModeLit, "Found invalid permissions: " + fileModeLit + " " + message diff --git a/go/src/security/MissingMinVersionTLS/MissingMinVersionTLS.ql b/go/src/security/MissingMinVersionTLS/MissingMinVersionTLS.ql index baa0f8d..389855c 100644 --- a/go/src/security/MissingMinVersionTLS/MissingMinVersionTLS.ql +++ b/go/src/security/MissingMinVersionTLS/MissingMinVersionTLS.ql @@ -17,9 +17,9 @@ import semmle.go.GoMod as GoMod * Flow of a `tls.Config` to a write to the `MinVersion` field. */ module TlsVersionConfig implements DataFlow::ConfigSig { - /** - * Holds if `source` is a TLS.Config instance. - */ + /** + * Holds if `source` is a TLS.Config instance. + */ predicate isSource(DataFlow::Node source) { exists(Variable v | configOrConfigPointer(v.getType()) and @@ -27,9 +27,9 @@ module TlsVersionConfig implements DataFlow::ConfigSig { ) } - /** - * Holds if a write to `sink`.MinVersion exists. - */ + /** + * Holds if a write to `sink`.MinVersion exists. + */ predicate isSink(DataFlow::Node sink) { exists(Write fieldWrite, Field fld | fld.hasQualifiedName("crypto/tls", "Config", "MinVersion") and @@ -37,6 +37,7 @@ module TlsVersionConfig implements DataFlow::ConfigSig { ) } } + module TlsVersionFlow = TaintTracking::Global; predicate structLitSetsMinVersion(StructLit lit) { @@ -47,9 +48,9 @@ predicate structLitSetsMinVersion(StructLit lit) { } module TlsConfigCreationConfig implements DataFlow::ConfigSig { - /** - * Holds if `source` is a TLS.Config literal without MinVersion set. - */ + /** + * Holds if `source` is a TLS.Config literal without MinVersion set. + */ predicate isSource(DataFlow::Node source) { exists(StructLit lit | lit.getType().hasQualifiedName("crypto/tls", "Config") and @@ -58,42 +59,42 @@ module TlsConfigCreationConfig implements DataFlow::ConfigSig { ) } - /** - * Holds if it is TLS.Config instance (a Variable). - */ - predicate isSink(DataFlow::Node sink) { - exists(Variable v | - sink.asExpr() = v.getAReference() - ) - } + /** + * Holds if it is TLS.Config instance (a Variable). + */ + predicate isSink(DataFlow::Node sink) { exists(Variable v | sink.asExpr() = v.getAReference()) } - /** - * Holds if TLS.Config literal is saved in a structure's field - */ + /** + * Holds if TLS.Config literal is saved in a structure's field + */ predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ) { exists(Write w | w.writesField(succ, _, pred)) } } + module TlsConfigCreationFlow = TaintTracking::Global; /** * Holds if `t` is a TLS.Config type or a pointer to it (or ptr to ptr...) or a struct containing it. */ predicate configOrConfigPointer(Type t) { - t.hasQualifiedName("crypto/tls", "Config") or - exists(Type tp | - tp.hasQualifiedName("crypto/tls", "Config") and - t = tp.getPointerType+() - ) or - exists(Type tp | - tp.hasQualifiedName("crypto/tls", "Config") and - t.(NamedType).getUnderlyingType().(StructType).hasField(_, tp) - ) or - exists(Type tp, Type tp2 | - tp.hasQualifiedName("crypto/tls", "Config") and - tp2 = tp.getPointerType+() and - t.(NamedType).getUnderlyingType().(StructType).hasField(_, tp2) - ) + t.hasQualifiedName("crypto/tls", "Config") + or + exists(Type tp | + tp.hasQualifiedName("crypto/tls", "Config") and + t = tp.getPointerType+() + ) + or + exists(Type tp | + tp.hasQualifiedName("crypto/tls", "Config") and + t.(NamedType).getUnderlyingType().(StructType).hasField(_, tp) + ) + or + exists(Type tp, Type tp2 | + tp.hasQualifiedName("crypto/tls", "Config") and + tp2 = tp.getPointerType+() and + t.(NamedType).getUnderlyingType().(StructType).hasField(_, tp2) + ) } /** @@ -102,7 +103,7 @@ predicate configOrConfigPointer(Type t) { */ bindingset[v] predicate goVersionAtLeast_1_18(string v) { - v.regexpMatch("1\\.(1[89]|[2-9][0-9]|[1-9][0-9]{2,})(\\.\\d+)?") + v.regexpMatch("1\\.(1[89]|[2-9][0-9]|[1-9][0-9]{2,})(\\.\\d+)?") } /** @@ -111,7 +112,7 @@ predicate goVersionAtLeast_1_18(string v) { */ bindingset[v] predicate goVersionAtLeast_1_22(string v) { - v.regexpMatch("1\\.(2[2-9]|[3-9][0-9]|[1-9][0-9]{2,})(\\.\\d+)?") + v.regexpMatch("1\\.(2[2-9]|[3-9][0-9]|[1-9][0-9]{2,})(\\.\\d+)?") } /** @@ -123,9 +124,7 @@ predicate goVersionAtLeast_1_22(string v) { */ predicate projectSupportsOldTlsDefaultsForServers() { not exists(GoModGoLine l | l = l) or - exists(GoModGoLine l | - not goVersionAtLeast_1_22(l.getVersion()) - ) + exists(GoModGoLine l | not goVersionAtLeast_1_22(l.getVersion())) } /** @@ -137,17 +136,15 @@ predicate projectSupportsOldTlsDefaultsForServers() { */ predicate projectSupportsOldTlsDefaultsForClients() { not exists(GoModGoLine l | l = l) or - exists(GoModGoLine l | - not goVersionAtLeast_1_18(l.getVersion()) - ) + exists(GoModGoLine l | not goVersionAtLeast_1_18(l.getVersion())) } /** * Holds if expression `e` mentions the config variable or struct literal. */ predicate mentionsConfig(Expr e, Variable v, StructLit configStruct) { - e.getAChild*() = v.getARead().asExpr() or - e.getAChild*() = configStruct + e.getAChild*() = v.getARead().asExpr() or + e.getAChild*() = configStruct } /** @@ -168,19 +165,19 @@ predicate usedAsClient(Variable v, StructLit configStruct) { ) or exists(CallExpr call | - // tls.Dial(network, addr, config) => argument 2 is config (0-based) + // tls.Dial(network, addr, config) => argument 2 is config (0-based) call.getTarget().hasQualifiedName("crypto/tls", "Dial") and mentionsConfig(call.getArgument(2), v, configStruct) ) or exists(CallExpr call | - // tls.DialWithDialer(dialer, network, addr, config) => argument 3 is config + // tls.DialWithDialer(dialer, network, addr, config) => argument 3 is config call.getTarget().hasQualifiedName("crypto/tls", "DialWithDialer") and mentionsConfig(call.getArgument(3), v, configStruct) ) or exists(CallExpr call | - // tls.Client(conn, config) => argument 1 is config + // tls.Client(conn, config) => argument 1 is config call.getTarget().hasQualifiedName("crypto/tls", "Client") and mentionsConfig(call.getArgument(1), v, configStruct) ) @@ -204,19 +201,19 @@ predicate usedAsServer(Variable v, StructLit configStruct) { ) or exists(CallExpr call | - // tls.Listen(network, addr, config) => argument 2 is config + // tls.Listen(network, addr, config) => argument 2 is config call.getTarget().hasQualifiedName("crypto/tls", "Listen") and mentionsConfig(call.getArgument(2), v, configStruct) ) or exists(CallExpr call | - // tls.NewListener(inner, config) => argument 1 is config + // tls.NewListener(inner, config) => argument 1 is config call.getTarget().hasQualifiedName("crypto/tls", "NewListener") and mentionsConfig(call.getArgument(1), v, configStruct) ) or exists(CallExpr call | - // tls.Server(conn, config) => argument 1 is config + // tls.Server(conn, config) => argument 1 is config call.getTarget().hasQualifiedName("crypto/tls", "Server") and mentionsConfig(call.getArgument(1), v, configStruct) ) @@ -225,44 +222,42 @@ predicate usedAsServer(Variable v, StructLit configStruct) { // v - a variable holding any structure which is or contains the tls.Config from StructLit configStruct, Variable v, DataFlow::Node source, DataFlow::Node sink where - // find tls.Config structures with MinVersion not set on the structure initialization - ( - TlsConfigCreationFlow::flow(source, sink) and - sink.asExpr() = v.getAReference() and - source.asExpr() = configStruct - ) - - // only explicitely defined, e.g., skip function arguments - and ( - exists(DeclStmt decl | v.getAReference() = decl.getAChild+()) - or + // find tls.Config structures with MinVersion not set on the structure initialization + ( + TlsConfigCreationFlow::flow(source, sink) and + sink.asExpr() = v.getAReference() and + source.asExpr() = configStruct + ) and + // only explicitely defined, e.g., skip function arguments + ( + exists(DeclStmt decl | v.getAReference() = decl.getAChild+()) + or exists(DefineStmt decl | v.getAReference() = decl.getAChild+()) - ) - - // skip field declarations - and not exists(FieldDecl decl | v.getAReference() = decl.getAChild+()) - - // if the tls.Config is assigned to a variable - and if configOrConfigPointer(v.getType()) then + ) and + // skip field declarations + not exists(FieldDecl decl | v.getAReference() = decl.getAChild+()) and + // if the tls.Config is assigned to a variable ( - // exclude if there is a later write to MinVersion - not exists(DataFlow::Node source2, DataFlow::Node sink2 | - TlsVersionFlow::flow(source2, sink2) and - source2.asExpr() = v.getAReference() - ) - ) else - any() - - // Version-aware filtering based on client vs server usage: - // - For clients: only flag if Go < 1.18 (when client default is TLS 1.0) - // - For servers: only flag if Go < 1.22 (when server default is TLS 1.0) - // - If neither classified, be conservative as "server-like" - and ( - (usedAsClient(v, configStruct) and projectSupportsOldTlsDefaultsForClients()) - or - (usedAsServer(v, configStruct) and projectSupportsOldTlsDefaultsForServers()) - or - (not usedAsClient(v, configStruct) and not usedAsServer(v, configStruct) and projectSupportsOldTlsDefaultsForServers()) + if configOrConfigPointer(v.getType()) + then + // exclude if there is a later write to MinVersion + not exists(DataFlow::Node source2, DataFlow::Node sink2 | + TlsVersionFlow::flow(source2, sink2) and + source2.asExpr() = v.getAReference() + ) + else any() + ) and + // Version-aware filtering based on client vs server usage: + // - For clients: only flag if Go < 1.18 (when client default is TLS 1.0) + // - For servers: only flag if Go < 1.22 (when server default is TLS 1.0) + // - If neither classified, be conservative as "server-like" + ( + usedAsClient(v, configStruct) and projectSupportsOldTlsDefaultsForClients() + or + usedAsServer(v, configStruct) and projectSupportsOldTlsDefaultsForServers() + or + not usedAsClient(v, configStruct) and + not usedAsServer(v, configStruct) and + projectSupportsOldTlsDefaultsForServers() ) - -select configStruct, "TLS.Config.MinVersion is never set for variable $@.", v, v.getName() \ No newline at end of file +select configStruct, "TLS.Config.MinVersion is never set for variable $@.", v, v.getName() diff --git a/go/src/security/TrimMisuse/TrimMisuse.ql b/go/src/security/TrimMisuse/TrimMisuse.ql index 3a3db95..1da9cf8 100644 --- a/go/src/security/TrimMisuse/TrimMisuse.ql +++ b/go/src/security/TrimMisuse/TrimMisuse.ql @@ -16,22 +16,21 @@ import semmle.go.dataflow.DataFlow /* * Flows from a string to TrimFamilyCall cutSet argument */ + module Trim2ndArgConfig implements DataFlow::ConfigSig { - predicate isSource(DataFlow::Node source) { - source.asExpr() instanceof StringLit - } + predicate isSource(DataFlow::Node source) { source.asExpr() instanceof StringLit } predicate isSink(DataFlow::Node sink) { - exists(TrimFamilyCall trimCall | - sink.asExpr() = trimCall.getCutSetArg() - ) + exists(TrimFamilyCall trimCall | sink.asExpr() = trimCall.getCutSetArg()) } } + module Trim2ndArgFlow = DataFlow::Global; /* * Calls to Trim methods that we are interested in */ + class TrimFamilyCall extends DataFlow::CallNode { TrimFamilyCall() { this.getTarget().hasQualifiedName("strings", ["TrimRight", "TrimLeft", "Trim"]) @@ -39,37 +38,30 @@ class TrimFamilyCall extends DataFlow::CallNode { this.getTarget().hasQualifiedName("bytes", ["TrimRight", "TrimLeft", "Trim"]) } - Expr getCutSetArg() { - result = this.getArgument(1).asExpr() - } - + Expr getCutSetArg() { result = this.getArgument(1).asExpr() } } from TrimFamilyCall trimCall, StringLit cutset where // get 2nd argument value, if possible exists(DataFlow::Node source, DataFlow::Node sink | - Trim2ndArgFlow::flow(source, sink) - and source.asExpr() = cutset - and sink.asExpr() = trimCall.getCutSetArg() - ) - - and ( + Trim2ndArgFlow::flow(source, sink) and + source.asExpr() = cutset and + sink.asExpr() = trimCall.getCutSetArg() + ) and + ( // repeated characters imply the bug cutset.getValue().length() != unique(string c | c = cutset.getValue().charAt(_) | c).length() - or - ( // long strings are considered suspicious - cutset.getValue().length() > 2 - + cutset.getValue().length() > 2 and // at least one alphanumeric - and exists(cutset.getValue().regexpFind("[a-zA-Z0-9]{2}", _, _)) - + exists(cutset.getValue().regexpFind("[a-zA-Z0-9]{2}", _, _)) and // exclude probable false-positives - and not cutset.getValue().matches("%1234567%") - and not cutset.getValue().matches("%abcdefghijklmnopqrstuvwxyz%") - ) + not cutset.getValue().matches("%1234567%") and + not cutset.getValue().matches("%abcdefghijklmnopqrstuvwxyz%") ) - -select trimCall, trimCall.getTarget().getName() + " will consider the second argument ($@) as a list of elements, and not as indivisible string.", cutset, cutset.getValue() +select trimCall, + trimCall.getTarget().getName() + + " will consider the second argument ($@) as a list of elements, and not as indivisible string.", + cutset, cutset.getValue() From 952f03883c0f38e87bd95cbc8cb86ecd8c067a12 Mon Sep 17 00:00:00 2001 From: Matt Schwager Date: Thu, 18 Dec 2025 14:36:48 -0500 Subject: [PATCH 3/4] Add git blame ignore on autoformatting commit --- .git-blame-ignore-revs | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 .git-blame-ignore-revs diff --git a/.git-blame-ignore-revs b/.git-blame-ignore-revs new file mode 100644 index 0000000..9144178 --- /dev/null +++ b/.git-blame-ignore-revs @@ -0,0 +1,2 @@ +# Format CodeQL .ql and .qll files using `codeql query format` +b3b8f8f95bf1a149fdca9a8e16593caa34f4678b From 87e7631ffd0b9860bd2b81fe34b2326f5df3f19b Mon Sep 17 00:00:00 2001 From: Matt Schwager Date: Thu, 18 Dec 2025 14:44:11 -0500 Subject: [PATCH 4/4] Format Makefile --- Makefile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index 88679fe..64c360d 100644 --- a/Makefile +++ b/Makefile @@ -12,10 +12,10 @@ format-check: pack-install: find . -iname "qlpack.yml" -exec \ - sh -c 'codeql pack install $$(dirname "$$1")' sh {} \; + sh -c 'codeql pack install $$(dirname "$$1")' sh {} \; pack-upgrade: find . -iname "qlpack.yml" -exec \ - sh -c 'codeql pack upgrade $$(dirname "$$1")' sh {} \; + sh -c 'codeql pack upgrade $$(dirname "$$1")' sh {} \; .PHONY: test format format-check pack-install pack-upgrade