Skip to content
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

Go: Switch from def-use flow to use-use flow #14751

Draft
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

owen-mc
Copy link
Contributor

@owen-mc owen-mc commented Nov 11, 2023

This is a resurrection of github/codeql-go#460. Rebasing seems to have gone okay. There are more test changes. I am currently going through them to see if they are expected or if they indicate that something need to change.

@github-actions github-actions bot added the Go label Nov 11, 2023
@owen-mc
Copy link
Contributor Author

owen-mc commented Nov 11, 2023

There are two lost results for go/ql/src/InconsistentCode/UnhandledCloseWritableHandle.ql on go/ql/test/query-tests/InconsistentCode/UnhandledCloseWritableHandle/tests.go. They have the exact same cause: in the following function

func closeFileDeferredIndirect(f *os.File) {
	var cont = func() {
		f.Close() // NOT OK, if `f` is writable
	}

	defer cont()
}

we don't have flow from capture variable f in the function literal to the use of f in f.Close(). This flow used to be caught by this disjunct in basicLocalFlowStep:

  // SSA -> Instruction
  exists(SsaDefinition pred, IR::Instruction succ |
    succ = pred.getVariable().getAUse() and
    nodeFrom = ssaNode(pred) and
    nodeTo = instructionNode(succ)
  )

but now that has changed to:

  // SSA defn -> first SSA use
  exists(SsaExplicitDefinition pred, IR::Instruction succ | succ = pred.getAFirstUse() |
    nodeFrom = ssaNode(pred) and
    nodeTo = instructionNode(succ)
  )
  or
  // SSA use -> successive SSA use
  // Note this case includes Phi node traversal
  exists(IR::Instruction pred, IR::Instruction succ | succ = getAnAdjacentUse(pred) |
    nodeFrom = instructionNode(pred) and
    nodeTo = instructionNode(succ)
  )

capture variable f is an SsaImplicitDefinition rather than an SsaExplicitDefinition, so the first of these disjuncts doesn't catch it. I am not sure which of these disjuncts should be amended to capture this flow.

@smowton
Copy link
Contributor

smowton commented Nov 13, 2023

Presumably this is because of switching from SsaDefinition to SsaExplicitDefintion, I suppose with intent to exclude phi nodes, but also excluding SsaVariableCapture by mistake.

@owen-mc owen-mc force-pushed the go/feature/use-use-flow branch 2 times, most recently from 7bc600c to 7367e46 Compare November 30, 2023 15:15
this = call.getOperand(_, safeDirective) and
// Mark "%q" formats as safe, but not "%#q", which would preserve newline characters.
safeDirective.regexpMatch("%[^%#]*q")
exists(DataFlow::Node arg, StringOps::Formatting::StringFormatCall call |

Check warning

Code scanning / CodeQL

Omittable 'exists' variable Warning

This exists variable can be omitted by using a don't-care expression
in this argument
.
varBlockReachesBaseCand(v, b1, b2) and
blockPrecedesVar(v, b2)
or
exists(ReachableBasicBlock mid |

Check warning

Code scanning / CodeQL

Omittable 'exists' variable Warning

This exists variable can be omitted by using a don't-care expression
in this argument
.
@owen-mc
Copy link
Contributor Author

owen-mc commented Dec 5, 2023

DCA showed a few extra alerts, which I've pushed a commit to fix, and perhaps a very slight increase in analysis time.

owen-mc and others added 16 commits December 6, 2023 15:37
…implement.

Queries / tests that required changes:
* The CleartextLogging and MissingErrorCheck queries are updated because they assumed def-use flow
* The CommandInjection query works around the shortcomings of use-use flow by essentially reintroducing def-use flow when it applies a sanitizer
* The OpenUrlRedirect query currently just accepts its fate; the tests are updated to avoid excess sanitization while the query comments on the problem. We should choose this approach or the CommandInjection one.
No changes in functionality.
Without this change the test
go/ql/test/query-tests/InconsistentCode/UnhandledCloseWritableHandle/UnhandledCloseWritableHandle.qlref
was failing.
Make it sanitize the result of the call rather than the input, so that
further uses of the input are still tainted. This means that it catches
things like `log.Print(fmt.Sprintf("user %q logged in.\n", username))`
where the argument to the LoggerCall contains a StringFormatCall, but
it misses things like `log.Printf("user %q logged in.\n", username)`. So
we extract the logic into a predicate and apply it as a condition in the
sink as well.

The downside of this approach is that if there are two tainted inputs
and only one has a safe format argument then we still sanitize the
result. Hopefully this is rare.
Previously, basiclocalflowstep was not flowing to variable captures
properly, so getACalleeSource was not finding any predecessors for
`handler` outside of the anonymous function.
We have an operator expression like `x * 5`. We want to follow where the
value of the operator expression goes. We used to follow local flow from
an operand, but now there is flow from that operand to the next use of
the variable. The fix is to explicitly start local flow from the
operator expression.

There are also some expected edge changes due to use-use flow.
New line from `rows` on line 25 to itself is formed of two smaller steps,
from `rows` on line 25 to `rows` on line 20 and then back again. These
are both from basicLocalFlowStep and are steps from a use to the next
use.
We were assuming that `sink` only had one successor, the TypeCastNode, but it
can now have an adjacent use as well.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants