C#: New query cs/uncontrolled-format-string - #513
Conversation
hvitved
left a comment
hvitved
left a comment
There was a problem hiding this comment.
Looks good, I just have a few suggestions.
| import DataFlow::PathGraph | ||
|
|
||
| class FormatStringConfiguration extends TaintTracking::Configuration | ||
| { |
|
|
||
| override predicate isSink(DataFlow::Node sink) { | ||
| exists(MethodCall call | sink.asExpr() = call.getArgumentForName("format") and | ||
| call.getTarget() = any(SystemStringClass s).getFormatMethod() |
There was a problem hiding this comment.
Import semmle.code.csharp.frameworks.Format and do
exists(MethodCall call, FormatMethod fm, Parameter p |
sink.asExpr() = call.getArgumentForParameter(p) |
call.getTarget() = fm and
p = fm.getParameter(fm.getFormatArgument())
)
There was a problem hiding this comment.
This is a really great suggestion. I found FormatCall which made things even simpler.
| from FormatStringConfiguration config, DataFlow::PathNode source, DataFlow::PathNode sink | ||
| where config.hasFlowPath(source, sink) | ||
| select sink.getNode(), source, sink, | ||
| "$@ flows to here and is used to format 'String.Format'.", source.getNode(), source.getNode().toString() |
There was a problem hiding this comment.
used to format 'String.Format' -> used as format string ?
jf205
left a comment
jf205
left a comment
There was a problem hiding this comment.
One comment on the text in the qhelp file - it is a matter of personal preference, so feel free to ignore.
One question about the example you include in the qhelp too - again feel free to ignore.
Otherwise LGTM.
| </overview> | ||
| <recommendation> | ||
|
|
||
| <p>Use a string literal for the format string, to prevent the possibility of data flow from |
There was a problem hiding this comment.
I would delete the comma after string.
| the application to crash.</p> | ||
|
|
||
| <sample src="UncontrolledFormatStringBad.cs" /> | ||
|
|
There was a problem hiding this comment.
If this is very obvious then please ignore this comment, but would it be helpful to include a 'fixed' version of your example here?
There was a problem hiding this comment.
It should normally be the case, but it's quite hard in this case because the fix would require quite a convoluted redesign that I think is beyond the scope of this help. I note that many other security queries don't have the Good version either.
b9ce1b8 to
6b2e339
Compare
3 participants
Should hopefully be straightforward. Note I haven't covered Format-like methods that forward the argument to
String.Format-- this would in some cases be handled by normal data-flow anyway.