-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
base: main
Are you sure you want to change the base?
Conversation
There are two lost results for func closeFileDeferredIndirect(f *os.File) {
var cont = func() {
f.Close() // NOT OK, if `f` is writable
}
defer cont()
} we don't have flow from // 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)
)
|
Presumably this is because of switching from SsaDefinition to SsaExplicitDefintion, I suppose with intent to exclude phi nodes, but also excluding SsaVariableCapture by mistake. |
7bc600c
to
7367e46
Compare
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
in this argument
7367e46
to
c44cc9e
Compare
DCA showed a few extra alerts, which I've pushed a commit to fix, and perhaps a very slight increase in analysis time. |
daae010
to
1e590ba
Compare
…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.
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.
We were assuming that `sink` only had one successor, the TypeCastNode, but it can now have an adjacent use as well.
1e590ba
to
a3f940d
Compare
I have just rebased this and fixed new test failures. |
By moving it from the expression evaluation to the type assertion evaluation we don't block flow to successor uses of the same variable.
a3f940d
to
a3dbc5e
Compare
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.