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

benchmarkUtils\fix(reader): adjust reader #122

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

Conversation

alejolagosm
Copy link
Contributor

  • adjust fluidattacks reader
  • adjust test file to conform to new format

- adjust fluidattacks reader
- adjust test file to conform to new format
@davewichers
Copy link
Contributor

@darkspirit510 - Can you review this change and make sure its still backward compatible with the older results format?

Copy link
Contributor

@darkspirit510 darkspirit510 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code itself looks good. Please have a look at my comments.

@@ -48,7 +62,14 @@ public TestSuiteResults parse(ResultFile resultFile) throws Exception {
for (CSVRecord record : records) {
TestCaseResult testCaseResult = new TestCaseResult();
// Read only useful rows of the csv results
if (record.get("description").split("OWASP").length < 2) {
String description;
try {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer not to use Exceptions in a regular programming way. I had a quick (untested) look to CSVRecord class. Can you have a look at the method isMapped as a replacement of the try/catch?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do! Thanks :)

004. Remote command execution,CWE-78,"The system builds system commands using inputs that can be manipulated externally, it does not properly override special elements that could modify the system command in OWASP/src/main/java/org/owasp/benchmark/testcode/BenchmarkTest00002.java",CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:C/C:H/I:H/A:H/E:U/RL:T/RC:R,CVSS:4.0/AV:A/AC:L/AT:N/PR:L/UI:N/VC:L/VI:N/VA:N/SC:N/SI:N/SA:N,https://docs.fluidattacks.com/criteria/vulnerabilities/004,skims,SAST,71,"code snippet",java.java_remote_command_execution
004. Remote command execution,CWE-78,"The system builds system commands using inputs that can be manipulated externally, it does not properly override special elements that could modify the system command in OWASP/src/main/java/org/owasp/benchmark/testcode/BenchmarkTest00003.java",CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:C/C:H/I:H/A:H/E:U/RL:T/RC:R,CVSS:4.0/AV:A/AC:L/AT:N/PR:L/UI:N/VC:L/VI:N/VA:N/SC:N/SI:N/SA:N,https://docs.fluidattacks.com/criteria/vulnerabilities/004,skims,SAST,89,"code snippet",java.java_remote_command_execution
Summary: 2 Vulnerabilities were found in your execution
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have an old result file (because I don't have one) to ensure that your change does not prevent Benchmark from reading Fluid result files from previous versions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous format is basically the same file that is now on the repo, so I can leave that one as is, and add a new test file, just to make sure the change works with both of them.

Although, it is always recommended to use the Fluid Attacks' scanner using the latest available version, so I would prefer to leave just the latest format.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, you should always use the latest version, but Benchmark allows to pass multiple result files, even from the same Tool (in different versions) to compare how scoring changed over given versions. That's why Benchmark should be able to read any version for a tool (except for discontinued Tools, they will be deleted eventually).

@darkspirit510
Copy link
Contributor

Oh, and by the way - could you provide a complete result file to either me ([email protected]) or @davewichers ([email protected]), preferably both old and new version? That way, we could verify everything works and keeps working if someone else submits a PR.

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.

3 participants