WIP: Add '#nohusky' tag to Brakeman

victormazevedo commented 4 years ago

Description

This PR aims to add '#nohusky' tag to Ruby's files to avoid false positives.

Closes #508

Proposed Changes

api/securitytest/brakeman.go: add VerifyNoHusky logic api/util/util.go: here I used banditCase func to Brakeman files. If it is the right approach, I think the banditCase could be renamed. api/util/util_test.go: add some unit tests. All tests passed.

Testing

I've tried to test my implementations with the step below:

In .env:

export HUSKYCI_CLIENT_REPO_BRANCH="poc-ruby-brakeman"

After:

source .env
make run-client

But, after this test it seems that my changes doesn't reflect in it.

Sample of output:


[HUSKYCI][*] poc-ruby-brakeman -> https://github.com/globocom/huskyCI.git
[HUSKYCI][*] huskyCI analysis started! xCgdK3GGrHvANJ90FDd66ZWcGN8QhRxe

[HUSKYCI][!] Title: Vulnerable Dependency: Command Injection Possible command injection
[HUSKYCI][!] Language: Ruby
[HUSKYCI][!] Tool: Brakeman
[HUSKYCI][!] Confidence: Medium
[HUSKYCI][!] Details: https://brakemanscanner.org/docs/warning_types/command_injection/
[HUSKYCI][!] File: app/controllers/application_controller.rb
[HUSKYCI][!] Line: 4
[HUSKYCI][!] Code: system("ls #{options}")
[HUSKYCI][!] Type: Command Injection

[HUSKYCI][SUMMARY] Ruby -> huskyci/brakeman:4.8.2
[HUSKYCI][SUMMARY] High: 0
[HUSKYCI][SUMMARY] Medium: 1
[HUSKYCI][SUMMARY] Low: 0
[HUSKYCI][SUMMARY] NoSecHusky: 0

[HUSKYCI][SUMMARY] Total
[HUSKYCI][SUMMARY] High: 0
[HUSKYCI][SUMMARY] Medium: 1
[HUSKYCI][SUMMARY] Low: 0
[HUSKYCI][SUMMARY] NoSecHusky: 0

[HUSKYCI][*] The following securityTests were executed and no blocking vulnerabilities were found:
[HUSKYCI][*] [huskyci/gitleaks:2.1.0]
[HUSKYCI][*] Some HIGH/MEDIUM issues were found in these securityTests:
[HUSKYCI][*] [huskyci/brakeman:4.8.2]
make: *** [run-client] Error 190

victormazevedo commented 4 years ago

I don't know how to debug properly and be sure of this, but it seems like Brakeman ignores comments in code (I'm reading Brakeman doc to understand more how it works under the table). In that way, when VerifyNoHusy is called with the warning.Code in parameter, the func doesn't recognize #nohusky.

Edited: After reading almost all doc of Brakeman and make some local tests, it seems the output does not include comments in Code attribute. So what I thought? When call VerifyNoHusy to Brakeman codes, get warning.File and warning.Line and take the line code from original source instead Brakeman's output. And then verify the #nohusky tag.

victormazevedo commented 4 years ago

In a conversation with Brakeman's maintainer, there's many types of output files. In HTML the whole code is presented, including comments. I'm trying to think in a logic to parse html and compare the file and line code, and save and use HTML's line code instead json's line code.

victormazevedo commented 4 years ago

@rafaveira3 analyzing all scenarios, I think the best is to try to contribute with brakeman to create a feature that enables user to choose if reports will include in warnings code with comments or doesn't (I'll try to do that.). Otherwise, I don't know if you have other ideas.

Things I thought:

Try to get the line code from results.html: after a conversation with Brakeman's maintainer, he said to me that HTML reports have the full code, including the comments. But I think this is not the best approach because .html files are painful to read and extract some information.
Try to get the line code from the original repo (via Bash). That is another painful solution. I've tried to do this: In Brakeman's container, we take advantage that we already had the repo directory. So, I tried to get line and code from JSON using jq and after this getting the content of line using sed. But after this, I've started to think about performance, and well, I don't think this is the best approach considering we may have a lot of warnings in a scan and almost them doesn't present the #nohuskytag.

With all modifications that I've already done, I think if the Brakeman could return warnings with comments, #nohuksy would work.

Krlier commented 3 years ago

Hi, @victormazevedo! Thanks for being interested in this issue and taking the time to contribute to huskyCI!

I've read the scenarios you proposed and I believe what suits us best is to contribute to Brakeman's project with this change to allow commented code to show up in the code output.

By doing this, just as you first proposed, we could reuse some of Bandit's ignore functions. If needed, we could rename than to serve a more general purpose such as this one.

What do you think?

rafaveira3 commented 3 years ago

Hey, @victormazevedo! Sorry for the long response, I am not handling globocom hacktoberfest issues anymore. Thanks a lot for the incredible effort you have done so far on this one. :smiley:

victormazevedo commented 3 years ago

Hi, @victormazevedo! Thanks for being interested in this issue and taking the time to contribute to huskyCI!

I've read the scenarios you proposed and I believe what suits us best is to contribute to Brakeman's project with this change to allow commented code to show up in the code output.

By doing this, just as you first proposed, we could reuse some of Bandit's ignore functions. If needed, we could rename than to serve a more general purpose such as this one.

What do you think?

Agreed, @Krlier! I don't know at which time I'll finish this because I've never worked with Ruby before, but this is not a problem! I'll figure out and will update you!

globocom / huskyCI