Model stdlib's IO package. #131
Conversation
ghost
mentioned this pull request
max-schaefer
left a comment
max-schaefer
left a comment
There was a problem hiding this comment.
LGTM on the whole, thanks for adding so many models! A few comments on consistency and typos. Perhaps a few tests would be good as well, they would have helped catch some of the latter.
Once #129 has landed we'll need to do an evaluation of this PR.
ql/src/semmle/go/frameworks/IO.qll
Outdated
| private class Copy extends TaintTracking::FunctionModel, Function { | ||
| // Include the Copy and CopyBuffer functions but exclude the | ||
| // CopyN function as it can't be realistically determined how long | ||
| // a buffer is enough for a particular exploit. |
There was a problem hiding this comment.
That's an interesting point, but I'd like to see a false positive first before excluding it.
There was a problem hiding this comment.
For a normal webapp, you can imagine a case where an untrusted parameter or something flows into a XSS sink through a copyN call. Now, for most purposes, if n is small enough say 5-10 bytes, regardless of whether the input is sanitised by any other means or not, the flow would not lead to any security issues as an attacker can't realisticaly frame any decent XSS Payload with just 5 bytes. This would lead to a false positive.
The issue may be more prominent in low level systems and crypto code but unfortunately, off the top of my head, I can't come up with any bad examples as of now.
There was a problem hiding this comment.
On second thought, I have included this with the PR. I think the True positive/false positive ratio would favor this change.
|
I have added the test now. I am also modelling functions from the bufio and other pacakges for which I will send in another PR soon. |
|
Can you rebase or merge this over master now that the #129 has been merged? |
|
Rebased and squashed for easier merging. |
|
I've started an evaluation, once that is done I think this is ready to merge. |
max-schaefer
left a comment
max-schaefer
left a comment
There was a problem hiding this comment.
Evaluation was uneventful; lgtm.
max-schaefer
added
the
needs-polishing
ghost
deleted the
max-schaefer
added
no-change-note-required
and removed
needs-polishing
ghost
mentioned this pull request
2 participants
This PR models the golang stdlib io package. It depends on #129.
Here's a brief of changes made.
Reader.Read,Writer.Write,WriteStringwhich Max wrote in Fix and improve taint-tracking through function arguments #129 into a newIO.qllfile. I have also added a few doc comments on them.iopackage. From what I can see, this completes the entire package.Also, please note that I have based my PR on #129. Since, the PR is not yet merged, you may have to cherry-pick the commits before merging.