Skip to content

Crypto: Fix QL-for-QL alerts and refactor type standardization #19814

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions cpp/ql/lib/experimental/quantum/Language.qll
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ module ArtifactFlowConfig implements DataFlow::ConfigSig {
module ArtifactFlow = DataFlow::Global<ArtifactFlowConfig>;

/**
* Artifact output to node input configuration
* An artifact output to node input configuration
*/
abstract class AdditionalFlowInputStep extends DataFlow::Node {
abstract DataFlow::Node getOutput();
Expand Down Expand Up @@ -91,9 +91,8 @@ module GenericDataSourceFlowConfig implements DataFlow::ConfigSig {

module GenericDataSourceFlow = TaintTracking::Global<GenericDataSourceFlowConfig>;

private class ConstantDataSource extends Crypto::GenericConstantSourceInstance instanceof Literal {
ConstantDataSource() { this instanceof OpenSslGenericSourceCandidateLiteral }

private class ConstantDataSource extends Crypto::GenericConstantSourceInstance instanceof OpenSslGenericSourceCandidateLiteral
{
override DataFlow::Node getOutputNode() { result.asExpr() = this }

override predicate flowsTo(Crypto::FlowAwareElement other) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ module KnownOpenSslAlgorithmToAlgorithmValueConsumerConfig implements DataFlow::
module KnownOpenSslAlgorithmToAlgorithmValueConsumerFlow =
DataFlow::Global<KnownOpenSslAlgorithmToAlgorithmValueConsumerConfig>;

module RSAPaddingAlgorithmToPaddingAlgorithmValueConsumerConfig implements DataFlow::ConfigSig {
module RsaPaddingAlgorithmToPaddingAlgorithmValueConsumerConfig implements DataFlow::ConfigSig {
predicate isSource(DataFlow::Node source) { source.asExpr() instanceof OpenSslPaddingLiteral }

predicate isSink(DataFlow::Node sink) {
Expand All @@ -60,8 +60,8 @@ module RSAPaddingAlgorithmToPaddingAlgorithmValueConsumerConfig implements DataF
}
}

module RSAPaddingAlgorithmToPaddingAlgorithmValueConsumerFlow =
DataFlow::Global<RSAPaddingAlgorithmToPaddingAlgorithmValueConsumerConfig>;
module RsaPaddingAlgorithmToPaddingAlgorithmValueConsumerFlow =
DataFlow::Global<RsaPaddingAlgorithmToPaddingAlgorithmValueConsumerConfig>;

class OpenSslAlgorithmAdditionalFlowStep extends AdditionalFlowInputStep {
OpenSslAlgorithmAdditionalFlowStep() { exists(AlgorithmPassthroughCall c | c.getInNode() = this) }
Expand Down Expand Up @@ -114,11 +114,11 @@ class CopyAndDupAlgorithmPassthroughCall extends AlgorithmPassthroughCall {
override DataFlow::Node getOutNode() { result = outNode }
}

class NIDToPointerPassthroughCall extends AlgorithmPassthroughCall {
class NidToPointerPassthroughCall extends AlgorithmPassthroughCall {
DataFlow::Node inNode;
DataFlow::Node outNode;

NIDToPointerPassthroughCall() {
NidToPointerPassthroughCall() {
this.getTarget().getName() in ["OBJ_nid2obj", "OBJ_nid2ln", "OBJ_nid2sn"] and
inNode.asExpr() = this.getArgument(0) and
outNode.asExpr() = this
Expand Down Expand Up @@ -150,11 +150,11 @@ class PointerToPointerPassthroughCall extends AlgorithmPassthroughCall {
override DataFlow::Node getOutNode() { result = outNode }
}

class PointerToNIDPassthroughCall extends AlgorithmPassthroughCall {
class PointerToNidPassthroughCall extends AlgorithmPassthroughCall {
DataFlow::Node inNode;
DataFlow::Node outNode;

PointerToNIDPassthroughCall() {
PointerToNidPassthroughCall() {
this.getTarget().getName() in ["OBJ_obj2nid", "OBJ_ln2nid", "OBJ_sn2nid", "OBJ_txt2nid"] and
(
inNode.asIndirectExpr() = this.getArgument(0)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,36 +5,35 @@ private import experimental.quantum.OpenSSL.AlgorithmInstances.KnownAlgorithmCon
private import experimental.quantum.OpenSSL.AlgorithmValueConsumers.DirectAlgorithmValueConsumer
private import experimental.quantum.OpenSSL.AlgorithmValueConsumers.OpenSSLAlgorithmValueConsumerBase
private import AlgToAVCFlow
private import codeql.quantum.experimental.Standardization::Types::KeyOpAlg as KeyOpAlg

/**
* Given a `KnownOpenSslBlockModeAlgorithmExpr`, converts this to a block family type.
* Does not bind if there is no mapping (no mapping to 'unknown' or 'other').
*/
predicate knownOpenSslConstantToBlockModeFamilyType(
KnownOpenSslBlockModeAlgorithmExpr e, Crypto::TBlockCipherModeOfOperationType type
KnownOpenSslBlockModeAlgorithmExpr e, KeyOpAlg::ModeOfOperationType type
) {
exists(string name |
name = e.(KnownOpenSslAlgorithmExpr).getNormalizedName() and
(
name.matches("CBC") and type instanceof Crypto::CBC
name = "CBC" and type instanceof KeyOpAlg::CBC
or
name.matches("CFB%") and type instanceof Crypto::CFB
name = "CFB%" and type instanceof KeyOpAlg::CFB
or
name.matches("CTR") and type instanceof Crypto::CTR
name = "CTR" and type instanceof KeyOpAlg::CTR
or
name.matches("GCM") and type instanceof Crypto::GCM
name = "GCM" and type instanceof KeyOpAlg::GCM
or
name.matches("OFB") and type instanceof Crypto::OFB
name = "OFB" and type instanceof KeyOpAlg::OFB
or
name.matches("XTS") and type instanceof Crypto::XTS
name = "XTS" and type instanceof KeyOpAlg::XTS
or
name.matches("CCM") and type instanceof Crypto::CCM
name = "CCM" and type instanceof KeyOpAlg::CCM
or
name.matches("GCM") and type instanceof Crypto::GCM
name = "CCM" and type instanceof KeyOpAlg::CCM
or
name.matches("CCM") and type instanceof Crypto::CCM
or
name.matches("ECB") and type instanceof Crypto::ECB
name = "ECB" and type instanceof KeyOpAlg::ECB
)
)
}
Expand Down Expand Up @@ -64,10 +63,10 @@ class KnownOpenSslBlockModeConstantAlgorithmInstance extends OpenSslAlgorithmIns
getterCall = this
}

override Crypto::TBlockCipherModeOfOperationType getModeType() {
override KeyOpAlg::ModeOfOperationType getModeType() {
knownOpenSslConstantToBlockModeFamilyType(this, result)
or
not knownOpenSslConstantToBlockModeFamilyType(this, _) and result = Crypto::OtherMode()
not knownOpenSslConstantToBlockModeFamilyType(this, _) and result = KeyOpAlg::OtherMode()
}

// NOTE: I'm not going to attempt to parse out the mode specific part, so returning
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,9 @@ predicate knownOpenSslConstantToCipherFamilyType(
or
name.matches("CAST5%") and type = KeyOpAlg::TSymmetricCipher(KeyOpAlg::CAST5())
or
name.matches("2DES%") and type = KeyOpAlg::TSymmetricCipher(KeyOpAlg::DoubleDES())
name.matches("2DES%") and type = KeyOpAlg::TSymmetricCipher(KeyOpAlg::DOUBLE_DES())
or
name.matches("3DES%") and type = KeyOpAlg::TSymmetricCipher(KeyOpAlg::TripleDES())
name.matches("3DES%") and type = KeyOpAlg::TSymmetricCipher(KeyOpAlg::TRIPLE_DES())
or
name.matches("DES%") and type = KeyOpAlg::TSymmetricCipher(KeyOpAlg::DES())
or
Expand Down Expand Up @@ -113,7 +113,7 @@ class KnownOpenSslCipherConstantAlgorithmInstance extends OpenSslAlgorithmInstan
this.(KnownOpenSslCipherAlgorithmExpr).getExplicitKeySize() = result
}

override Crypto::KeyOpAlg::Algorithm getAlgorithmType() {
override KeyOpAlg::AlgorithmType getAlgorithmType() {
knownOpenSslConstantToCipherFamilyType(this, result)
or
not knownOpenSslConstantToCipherFamilyType(this, _) and
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,16 +39,22 @@ class KnownOpenSslEllipticCurveConstantAlgorithmInstance extends OpenSslAlgorith
result = this.(Call).getTarget().getName()
}

override Crypto::TEllipticCurveType getEllipticCurveType() {
Crypto::ellipticCurveNameToKeySizeAndFamilyMapping(this.getParsedEllipticCurveName(), _, result)
override Crypto::EllipticCurveFamilyType getEllipticCurveFamilyType() {
if
Crypto::ellipticCurveNameToKnownKeySizeAndFamilyMapping(this.getParsedEllipticCurveName(), _,
_)
then
Crypto::ellipticCurveNameToKnownKeySizeAndFamilyMapping(this.getParsedEllipticCurveName(), _,
result)
else result = Crypto::OtherEllipticCurveType()
}

override string getParsedEllipticCurveName() {
result = this.(KnownOpenSslAlgorithmExpr).getNormalizedName()
}

override int getKeySize() {
Crypto::ellipticCurveNameToKeySizeAndFamilyMapping(this.(KnownOpenSslAlgorithmExpr)
Crypto::ellipticCurveNameToKnownKeySizeAndFamilyMapping(this.(KnownOpenSslAlgorithmExpr)
.getNormalizedName(), result, _)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,35 +11,35 @@ predicate knownOpenSslConstantToHashFamilyType(
exists(string name |
name = e.(KnownOpenSslAlgorithmExpr).getNormalizedName() and
(
name.matches("BLAKE2B") and type instanceof Crypto::BLAKE2B
name = "BLAKE2B" and type instanceof Crypto::BLAKE2B
or
name.matches("BLAKE2S") and type instanceof Crypto::BLAKE2S
name = "BLAKE2S" and type instanceof Crypto::BLAKE2S
or
name.matches("GOST%") and type instanceof Crypto::GOSTHash
name.matches("GOST%") and type instanceof Crypto::GOST_HASH
or
name.matches("MD2") and type instanceof Crypto::MD2
name = "MD2" and type instanceof Crypto::MD2
or
name.matches("MD4") and type instanceof Crypto::MD4
name = "MD4" and type instanceof Crypto::MD4
or
name.matches("MD5") and type instanceof Crypto::MD5
name = "MD5" and type instanceof Crypto::MD5
or
name.matches("MDC2") and type instanceof Crypto::MDC2
name = "MDC2" and type instanceof Crypto::MDC2
or
name.matches("POLY1305") and type instanceof Crypto::POLY1305
name = "POLY1305" and type instanceof Crypto::POLY1305
or
name.matches(["SHA", "SHA1"]) and type instanceof Crypto::SHA1
or
name.matches("SHA_%") and not name.matches(["SHA1", "SHA3-"]) and type instanceof Crypto::SHA2
or
name.matches("SHA3-%") and type instanceof Crypto::SHA3
or
name.matches(["SHAKE"]) and type instanceof Crypto::SHAKE
name = "SHAKE" and type instanceof Crypto::SHAKE
or
name.matches("SM3") and type instanceof Crypto::SM3
name = "SM3" and type instanceof Crypto::SM3
or
name.matches("RIPEMD160") and type instanceof Crypto::RIPEMD160
name = "RIPEMD160" and type instanceof Crypto::RIPEMD160
or
name.matches("WHIRLPOOL") and type instanceof Crypto::WHIRLPOOL
name = "WHIRLPOOL" and type instanceof Crypto::WHIRLPOOL
)
)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,8 @@ string getAlgorithmAlias(string alias) {
}

/**
* Finds aliases of known alagorithms defined by users (through obj_name_add and various macros pointing to this function)
* Holds for aliases of known algorithms defined by users
* (through obj_name_add and various macros pointing to this function).
*
* The `target` and `alias` are converted to lowercase to be of a standard form.
*/
Expand All @@ -222,7 +223,7 @@ predicate customAliases(string target, string alias) {
}

/**
* A hard-coded mapping of known algorithm aliases in OpenSsl.
* Holds for a hard-coded mapping of known algorithm aliases in OpenSsl.
* This was derived by applying the same kind of logic foun din `customAliases` to the
* OpenSsl code base directly.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ private import experimental.quantum.OpenSSL.Operations.OpenSSLOperations
private import AlgToAVCFlow

class KnownOpenSslMacConstantAlgorithmInstance extends OpenSslAlgorithmInstance,
Crypto::MACAlgorithmInstance instanceof KnownOpenSslMacAlgorithmExpr
Crypto::MacAlgorithmInstance instanceof KnownOpenSslMacAlgorithmExpr
{
OpenSslAlgorithmValueConsumer getterCall;

Expand Down Expand Up @@ -39,14 +39,14 @@ class KnownOpenSslMacConstantAlgorithmInstance extends OpenSslAlgorithmInstance,
result = this.(Call).getTarget().getName()
}

override Crypto::TMACType getMacType() {
this instanceof KnownOpenSslHMacAlgorithmExpr and result instanceof Crypto::THMAC
override Crypto::MacType getMacType() {
this instanceof KnownOpenSslHMacAlgorithmExpr and result = Crypto::HMAC()
or
this instanceof KnownOpenSslCMacAlgorithmExpr and result instanceof Crypto::TCMAC
this instanceof KnownOpenSslCMacAlgorithmExpr and result = Crypto::CMAC()
}
}

class KnownOpenSslHMacConstantAlgorithmInstance extends Crypto::HMACAlgorithmInstance,
class KnownOpenSslHMacConstantAlgorithmInstance extends Crypto::HmacAlgorithmInstance,
KnownOpenSslMacConstantAlgorithmInstance
{
override Crypto::AlgorithmValueConsumer getHashAlgorithmValueConsumer() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ private import experimental.quantum.OpenSSL.AlgorithmInstances.KnownAlgorithmCon
private import AlgToAVCFlow
private import experimental.quantum.OpenSSL.AlgorithmValueConsumers.DirectAlgorithmValueConsumer
private import experimental.quantum.OpenSSL.AlgorithmValueConsumers.OpenSSLAlgorithmValueConsumerBase
private import codeql.quantum.experimental.Standardization::Types::KeyOpAlg as KeyOpAlg

/**
* A class to define padding specific integer values.
Expand All @@ -28,18 +29,18 @@ class OpenSslPaddingLiteral extends Literal {
* Does not bind if there is no mapping (no mapping to 'unknown' or 'other').
*/
predicate knownOpenSslConstantToPaddingFamilyType(
KnownOpenSslPaddingAlgorithmExpr e, Crypto::TPaddingType type
KnownOpenSslPaddingAlgorithmExpr e, KeyOpAlg::PaddingSchemeType type
) {
exists(string name |
name = e.(KnownOpenSslAlgorithmExpr).getNormalizedName() and
(
name.matches("OAEP") and type = Crypto::OAEP()
name = "OAEP" and type = KeyOpAlg::OAEP()
or
name.matches("PSS") and type = Crypto::PSS()
name = "PSS" and type = KeyOpAlg::PSS()
or
name.matches("PKCS7") and type = Crypto::PKCS7()
name = "PKCS7" and type = KeyOpAlg::PKCS7()
or
name.matches("PKCS1V15") and type = Crypto::PKCS1_v1_5()
name = "PKCS1V15" and type = KeyOpAlg::PKCS1_V1_5()
)
)
}
Expand Down Expand Up @@ -85,7 +86,7 @@ class KnownOpenSslPaddingConstantAlgorithmInstance extends OpenSslAlgorithmInsta
// Source is `this`
src.asExpr() = this and
// This traces to a padding-specific consumer
RSAPaddingAlgorithmToPaddingAlgorithmValueConsumerFlow::flow(src, sink)
RsaPaddingAlgorithmToPaddingAlgorithmValueConsumerFlow::flow(src, sink)
) and
isPaddingSpecificConsumer = true
}
Expand All @@ -98,24 +99,24 @@ class KnownOpenSslPaddingConstantAlgorithmInstance extends OpenSslAlgorithmInsta

override OpenSslAlgorithmValueConsumer getAvc() { result = getterCall }

Crypto::TPaddingType getKnownPaddingType() {
this.(Literal).getValue().toInt() in [1, 7, 8] and result = Crypto::PKCS1_v1_5()
KeyOpAlg::PaddingSchemeType getKnownPaddingType() {
this.(Literal).getValue().toInt() in [1, 7, 8] and result = KeyOpAlg::PKCS1_V1_5()
or
this.(Literal).getValue().toInt() = 3 and result = Crypto::NoPadding()
this.(Literal).getValue().toInt() = 3 and result = KeyOpAlg::NoPadding()
or
this.(Literal).getValue().toInt() = 4 and result = Crypto::OAEP()
this.(Literal).getValue().toInt() = 4 and result = KeyOpAlg::OAEP()
or
this.(Literal).getValue().toInt() = 5 and result = Crypto::ANSI_X9_23()
this.(Literal).getValue().toInt() = 5 and result = KeyOpAlg::ANSI_X9_23()
or
this.(Literal).getValue().toInt() = 6 and result = Crypto::PSS()
this.(Literal).getValue().toInt() = 6 and result = KeyOpAlg::PSS()
}

override Crypto::TPaddingType getPaddingType() {
override KeyOpAlg::PaddingSchemeType getPaddingType() {
isPaddingSpecificConsumer = true and
(
result = this.getKnownPaddingType()
or
not exists(this.getKnownPaddingType()) and result = Crypto::OtherPadding()
not exists(this.getKnownPaddingType()) and result = KeyOpAlg::OtherPadding()
)
or
isPaddingSpecificConsumer = false and
Expand Down Expand Up @@ -143,7 +144,7 @@ class KnownOpenSslPaddingConstantAlgorithmInstance extends OpenSslAlgorithmInsta
// this instanceof Literal and
// this.getValue().toInt() in [0, 1, 3, 4, 5, 6, 7, 8]
// // TODO: trace to padding-specific consumers
// RSAPaddingAlgorithmToPaddingAlgorithmValueConsumerFlow
// RsaPaddingAlgorithmToPaddingAlgorithmValueConsumerFlow
// }
// override string getRawPaddingAlgorithmName() { result = this.(Literal).getValue().toString() }
// override Crypto::TPaddingType getPaddingType() {
Expand All @@ -161,18 +162,18 @@ class KnownOpenSslPaddingConstantAlgorithmInstance extends OpenSslAlgorithmInsta
// else result = Crypto::OtherPadding()
// }
// }
class OAEPPaddingAlgorithmInstance extends Crypto::OAEPPaddingAlgorithmInstance,
class OaepPaddingAlgorithmInstance extends Crypto::OaepPaddingAlgorithmInstance,
KnownOpenSslPaddingConstantAlgorithmInstance
{
OAEPPaddingAlgorithmInstance() {
this.(Crypto::PaddingAlgorithmInstance).getPaddingType() = Crypto::OAEP()
OaepPaddingAlgorithmInstance() {
this.(Crypto::PaddingAlgorithmInstance).getPaddingType() = KeyOpAlg::OAEP()
}

override Crypto::HashAlgorithmInstance getOAEPEncodingHashAlgorithm() {
override Crypto::HashAlgorithmInstance getOaepEncodingHashAlgorithm() {
none() //TODO
}

override Crypto::HashAlgorithmInstance getMGF1HashAlgorithm() {
override Crypto::HashAlgorithmInstance getMgf1HashAlgorithm() {
none() //TODO
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ class KnownOpenSslSignatureConstantAlgorithmInstance extends OpenSslAlgorithmIns
none()
}

override KeyOpAlg::Algorithm getAlgorithmType() {
override KeyOpAlg::AlgorithmType getAlgorithmType() {
knownOpenSslConstantToSignatureFamilyType(this, result)
or
not knownOpenSslConstantToSignatureFamilyType(this, _) and
Expand Down
Loading
Loading