New Rule Tainted file

[wileystar]

I closed the previous rule/lfi branch and re-created it here. The git history was very messy and difficult to follow so I wanted to start clean. I implemented most of the recommendations from @cosmincojocar. I can investigate the CFG library and work on a general solution we can use in GoAST.

[ccojocar]

lgtm

[wileystar]

thanks for the approval @cosmincojocar -- what's the process to merge an approved PR?

[gcmurphy]

@coredefend I'll try to find some time today to do final review and merge for this change. (Bit busy with other things ATM).

[wileystar]

Thanks!

…

On Mar 6, 2018 7:01 PM, "Grant Murphy" ***@***.***> wrote: @coredefend <https://github.com/coredefend> I'll try to find some time today to do final review and merge for this change. (Bit busy with other things ATM). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#183 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AOLa9McnIh342Ml-i_LTP-aSBWoclGCAks5tbyNpgaJpZM4SdjB7> .

[gcmurphy]

Finally got a chance to take look at this. dd0a112 looks on the right track.

I do have concerns that this will be a noisy rule. Currently it will warn whenever open a file using a variable so that could be a lot. (However we do report unhandled errors which is considerably worse).

IMO the most useful case we'd be trying to defend against here is within http server context so we might be able to do more to reduce other false positives.

If we went down this path I would consider adding these types of enhancements:

• Detecting if the file open is in the context of function signature that matches a standard http handler.
• Tracking cases where identifiers are assigned a value from http request parameters
• Checking the gas context Imports if "net/http" (or other popular frameworks) are being used.

(The last is probably the easiest).

@coredefend @cosmincojocar Thoughts? Do you think we should just report everything or add some additional heuristics for a more specific use case?

[jovonuber]

Hi @gcmurphy, thanks for the comments. I think in this iteration, we should focus on getting the appropriate rules implemented. This rule was modeled after the subproc rule so it should have comparable signal-to-noise ratio. I wonder if we might miss edge cases where there is a vulnerable os.Open outside of the "net/http" (i.e, we forget to update the rule to consider new popular frameworks that perform File I/O)

As a holistic approach, we should implement CFG/DFG w/ taint analysis. This would provide a scalable way to handle heuristics for all of the rules.

I'd vote to report everything and then decide if rule-level fine tuning is needed or library-level taint analysis is our approach.

[ccojocar]

I also agree that this rule is useful in http context. Maybe we could go with the generic reporting for simplicity. We can narrow it down to http if there are complains.

It would be nice in the mid/long term to do some work on a generic data flow analysis. godoctor has something in this direction https://github.com/godoctor/godoctor/blob/master/analysis/dataflow/dataflow.go.

[gcmurphy]

Sounds good. I'll merge this as is and we can look into integrating dataflow analysis to improve rules later on.

[wileystar]

@gcmurphy @ccojocar howdy! i wanted to circle back on cfg features for gosec. is someone actively working on it? if not, i'd like to. let's start a discussion around the design of this feature. i can open an "issue" to discuss...

[ccojocar]

There is this issue #192 for introducing some better flow analysis. I don't think someone is actively working on it. Fell free to propose a solution there and then if everyone agrees, you can start implementing it. Thanks for looking into it!