Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Add '#nohusky' tag to Brakeman #521

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

victormazevedo
Copy link

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
Copy link
Author

victormazevedo commented Oct 17, 2020

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
Copy link
Author

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
Copy link
Author

@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
Copy link
Contributor

Krlier commented Oct 30, 2020

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
Copy link
Contributor

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. 😃

@victormazevedo
Copy link
Author

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add '#nohusky' tag to Brakeman scans
3 participants