Model Spew logging framework #375
Conversation
smowton
left a comment
There was a problem hiding this comment.
Looks good -- let's have an lgtm eval with some query that cares about the logger-call range, and please add a change note
ql/src/semmle/go/frameworks/Spew.qll
Outdated
| fn in ["Dump", "Print", "Println"] and | ||
| this = f.getACall() | ||
| | | ||
| f.hasQualifiedName(pkg, fn) |
There was a problem hiding this comment.
Does this not work?
| f.hasQualifiedName(pkg, fn) | |
| this.getTarget().hasQualifiedName(pkg, fn) |
I'd also be tempted to inline packagePath here.
And similar below.
(Is this code generated via @gagliardetto's tool?)
There was a problem hiding this comment.
Good point. The code isn't auto-generated - I think I copied the code from another model and changed the details.
| } | ||
| } | ||
|
|
||
| from TaintTracking::Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink |
There was a problem hiding this comment.
I'd still like to transition to comment-based tests. I should put making a framework for that on my to-do list.
ql/src/semmle/go/frameworks/Spew.qll
Outdated
| } | ||
|
|
||
| override DataFlow::Node getAMessageComponent() { | ||
| exists(int i | i >= 1 | result = this.getArgument(i)) |
There was a problem hiding this comment.
Why are you excluding the format string here and below?
There was a problem hiding this comment.
Good point. I've fixed it now.
|
@smowton I have added a change note. Our modelling of loggers is only used in two places: the cleartext logging query, and to rule out some FPs in ConstantOauth2State. I ran the cleartext logging query on all projects in lgtm with an extra predicate to make sure the sink is from Spew, and got nothing. But that is what I'd expect, as I wouldn't really expect passwords and the like to be in structs that are being pretty-printed. |
|
Fair enough -- I thought perhaps we might spot some things heading to Spew's printf methods as ordinary strings (presumably alongside structs and such that motivate using Spew). Using this is of course more rare than the stdlib |
smowton
left a comment
There was a problem hiding this comment.
Actually one change to make then lgtm
Co-authored-by: Chris Smowton <smowton@github.com>
No description provided.