From c747f24b39e6a95db82db2fbf390efe9079fbd6a Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Mon, 1 Oct 2018 17:33:18 +0100 Subject: [PATCH 1/6] CPP: Fix the initialized array case in getBufferSize. --- cpp/ql/src/semmle/code/cpp/commons/Buffer.qll | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/cpp/ql/src/semmle/code/cpp/commons/Buffer.qll b/cpp/ql/src/semmle/code/cpp/commons/Buffer.qll index b184832e1661..0ceee2b7d08d 100644 --- a/cpp/ql/src/semmle/code/cpp/commons/Buffer.qll +++ b/cpp/ql/src/semmle/code/cpp/commons/Buffer.qll @@ -58,6 +58,10 @@ int getBufferSize(Expr bufferExpr, Element why) { // buffer is an initialized array // e.g. int buffer[] = {1, 2, 3}; why = bufferVar.getInitializer().getExpr() and + ( + why instanceof AggregateLiteral or + why instanceof StringLiteral + ) and result = why.(Expr).getType().(ArrayType).getSize() and not exists(bufferVar.getType().getUnspecifiedType().(ArrayType).getSize()) ) or exists(Class parentClass, VariableAccess parentPtr | From ef8ca5de587e79e6435f68da10088871998de0c2 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Tue, 2 Oct 2018 09:15:35 +0100 Subject: [PATCH 2/6] CPP: Replace def-use with dataflow in getBufferSize. --- cpp/ql/src/semmle/code/cpp/commons/Buffer.qll | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/cpp/ql/src/semmle/code/cpp/commons/Buffer.qll b/cpp/ql/src/semmle/code/cpp/commons/Buffer.qll index 0ceee2b7d08d..1584ec8a56e4 100644 --- a/cpp/ql/src/semmle/code/cpp/commons/Buffer.qll +++ b/cpp/ql/src/semmle/code/cpp/commons/Buffer.qll @@ -1,4 +1,5 @@ import cpp +import semmle.code.cpp.dataflow.DataFlow /** * Holds if `sizeof(s)` occurs as part of the parameter of a dynamic @@ -75,16 +76,13 @@ int getBufferSize(Expr bufferExpr, Element why) { bufferVar.getType().getSize() - parentClass.getSize() ) - ) or exists(Expr def | + ) or ( // buffer is assigned with an allocation - definitionUsePair(_, def, bufferExpr) and - exprDefinition(_, def, why) and + DataFlow::localFlowStep(DataFlow::exprNode(why), DataFlow::exprNode(bufferExpr)) and isFixedSizeAllocationExpr(why, result) - ) or exists(Expr def, Expr e, Element why2 | - // buffer is assigned with another buffer - definitionUsePair(_, def, bufferExpr) and - exprDefinition(_, def, e) and - result = getBufferSize(e, why2) and + ) or exists(Expr def, Element why2 | + DataFlow::localFlowStep(DataFlow::exprNode(def), DataFlow::exprNode(bufferExpr)) and + result = getBufferSize(def, why2) and ( why = def or why = why2 From beb21f92d3b0c1506800e36522753243e0d8af81 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Tue, 2 Oct 2018 10:20:46 +0100 Subject: [PATCH 3/6] CPP: Separate the dataflow case from dynamic allocation. --- cpp/ql/src/semmle/code/cpp/commons/Buffer.qll | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/cpp/ql/src/semmle/code/cpp/commons/Buffer.qll b/cpp/ql/src/semmle/code/cpp/commons/Buffer.qll index 1584ec8a56e4..65aa7ebee20d 100644 --- a/cpp/ql/src/semmle/code/cpp/commons/Buffer.qll +++ b/cpp/ql/src/semmle/code/cpp/commons/Buffer.qll @@ -77,10 +77,11 @@ int getBufferSize(Expr bufferExpr, Element why) { parentClass.getSize() ) ) or ( - // buffer is assigned with an allocation - DataFlow::localFlowStep(DataFlow::exprNode(why), DataFlow::exprNode(bufferExpr)) and - isFixedSizeAllocationExpr(why, result) + // buffer is a fixed size dynamic allocation + isFixedSizeAllocationExpr(bufferExpr, result) and + why = bufferExpr ) or exists(Expr def, Element why2 | + // dataflow DataFlow::localFlowStep(DataFlow::exprNode(def), DataFlow::exprNode(bufferExpr)) and result = getBufferSize(def, why2) and ( From fe6c9f9ea2b0bb3ece895496b6f6e9b591e2c774 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Tue, 2 Oct 2018 10:27:40 +0100 Subject: [PATCH 4/6] CPP: Stricter dataflow in getBufferSize. --- cpp/ql/src/semmle/code/cpp/commons/Buffer.qll | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/cpp/ql/src/semmle/code/cpp/commons/Buffer.qll b/cpp/ql/src/semmle/code/cpp/commons/Buffer.qll index 65aa7ebee20d..c27183360f75 100644 --- a/cpp/ql/src/semmle/code/cpp/commons/Buffer.qll +++ b/cpp/ql/src/semmle/code/cpp/commons/Buffer.qll @@ -80,13 +80,15 @@ int getBufferSize(Expr bufferExpr, Element why) { // buffer is a fixed size dynamic allocation isFixedSizeAllocationExpr(bufferExpr, result) and why = bufferExpr - ) or exists(Expr def, Element why2 | + ) or forex(Expr def | // dataflow - DataFlow::localFlowStep(DataFlow::exprNode(def), DataFlow::exprNode(bufferExpr)) and - result = getBufferSize(def, why2) and - ( - why = def or - why = why2 + DataFlow::localFlowStep(DataFlow::exprNode(def), DataFlow::exprNode(bufferExpr)) | + exists(Element why2 | + result = getBufferSize(def, why2) and + ( + why = def or + why = why2 + ) ) ) or exists(Type bufferType | // buffer is the address of a variable From 8ab830f21c371147fdb49bba876b7b21c6106b55 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Tue, 2 Oct 2018 11:00:26 +0100 Subject: [PATCH 5/6] CPP: Allow multiple dataflow sources. --- cpp/ql/src/semmle/code/cpp/commons/Buffer.qll | 21 +++++++++++-------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/cpp/ql/src/semmle/code/cpp/commons/Buffer.qll b/cpp/ql/src/semmle/code/cpp/commons/Buffer.qll index c27183360f75..6ef6c5f28328 100644 --- a/cpp/ql/src/semmle/code/cpp/commons/Buffer.qll +++ b/cpp/ql/src/semmle/code/cpp/commons/Buffer.qll @@ -80,15 +80,18 @@ int getBufferSize(Expr bufferExpr, Element why) { // buffer is a fixed size dynamic allocation isFixedSizeAllocationExpr(bufferExpr, result) and why = bufferExpr - ) or forex(Expr def | - // dataflow - DataFlow::localFlowStep(DataFlow::exprNode(def), DataFlow::exprNode(bufferExpr)) | - exists(Element why2 | - result = getBufferSize(def, why2) and - ( - why = def or - why = why2 - ) + ) or ( + // dataflow (all sources must be the same size) + forex(Expr def | + DataFlow::localFlowStep(DataFlow::exprNode(def), DataFlow::exprNode(bufferExpr)) | + result = getBufferSize(def, _) + ) and + + // find reason + exists(Expr def | + DataFlow::localFlowStep(DataFlow::exprNode(def), DataFlow::exprNode(bufferExpr)) | + why = def or + result = getBufferSize(def, why) ) ) or exists(Type bufferType | // buffer is the address of a variable From 8163def3ae29889a76cfff3e3fb53a74f361ad7f Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Fri, 5 Oct 2018 18:37:05 +0100 Subject: [PATCH 6/6] CPP: Alter the dataflow case. --- cpp/ql/src/semmle/code/cpp/commons/Buffer.qll | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/cpp/ql/src/semmle/code/cpp/commons/Buffer.qll b/cpp/ql/src/semmle/code/cpp/commons/Buffer.qll index 6ef6c5f28328..a3de2f3eb597 100644 --- a/cpp/ql/src/semmle/code/cpp/commons/Buffer.qll +++ b/cpp/ql/src/semmle/code/cpp/commons/Buffer.qll @@ -48,6 +48,7 @@ predicate memberMayBeVarSize(Class c, MemberVariable v) { /** * Get the size in bytes of the buffer pointed to by an expression (if this can be determined). */ +language[monotonicAggregates] int getBufferSize(Expr bufferExpr, Element why) { exists(Variable bufferVar | bufferVar = bufferExpr.(VariableAccess).getTarget() | ( @@ -82,16 +83,19 @@ int getBufferSize(Expr bufferExpr, Element why) { why = bufferExpr ) or ( // dataflow (all sources must be the same size) - forex(Expr def | + result = min(Expr def | DataFlow::localFlowStep(DataFlow::exprNode(def), DataFlow::exprNode(bufferExpr)) | - result = getBufferSize(def, _) + getBufferSize(def, _) + ) and result = max(Expr def | + DataFlow::localFlowStep(DataFlow::exprNode(def), DataFlow::exprNode(bufferExpr)) | + getBufferSize(def, _) ) and // find reason exists(Expr def | DataFlow::localFlowStep(DataFlow::exprNode(def), DataFlow::exprNode(bufferExpr)) | why = def or - result = getBufferSize(def, why) + exists(getBufferSize(def, why)) ) ) or exists(Type bufferType | // buffer is the address of a variable