From a63ebf178d87bf330a37ee1dd769fecc6b8ea022 Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Wed, 23 Oct 2024 21:58:25 +0100 Subject: [PATCH 01/12] Make StructType.hasEmbeddedField public --- go/ql/lib/semmle/go/Types.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/ql/lib/semmle/go/Types.qll b/go/ql/lib/semmle/go/Types.qll index 1b09ea466cc4..8abd94776cc3 100644 --- a/go/ql/lib/semmle/go/Types.qll +++ b/go/ql/lib/semmle/go/Types.qll @@ -483,7 +483,7 @@ class StructType extends @structtype, CompositeType { /** * Holds if there is an embedded field at `depth`, with either type `tp` or a pointer to `tp`. */ - private predicate hasEmbeddedField(Type tp, int depth) { + predicate hasEmbeddedField(Type tp, int depth) { exists(Field f | this.hasFieldCand(_, f, depth, true) | tp = f.getType() or tp = f.getType().(PointerType).getBaseType() From 32b1a1a7ca7ff154504ddac2ed11856543185201 Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Wed, 23 Oct 2024 21:58:52 +0100 Subject: [PATCH 02/12] Simplify ensureCorrectTypeInfo --- .../go/dataflow/internal/FlowSummaryImpl.qll | 38 +++---------------- 1 file changed, 5 insertions(+), 33 deletions(-) diff --git a/go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll b/go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll index 14a92748d526..a114a2447229 100644 --- a/go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll +++ b/go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll @@ -276,42 +276,23 @@ module SourceSinkInterpretationInput implements or exists(DataFlow::Write fw | fw.writesField(recv, sse.asEntity(), _)) ) and - exists(string pkg, string typename, boolean subtypes, Type syntacticRecvBaseType | + exists(string pkg, string typename, boolean subtypes, Type syntacticRecvBaseType, Type targetType | sse.hasTypeInfo(pkg, typename, subtypes) and + targetType.hasQualifiedName(pkg, typename) and syntacticRecvBaseType = lookThroughPointerType(getSyntacticRecv(recv).getType()) | subtypes = [true, false] and - syntacticRecvBaseType.hasQualifiedName(pkg, typename) + syntacticRecvBaseType = targetType or subtypes = true and ( // `syntacticRecvBaseType`'s underlying type might be an interface type and `sse` // might be a method defined on an interface which is a subtype of it. - exists(Type t | - t = syntacticRecvBaseType.getUnderlyingType().(InterfaceType).getAnEmbeddedInterface() and - t.hasQualifiedName(pkg, typename) and - sse.asEntity().(Method).hasQualifiedName(pkg, typename, _) - ) + targetType = syntacticRecvBaseType.getUnderlyingType().(InterfaceType).getAnEmbeddedInterface() or // `syntacticRecvBaseType`'s underlying type might be a struct type and `sse` // might be a promoted method or field. - exists(StructType st, Field field1, Field field2, int depth1, int depth2, Type t1, Type t2 | - st = syntacticRecvBaseType.getUnderlyingType() and - field1 = st.getFieldAtDepth(_, depth1) and - field2 = st.getFieldAtDepth(_, depth2) and - t1 = lookThroughPointerType(field1.getType()) and - t2 = lookThroughPointerType(field2.getType()) and - ( - field1 = field2 - or - field2 = t1.getUnderlyingType().(StructType).getFieldAtDepth(_, depth2 - depth1 - 1) - ) and - matchTypeInfo(sse, t1) - | - sse.asEntity().(Method).getReceiverBaseType() = t2 - or - sse.asEntity().(Field).getDeclaringType() = t2.getUnderlyingType() - ) + syntacticRecvBaseType.getUnderlyingType().(StructType).hasEmbeddedField(targetType, _) ) ) } @@ -343,15 +324,6 @@ module SourceSinkInterpretationInput implements .getBaseInstruction() } - bindingset[sse, t] - pragma[inline_late] - private predicate matchTypeInfo(SourceOrSinkElement sse, Type t) { - exists(string pkg, string typename | - sse.hasTypeInfo(pkg, typename, true) and - t.hasQualifiedName(pkg, typename) - ) - } - /** Provides additional sink specification logic. */ bindingset[c] predicate interpretOutput(string c, InterpretNode mid, InterpretNode node) { From a2beb3a4be18783c2ed4422981e3e531278414e9 Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Wed, 23 Oct 2024 22:04:32 +0100 Subject: [PATCH 03/12] Simplify interpretElement --- go/ql/lib/semmle/go/dataflow/ExternalFlow.qll | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/go/ql/lib/semmle/go/dataflow/ExternalFlow.qll b/go/ql/lib/semmle/go/dataflow/ExternalFlow.qll index 68a876deaea6..1a59b272fb81 100644 --- a/go/ql/lib/semmle/go/dataflow/ExternalFlow.qll +++ b/go/ql/lib/semmle/go/dataflow/ExternalFlow.qll @@ -475,10 +475,8 @@ SourceSinkInterpretationInput::SourceOrSinkElement interpretElement( // Go does not need to distinguish functions with signature signature = "" and exists(string p | p = interpretPackage(pkg) | - exists(Field f | f.hasQualifiedName(p, type, name) | - result.asEntity() = f and - result.hasTypeInfo(p, type, subtypes) - ) + result.asEntity().(Field).hasQualifiedName(p, type, name) and + result.hasTypeInfo(p, type, subtypes) or exists(Method m | m.hasQualifiedName(p, type, name) | result.asEntity() = m and From 652a6242b4722125714977a90b8bbd532c16e941 Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Wed, 23 Oct 2024 22:08:26 +0100 Subject: [PATCH 04/12] Further simplify interpretElement to avoid 'm' only being used on one side of a disjunction --- go/ql/lib/semmle/go/dataflow/ExternalFlow.qll | 30 +++++++++---------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/go/ql/lib/semmle/go/dataflow/ExternalFlow.qll b/go/ql/lib/semmle/go/dataflow/ExternalFlow.qll index 1a59b272fb81..c557b5af7de3 100644 --- a/go/ql/lib/semmle/go/dataflow/ExternalFlow.qll +++ b/go/ql/lib/semmle/go/dataflow/ExternalFlow.qll @@ -475,23 +475,21 @@ SourceSinkInterpretationInput::SourceOrSinkElement interpretElement( // Go does not need to distinguish functions with signature signature = "" and exists(string p | p = interpretPackage(pkg) | - result.asEntity().(Field).hasQualifiedName(p, type, name) and - result.hasTypeInfo(p, type, subtypes) + result.hasTypeInfo(p, type, subtypes) and + ( + result.asEntity().(Field).hasQualifiedName(p, type, name) or + result.asEntity().(Method).hasQualifiedName(p, type, name) + ) or - exists(Method m | m.hasQualifiedName(p, type, name) | - result.asEntity() = m and - result.hasTypeInfo(p, type, subtypes) - or - subtypes = true and - // p.type is an interface and we include types which implement it - exists(Method m2, string pkg2, string type2 | - m2.getReceiverType().implements(p, type) and - m2.getName() = name and - m2.getReceiverBaseType().hasQualifiedName(pkg2, type2) - | - result.asEntity() = m2 and - result.hasTypeInfo(pkg2, type2, subtypes) - ) + subtypes = true and + // p.type is an interface and we include types which implement it + exists(Method m2, string pkg2, string type2 | + m2.getReceiverType().implements(p, type) and + m2.getName() = name and + m2.getReceiverBaseType().hasQualifiedName(pkg2, type2) + | + result.asEntity() = m2 and + result.hasTypeInfo(pkg2, type2, subtypes) ) or type = "" and From 7572edc5bed9fabd6e2381d0af57e47848274595 Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Wed, 23 Oct 2024 22:13:14 +0100 Subject: [PATCH 05/12] Add comment noting that a Method or Field might have multiple SourceOrSinkElements --- go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll | 3 +++ 1 file changed, 3 insertions(+) diff --git a/go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll b/go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll index a114a2447229..5caa2570394b 100644 --- a/go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll +++ b/go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll @@ -149,6 +149,9 @@ module SourceSinkInterpretationInput implements ) } + // Note that due to embedding, which is currently implemented via some Methods + // or Fields having multiple qualified names, a given Method or Field is liable + // to have more than one SourceOrSinkElement, one for each of the names it claims. private newtype TSourceOrSinkElement = TMethodEntityElement(Method m, string pkg, string type, boolean subtypes) { m.hasQualifiedName(pkg, type, _) and subtypes = [true, false] From 2a78ddb9c9ef40a8c8d9162bde207144f9705c50 Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Wed, 23 Oct 2024 22:13:58 +0100 Subject: [PATCH 06/12] Correct hasTypeInfo doc comment --- go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll b/go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll index 5caa2570394b..90b2771b2060 100644 --- a/go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll +++ b/go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll @@ -178,7 +178,7 @@ module SourceSinkInterpretationInput implements AstNode asAstNode() { this = TAstElement(result) } /** - * Holds if this source or sink element is a method that was specified + * Holds if this source or sink element is a method or field that was specified * with the given values for `pkg`, `type` and `subtypes`. */ predicate hasTypeInfo(string pkg, string type, boolean subtypes) { From 5e2b5405f3211a1a3ec38bc70c95ebf9a6433f48 Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Wed, 23 Oct 2024 22:19:18 +0100 Subject: [PATCH 07/12] Rename ensureCorrectTypeInfo and getSyntacticRecv --- .../go/dataflow/internal/FlowSummaryImpl.qll | 33 ++++++++++--------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll b/go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll index 90b2771b2060..18e7c3613b56 100644 --- a/go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll +++ b/go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll @@ -244,7 +244,7 @@ module SourceSinkInterpretationInput implements ( not callTarget instanceof Method or - ensureCorrectTypeInfo(result, cn.getReceiver()) + elementAppliesToQualifier(result, cn.getReceiver()) ) ) } @@ -271,36 +271,39 @@ module SourceSinkInterpretationInput implements } } - private predicate ensureCorrectTypeInfo(SourceOrSinkElement sse, DataFlow::Node recv) { + private predicate elementAppliesToQualifier(SourceOrSinkElement sse, DataFlow::Node qual) { ( - exists(DataFlow::CallNode cn | cn.getReceiver() = recv and cn.getTarget() = sse.asEntity()) + exists(DataFlow::CallNode cn | cn.getReceiver() = qual and cn.getTarget() = sse.asEntity()) or - exists(DataFlow::FieldReadNode frn | frn.getBase() = recv and frn.getField() = sse.asEntity()) + exists(DataFlow::FieldReadNode frn | frn.getBase() = qual and frn.getField() = sse.asEntity()) or - exists(DataFlow::Write fw | fw.writesField(recv, sse.asEntity(), _)) + exists(DataFlow::Write fw | fw.writesField(qual, sse.asEntity(), _)) ) and - exists(string pkg, string typename, boolean subtypes, Type syntacticRecvBaseType, Type targetType | + exists( + string pkg, string typename, boolean subtypes, Type syntacticQualBaseType, Type targetType + | sse.hasTypeInfo(pkg, typename, subtypes) and targetType.hasQualifiedName(pkg, typename) and - syntacticRecvBaseType = lookThroughPointerType(getSyntacticRecv(recv).getType()) + syntacticQualBaseType = lookThroughPointerType(getSyntacticQualifier(qual).getType()) | subtypes = [true, false] and - syntacticRecvBaseType = targetType + syntacticQualBaseType = targetType or subtypes = true and ( - // `syntacticRecvBaseType`'s underlying type might be an interface type and `sse` + // `syntacticQualBaseType`'s underlying type might be an interface type and `sse` // might be a method defined on an interface which is a subtype of it. - targetType = syntacticRecvBaseType.getUnderlyingType().(InterfaceType).getAnEmbeddedInterface() + targetType = + syntacticQualBaseType.getUnderlyingType().(InterfaceType).getAnEmbeddedInterface() or - // `syntacticRecvBaseType`'s underlying type might be a struct type and `sse` + // `syntacticQualBaseType`'s underlying type might be a struct type and `sse` // might be a promoted method or field. - syntacticRecvBaseType.getUnderlyingType().(StructType).hasEmbeddedField(targetType, _) + syntacticQualBaseType.getUnderlyingType().(StructType).hasEmbeddedField(targetType, _) ) ) } - private DataFlow::Node getSyntacticRecv(DataFlow::Node n) { + private DataFlow::Node getSyntacticQualifier(DataFlow::Node n) { exists(DataFlow::Node n2 | // look through implicit dereference, if there is one not exists(n.asInstruction().(IR::EvalImplicitDerefInstruction).getOperand()) and @@ -346,7 +349,7 @@ module SourceSinkInterpretationInput implements exists(DataFlow::FieldReadNode frn | frn = n | c = "" and frn.getField() = e.asEntity() and - ensureCorrectTypeInfo(e, frn.getBase()) + elementAppliesToQualifier(e, frn.getBase()) ) ) } @@ -367,7 +370,7 @@ module SourceSinkInterpretationInput implements | c = "" and fw.writesField(base, f, node.asNode()) and - ensureCorrectTypeInfo(e, base) + elementAppliesToQualifier(e, base) ) } } From 3e47280f2dea0b1b6222fa8eea304c4fcda5d0f2 Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Wed, 23 Oct 2024 22:21:39 +0100 Subject: [PATCH 08/12] Rename getImplicitFieldReadInstruction --- go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll b/go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll index 18e7c3613b56..ca16d9abd01c 100644 --- a/go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll +++ b/go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll @@ -316,13 +316,13 @@ module SourceSinkInterpretationInput implements } private DataFlow::Node skipImplicitFieldReads(DataFlow::Node n) { - not exists(getImplicitFieldReadInstruction(n)) and result = n + not exists(lookThroughImplicitFieldRead(n)) and result = n or - result = skipImplicitFieldReads(getImplicitFieldReadInstruction(n)) + result = skipImplicitFieldReads(lookThroughImplicitFieldRead(n)) } pragma[inline] - private DataFlow::Node getImplicitFieldReadInstruction(DataFlow::Node n) { + private DataFlow::Node lookThroughImplicitFieldRead(DataFlow::Node n) { result.asInstruction() = n.(DataFlow::InstructionNode) .asInstruction() From cfd2f7be807f3273f7ee4b19c2b6939d9a7e6232 Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Wed, 23 Oct 2024 22:22:29 +0100 Subject: [PATCH 09/12] Remove probably-unnecessary pragma --- go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll | 1 - 1 file changed, 1 deletion(-) diff --git a/go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll b/go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll index ca16d9abd01c..4c9680ef3507 100644 --- a/go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll +++ b/go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll @@ -321,7 +321,6 @@ module SourceSinkInterpretationInput implements result = skipImplicitFieldReads(lookThroughImplicitFieldRead(n)) } - pragma[inline] private DataFlow::Node lookThroughImplicitFieldRead(DataFlow::Node n) { result.asInstruction() = n.(DataFlow::InstructionNode) From 1311ca1d374146569e2784a9025d6801fe41186a Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Wed, 23 Oct 2024 22:30:05 +0100 Subject: [PATCH 10/12] Explain getSyntacticQual --- go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll b/go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll index 4c9680ef3507..77d7fdc10dc0 100644 --- a/go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll +++ b/go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll @@ -303,6 +303,13 @@ module SourceSinkInterpretationInput implements ) } + /** + * Gets `underlying`, where `n` if of the form `implicitDeref?(underlying.implicitFieldRead1.implicitFieldRead2...)` + * + * For Go syntax like `qualifier.method()` or `qualifier.field`, this is the type of `qualifier`, before any + * implicit dereference is interposed because `qualifier` is of pointer type, or implicit field accesses + * navigate to any embedded struct types that truly host `field`. + */ private DataFlow::Node getSyntacticQualifier(DataFlow::Node n) { exists(DataFlow::Node n2 | // look through implicit dereference, if there is one From da13e6b00a6433bc8d3b339c4afc3cae6318d778 Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Wed, 23 Oct 2024 22:34:41 +0100 Subject: [PATCH 11/12] Improve comments --- go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll b/go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll index 77d7fdc10dc0..35ffeb26bed0 100644 --- a/go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll +++ b/go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll @@ -292,12 +292,12 @@ module SourceSinkInterpretationInput implements subtypes = true and ( // `syntacticQualBaseType`'s underlying type might be an interface type and `sse` - // might be a method defined on an interface which is a subtype of it. + // might refer to a method defined on an interface embedded within it. targetType = syntacticQualBaseType.getUnderlyingType().(InterfaceType).getAnEmbeddedInterface() or // `syntacticQualBaseType`'s underlying type might be a struct type and `sse` - // might be a promoted method or field. + // might refer to an embedded method or field. syntacticQualBaseType.getUnderlyingType().(StructType).hasEmbeddedField(targetType, _) ) ) From c944f1d0bd6bf39d32df2c576bc71701fae3f47d Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Wed, 23 Oct 2024 22:53:02 +0100 Subject: [PATCH 12/12] Document elementAppliesToQualifier --- go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll b/go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll index 35ffeb26bed0..e54fb5665042 100644 --- a/go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll +++ b/go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll @@ -271,6 +271,14 @@ module SourceSinkInterpretationInput implements } } + /** + * Holds if method or field spec `sse` applies in the context of qualifier `qual`. + * + * Note that naively checking `sse.asEntity()`'s qualified name is not correct, because + * `Method`s and `Field`s may have multiple qualified names due to embedding. We must instead + * check that the specific name given be `sse.hasTypeInfo` refers to either `qual`'s type + * or to a type it embeds. + */ private predicate elementAppliesToQualifier(SourceOrSinkElement sse, DataFlow::Node qual) { ( exists(DataFlow::CallNode cn | cn.getReceiver() = qual and cn.getTarget() = sse.asEntity())