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.

Sign up for GitHub

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

Jump to bottom

benchmarkUtils\fix(reader): adjust reader #122

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

benchmarkUtils\fix(reader): adjust reader #122

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 29 additions & 8 deletions plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/FluidAttacksReader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,26 @@ public class FluidAttacksReader extends Reader {

@Override
public boolean canRead(ResultFile resultFile) {
return resultFile.filename().endsWith("csv")
&& resultFile
.line(0)
.trim()
.equals(
"title,cwe,description,cvss,finding,stream,kind,where,snippet,method");
if (!resultFile.filename().endsWith("csv")) {
return false;
}

String headerLine = resultFile.line(0).trim();
String[] headers = headerLine.split(",");

boolean hasCwe = false;
boolean hasDescription = false;

for (String header : headers) {
if (header.equalsIgnoreCase("cwe")) {
hasCwe = true;
}
if (header.equalsIgnoreCase("description")) {
hasDescription = true;
}
}

return hasCwe && hasDescription;
}

@Override
Expand All @@ -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 :)

description = record.get("description");
} catch (IllegalArgumentException e) {
continue;
}

if (description == null || description.split("OWASP").length < 2) {
continue;
}
String what = record.get("description").split("OWASP")[1];
Expand Down Expand Up @@ -134,4 +155,4 @@ private static String cweToCategory(String cwe) {
return "other";
}
}
}
}
2 changes: 1 addition & 1 deletion plugin/src/test/java/org/owasp/benchmarkutils/score/parsers/FluidAttacksReaderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ void readerHandlesGivenResultFile() throws Exception {
assertTrue(result.isCommercial());
assertEquals("Fluid Attacks", result.getToolName());

assertEquals(2, result.getTotalResults());
assertEquals(3, result.getTotalResults());

assertEquals(CweNumber.SQL_INJECTION, result.get(1).get(0).getCWE());
assertEquals(CweNumber.COMMAND_INJECTION, result.get(2).get(0).getCWE());
Expand Down
8 changes: 5 additions & 3 deletions plugin/src/test/resources/testfiles/Benchmark_Fluid-Attacks-v20210416.csv
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
title,cwe,description,cvss,finding,stream,kind,where,snippet,method
112. SQL injection - Java SQL API,CWE-89,Use of attacker controlled parameters for querying the data base in OWASP/src/main/java/org/owasp/benchmark/testcode/BenchmarkTest00001.java,CVSS:3.1/AV:N/AC:H/PR:L/UI:N/S:U/C:L/I:N/A:N/E:U/RL:O/RC:R,https://docs.fluidattacks.com/criteria/vulnerabilities/112,skims,SAST,59,"code snippet",java.java_sql_injection
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,https://docs.fluidattacks.com/criteria/vulnerabilities/004,skims,SAST,71,"code snippet",java.java_remote_command_execution
title,cwe,description,cvss,cvss_v4,finding,stream,kind,where,snippet,method
112. SQL injection - Java SQL API,CWE-89,Use of attacker controlled parameters for querying the data base in OWASP/src/main/java/org/owasp/benchmark/testcode/BenchmarkTest00001.java,CVSS:3.1/AV:N/AC:H/PR:L/UI:N/S:U/C:L/I:N/A:N/E:U/RL:O/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/112,skims,SAST,59,"code snippet",java.java_sql_injection
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).