Skip to content

Dataflow: Flow-state changing steps should always be in path explanations #8381

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.

Sign up for GitHub

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

Closed
MathiasVP wants to merge 4 commits into from
Closed

Dataflow: Flow-state changing steps should always be in path explanations #8381

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
00ea8f0
Dataflow: State-changing taint steps should not be stepped over by th…
MathiasVP Mar 10, 2022
cd1f266
Dataflow: Sync identical files.
MathiasVP Mar 10, 2022
a465a05
Java: Accept test changes.
MathiasVP Mar 14, 2022
30953b2
Python: Accept test changes.
MathiasVP Mar 14, 2022
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
72 changes: 63 additions & 9 deletions cpp/ql/lib/semmle/code/cpp/dataflow/internal/DataFlowImpl.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1709,15 +1709,28 @@ private module LocalFlowBigStep {
predicate localFlowEntry(NodeEx node, FlowState state, Configuration config) {
Stage2::revFlow(node, state, config) and
(
sourceNode(node, state, config) or
jumpStep(_, node, config) or
additionalJumpStep(_, node, config) or
additionalJumpStateStep(_, _, node, state, config) or
node instanceof ParamNodeEx or
node.asNode() instanceof OutNodeExt or
store(_, _, node, _, config) or
read(_, _, node, config) or
sourceNode(node, state, config)
or
jumpStep(_, node, config)
or
additionalJumpStep(_, node, config)
or
additionalJumpStateStep(_, _, node, state, config)
or
node instanceof ParamNodeEx
or
node.asNode() instanceof OutNodeExt
or
store(_, _, node, _, config)
or
read(_, _, node, config)
or
node instanceof FlowCheckNode
or
exists(FlowState s |
additionalLocalStateStep(_, s, node, state, config) and
s != state
)
)
}

Expand All @@ -1737,6 +1750,9 @@ private module LocalFlowBigStep {
or
exists(NodeEx next, FlowState s | Stage2::revFlow(next, s, config) |
additionalJumpStateStep(node, state, next, s, config)
or
additionalLocalStateStep(node, state, next, s, config) and
s != state
)
or
Stage2::revFlow(node, state, config) and
Expand Down Expand Up @@ -1773,12 +1789,12 @@ private module LocalFlowBigStep {
NodeEx node1, FlowState state1, NodeEx node2, FlowState state2, boolean preservesValue,
DataFlowType t, Configuration config, LocalCallContext cc
) {
state1 = pragma[only_bind_into](state2) and
not isUnreachableInCallCached(node2.asNode(), cc.(LocalCallContextSpecificCall).getCall()) and
(
localFlowEntry(node1, pragma[only_bind_into](state1), pragma[only_bind_into](config)) and
(
localFlowStepNodeCand1(node1, node2, config) and
state1 = state2 and
preservesValue = true and
t = node1.getDataFlowType() // irrelevant dummy value
or
Expand Down Expand Up @@ -1972,6 +1988,13 @@ private module Stage3 {
ap0 instanceof ApNil
)
or
exists(NodeEx mid, FlowState state0, ApNil nil |
Copy link
Contributor

@hvitved hvitved Mar 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this change, the code for stages 2-4 are no longer in sync. The local call context checks done by localFlowBigStep are also omitted.

Copy link
Contributor Author

@MathiasVP MathiasVP Mar 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this change, the code for stages 2-4 are no longer in sync.
Oops. That's true. I thought I had synced them up, but apparently, I hadn't 🤦.

The local call context checks done by localFlowBigStep are also omitted.

Ah, that's a good point. I guess this is what you fixed with this change in your alternative PR, right?

I must admit I don't fully understand how the dataflow library implements call contexts yet, so I'm not surprised I didn't handle this correctly 😄. Thanks for catching it!

Copy link
Contributor

@hvitved hvitved Mar 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that's a good point. I guess this is what you fixed with this change in your alternative PR, right?

Yes.

fwdFlow(mid, pragma[only_bind_into](state0), cc, argAp, nil, pragma[only_bind_into](config)) and
additionalLocalStateStep(mid, state0, node, state, config) and
state != state0 and
ap = getApNil(node)
)
or
exists(NodeEx mid |
fwdFlow(mid, pragma[only_bind_into](state), _, _, ap, pragma[only_bind_into](config)) and
jumpStep(mid, node, config) and
Expand Down Expand Up @@ -2197,6 +2220,13 @@ private module Stage3 {
ap instanceof ApNil
)
or
exists(NodeEx mid, FlowState state0, ApNil nil |
additionalLocalStateStep(node, state, mid, state0, config) and
state != state0 and
revFlow(mid, state0, toReturn, returnAp, nil, pragma[only_bind_into](config)) and
ap instanceof ApNil
)
or
exists(NodeEx mid |
jumpStep(node, mid, config) and
revFlow(mid, state, _, _, ap, config) and
Expand Down Expand Up @@ -2800,6 +2830,13 @@ private module Stage4 {
ap0 instanceof ApNil
)
or
exists(NodeEx mid, FlowState state0, ApNil nil |
fwdFlow(mid, pragma[only_bind_into](state0), cc, argAp, nil, pragma[only_bind_into](config)) and
additionalLocalStateStep(mid, state0, node, state, config) and
state != state0 and
ap = getApNil(node)
)
or
exists(NodeEx mid |
fwdFlow(mid, pragma[only_bind_into](state), _, _, ap, pragma[only_bind_into](config)) and
jumpStep(mid, node, config) and
Expand Down Expand Up @@ -3025,6 +3062,13 @@ private module Stage4 {
ap instanceof ApNil
)
or
exists(NodeEx mid, FlowState state0, ApNil nil |
additionalLocalStateStep(node, state, mid, state0, config) and
state != state0 and
revFlow(mid, state0, toReturn, returnAp, nil, pragma[only_bind_into](config)) and
ap instanceof ApNil
)
or
exists(NodeEx mid |
jumpStep(node, mid, config) and
revFlow(mid, state, _, _, ap, config) and
Expand Down Expand Up @@ -3901,6 +3945,16 @@ private predicate pathStep(
ap0 instanceof AccessPathNil
)
or
exists(FlowState midstate |
additionalLocalStateStep(mid.getNodeEx(), midstate, node, state, mid.getConfiguration()) and
midstate = mid.getState() and
midstate != state
) and
cc instanceof CallContextAny and
sc instanceof SummaryCtxNone and
mid.getAp() instanceof AccessPathNil and
ap = TAccessPathNil(node.getDataFlowType())
or
jumpStep(mid.getNodeEx(), node, mid.getConfiguration()) and
state = mid.getState() and
cc instanceof CallContextAny and
Expand Down
72 changes: 63 additions & 9 deletions cpp/ql/lib/semmle/code/cpp/dataflow/internal/DataFlowImpl2.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1709,15 +1709,28 @@ private module LocalFlowBigStep {
predicate localFlowEntry(NodeEx node, FlowState state, Configuration config) {
Stage2::revFlow(node, state, config) and
(
sourceNode(node, state, config) or
jumpStep(_, node, config) or
additionalJumpStep(_, node, config) or
additionalJumpStateStep(_, _, node, state, config) or
node instanceof ParamNodeEx or
node.asNode() instanceof OutNodeExt or
store(_, _, node, _, config) or
read(_, _, node, config) or
sourceNode(node, state, config)
or
jumpStep(_, node, config)
or
additionalJumpStep(_, node, config)
or
additionalJumpStateStep(_, _, node, state, config)
or
node instanceof ParamNodeEx
or
node.asNode() instanceof OutNodeExt
or
store(_, _, node, _, config)
or
read(_, _, node, config)
or
node instanceof FlowCheckNode
or
exists(FlowState s |
additionalLocalStateStep(_, s, node, state, config) and
s != state
)
)
}

Expand All @@ -1737,6 +1750,9 @@ private module LocalFlowBigStep {
or
exists(NodeEx next, FlowState s | Stage2::revFlow(next, s, config) |
additionalJumpStateStep(node, state, next, s, config)
or
additionalLocalStateStep(node, state, next, s, config) and
s != state
)
or
Stage2::revFlow(node, state, config) and
Expand Down Expand Up @@ -1773,12 +1789,12 @@ private module LocalFlowBigStep {
NodeEx node1, FlowState state1, NodeEx node2, FlowState state2, boolean preservesValue,
DataFlowType t, Configuration config, LocalCallContext cc
) {
state1 = pragma[only_bind_into](state2) and
not isUnreachableInCallCached(node2.asNode(), cc.(LocalCallContextSpecificCall).getCall()) and
(
localFlowEntry(node1, pragma[only_bind_into](state1), pragma[only_bind_into](config)) and
(
localFlowStepNodeCand1(node1, node2, config) and
state1 = state2 and
preservesValue = true and
t = node1.getDataFlowType() // irrelevant dummy value
or
Expand Down Expand Up @@ -1972,6 +1988,13 @@ private module Stage3 {
ap0 instanceof ApNil
)
or
exists(NodeEx mid, FlowState state0, ApNil nil |
fwdFlow(mid, pragma[only_bind_into](state0), cc, argAp, nil, pragma[only_bind_into](config)) and
additionalLocalStateStep(mid, state0, node, state, config) and
state != state0 and
ap = getApNil(node)
)
or
exists(NodeEx mid |
fwdFlow(mid, pragma[only_bind_into](state), _, _, ap, pragma[only_bind_into](config)) and
jumpStep(mid, node, config) and
Expand Down Expand Up @@ -2197,6 +2220,13 @@ private module Stage3 {
ap instanceof ApNil
)
or
exists(NodeEx mid, FlowState state0, ApNil nil |
additionalLocalStateStep(node, state, mid, state0, config) and
state != state0 and
revFlow(mid, state0, toReturn, returnAp, nil, pragma[only_bind_into](config)) and
ap instanceof ApNil
)
or
exists(NodeEx mid |
jumpStep(node, mid, config) and
revFlow(mid, state, _, _, ap, config) and
Expand Down Expand Up @@ -2800,6 +2830,13 @@ private module Stage4 {
ap0 instanceof ApNil
)
or
exists(NodeEx mid, FlowState state0, ApNil nil |
fwdFlow(mid, pragma[only_bind_into](state0), cc, argAp, nil, pragma[only_bind_into](config)) and
additionalLocalStateStep(mid, state0, node, state, config) and
state != state0 and
ap = getApNil(node)
)
or
exists(NodeEx mid |
fwdFlow(mid, pragma[only_bind_into](state), _, _, ap, pragma[only_bind_into](config)) and
jumpStep(mid, node, config) and
Expand Down Expand Up @@ -3025,6 +3062,13 @@ private module Stage4 {
ap instanceof ApNil
)
or
exists(NodeEx mid, FlowState state0, ApNil nil |
additionalLocalStateStep(node, state, mid, state0, config) and
state != state0 and
revFlow(mid, state0, toReturn, returnAp, nil, pragma[only_bind_into](config)) and
ap instanceof ApNil
)
or
exists(NodeEx mid |
jumpStep(node, mid, config) and
revFlow(mid, state, _, _, ap, config) and
Expand Down Expand Up @@ -3901,6 +3945,16 @@ private predicate pathStep(
ap0 instanceof AccessPathNil
)
or
exists(FlowState midstate |
additionalLocalStateStep(mid.getNodeEx(), midstate, node, state, mid.getConfiguration()) and
midstate = mid.getState() and
midstate != state
) and
cc instanceof CallContextAny and
sc instanceof SummaryCtxNone and
mid.getAp() instanceof AccessPathNil and
ap = TAccessPathNil(node.getDataFlowType())
or
jumpStep(mid.getNodeEx(), node, mid.getConfiguration()) and
state = mid.getState() and
cc instanceof CallContextAny and
Expand Down
Loading