Skip to content

Commit 9623bcc

Browse files
dandersonappsecaidandersonappsecai
committed
Modified the fix verification feature of the Benchmark verification crawler based on feedback from Dave Wichers.
1 parent 95df346 commit 9623bcc

File tree

2 files changed

+114
-34
lines changed

2 files changed

+114
-34
lines changed

‎plugin/src/main/java/org/owasp/benchmarkutils/tools/BenchmarkCrawlerVerification.java

Lines changed: 105 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -22,9 +22,13 @@
2222
import java.io.FileNotFoundException;
2323
import java.io.FileWriter;
2424
import java.io.IOException;
25+
import java.nio.file.Files;
26+
import java.nio.file.Paths;
2527
import java.util.ArrayList;
2628
import java.util.Date;
29+
import java.util.HashMap;
2730
import java.util.List;
31+
import java.util.Map;
2832
import java.util.stream.Collectors;
2933
import javax.xml.bind.JAXBException;
3034
import javax.xml.parsers.ParserConfigurationException;
@@ -68,10 +72,12 @@ public class BenchmarkCrawlerVerification extends BenchmarkCrawler {
6872
private static boolean isTimingEnabled = false;
6973
// private boolean verifyFixed = false; // DEBUG
7074
private String configurationDirectory = Utils.DATA_DIR;
71-
private String defaultOutputDirectory = Utils.DATA_DIR;
72-
private String defaultBeforeFixOutputDirectory =
73-
new File(new File(Utils.DATA_DIR).getParent(), "before_data")
74-
.getAbsolutePath(); // DEBUG: Utils.DATA_DIR;
75+
private String defaultOutputDirectory = configurationDirectory;
76+
// private String defaultFixedOutputDirectory = Paths.get(Utils.DATA_DIR,
77+
// "fixstatus").toString();
78+
// private String defaultUnfixedSrcDirectory =
79+
// new File(new File(Utils.DATA_DIR).getParent(), "before_data")
80+
// .getAbsolutePath(); // DEBUG: Utils.DATA_DIR;
7581
private static final String FILENAME_TIMES_ALL = "crawlerTimes.txt";
7682
private static final String FILENAME_TIMES = "crawlerSlowTimes.txt";
7783
private static final String FILENAME_NON_DISCRIMINATORY_LOG = "nonDiscriminatoryTestCases.txt";
@@ -88,21 +94,27 @@ public class BenchmarkCrawlerVerification extends BenchmarkCrawler {
8894
SimpleFileLogger eLogger;
8995
SimpleFileLogger uLogger;
9096

91-
@Parameter(property = "json", defaultValue = "false")
97+
@Parameter(property = "generateJSONResults", defaultValue = "false")
9298
private String generateJSONResults;
9399

94100
@Parameter(property = "verifyFixed", defaultValue = "false")
95101
private String verifyFixed;
96102

97-
@Parameter(property = "beforeFixOutputDirectory")
98-
private String beforeFixOutputDirectory;
103+
@Parameter(property = "unfixedSrcDirectory")
104+
private String unfixedSourceDirectory;
105+
106+
@Parameter(property = "fixedSrcDirectory")
107+
private String fixedSourceDirectory;
99108

100109
@Parameter(property = "outputDirectory")
101110
private String outputDirectory;
102111

103112
@Parameter(property = "testCaseName")
104113
private String selectedTestCaseName;
105114

115+
private Map<String, TestCaseVerificationResults> testCaseNameToTestCaseVerificationResultsMap =
116+
new HashMap<>();
117+
106118
BenchmarkCrawlerVerification() {
107119
// A default constructor required to support Maven plugin API.
108120
// The theCrawlerFile has to be instantiated before a crawl can be done.
@@ -352,7 +364,7 @@ protected void crawl(TestSuite testSuite) throws Exception {
352364

353365
// Then generate JSON file with ALL verification results if generateJSONResults
354366
// is enabled. This has to go at end because previous methods have some side
355-
// affects that fill in test case verification values.
367+
// effects that fill in test case verification values.
356368
RegressionTesting.genAllTCResultsToJsonFile(
357369
resultsCollection,
358370
getOutputDirectory(),
@@ -391,7 +403,7 @@ protected void crawl(TestSuite testSuite) throws Exception {
391403
* @param testSuite
392404
* @throws Exception
393405
*/
394-
protected void crawlToVerifyFix(TestSuite testSuite, String beforeFixOutputDirectory)
406+
protected void crawlToVerifyFix(TestSuite testSuite, String unfixedOutputDirectory)
395407
throws Exception {
396408

397409
// Get config directory for RUN 1 from CLI option
@@ -481,15 +493,15 @@ private void log(ResponseInfo responseInfo) throws IOException {
481493
/**
482494
* For the verification crawler, processing the result means verifying whether the test case is
483495
* actually vulnerable or not, relative to whether it is supposed to be vulnerable. This method
484-
* has a side-affect of setting request.setPassed() for the current test case. Passing means it
496+
* has a side-effect of setting request.setPassed() for the current test case. Passing means it
485497
* was exploitable for a True Positive and appears to not be exploitable for a False Positive.
486498
*
487499
* @param result - The results required to verify this test case.
488500
* @throws FileNotFoundException
489501
* @throws LoggerConfigurationException
490502
*/
491503
protected void handleResponse(TestCaseVerificationResults results)
492-
throws FileNotFoundException, LoggerConfigurationException {
504+
throws FileNotFoundException, IOException, LoggerConfigurationException {
493505

494506
// Check to see if this specific test case has a specified expected response value.
495507
// If so, run it through verification using it's specific attackSuccessIndicator.
@@ -498,18 +510,46 @@ protected void handleResponse(TestCaseVerificationResults results)
498510
RegressionTesting.verifyTestCase(results);
499511

500512
if (Boolean.parseBoolean(verifyFixed)) {
501-
TestCaseVerificationResultsCollection beforeFixResultsCollection =
502-
loadTestCaseVerificationResults(beforeFixOutputDirectory);
503-
TestCaseVerificationResults beforeFixResults =
504-
beforeFixResultsCollection.getResultsObjects().get(0);
505-
if (beforeFixResults.getTestCase().getName().equals(results.getTestCase().getName())) {
506-
verifyFix(beforeFixResults, results);
513+
String unfixedOutputDirectory = configurationDirectory;
514+
515+
TestCaseVerificationResults fixedResults = results;
516+
TestCaseVerificationResultsCollection unfixedResultsCollection =
517+
loadTestCaseVerificationResults(unfixedOutputDirectory);
518+
TestCaseVerificationResults unfixedResults =
519+
testCaseNameToTestCaseVerificationResultsMap.get(
520+
fixedResults.getTestCase().getName());
521+
if (unfixedResults
522+
.getTestCase()
523+
.getName()
524+
.equals(fixedResults.getTestCase().getName())) {
525+
// FIXME: Generalize this so it can support languages other than Java and multiple
526+
// source files per testcase.
527+
String unfixedSourceFile =
528+
Paths.get(unfixedSourceDirectory, unfixedResults.getTestCase().getName())
529+
.toString()
530+
+ ".java";
531+
String fixedSourceFile =
532+
Paths.get(fixedSourceDirectory, fixedResults.getTestCase().getName())
533+
.toString()
534+
+ ".java";
535+
String unfixedSourceFileContents =
536+
new String(Files.readAllBytes(Paths.get(unfixedSourceFile)));
537+
String fixedSourceFileContents =
538+
new String(Files.readAllBytes(Paths.get(fixedSourceFile)));
539+
if (!unfixedSourceFileContents.equals(fixedSourceFileContents)) {
540+
verifyFix(unfixedResults, fixedResults);
541+
} else {
542+
System.out.println(
543+
"WARNING: Testcase "
544+
+ fixedResults.getTestCase().getName()
545+
+ " source file unmodified");
546+
}
507547
} else {
508548
System.out.println(
509549
"WARNING: After fix testcase is "
510-
+ results.getTestCase().getName()
550+
+ fixedResults.getTestCase().getName()
511551
+ " but before fix testcase is "
512-
+ beforeFixResults.getTestCase().getName());
552+
+ unfixedResults.getTestCase().getName());
513553
}
514554
}
515555
}
@@ -522,6 +562,11 @@ private TestCaseVerificationResultsCollection loadTestCaseVerificationResults(
522562
results =
523563
Utils.jsonToTestCaseVerificationResultsList(
524564
new File(directory, FILENAME_TC_VERIF_RESULTS_JSON));
565+
for (TestCaseVerificationResults testCaseResults : results.getResultsObjects()) {
566+
testCaseNameToTestCaseVerificationResultsMap.put(
567+
testCaseResults.getTestCase().getName(), testCaseResults);
568+
}
569+
525570
} catch (JAXBException
526571
| FileNotFoundException
527572
| SAXException
@@ -554,6 +599,10 @@ private boolean verifyFix(
554599
TestCaseVerificationResults beforeFixResults,
555600
TestCaseVerificationResults afterFixResults) {
556601

602+
boolean wasNotVerfiable =
603+
afterFixResults.getTestCase().isVulnerability()
604+
&& afterFixResults.getTestCase().isUnverifiable()
605+
&& afterFixResults.isPassed();
557606
boolean wasExploited =
558607
afterFixResults.getTestCase().isVulnerability()
559608
&& !afterFixResults.getTestCase().isUnverifiable()
@@ -563,6 +612,9 @@ private boolean verifyFix(
563612
.getResponseToSafeValue()
564613
.getResponseString()
565614
.equals(afterFixResults.getResponseToSafeValue().getResponseString());
615+
if (wasNotVerfiable) {
616+
System.out.println("NOT FIXED: Vulnerability could not be verified");
617+
}
566618
if (wasExploited) {
567619
System.out.println("NOT FIXED: Vulnerability was exploited");
568620
}
@@ -573,6 +625,7 @@ private boolean verifyFix(
573625
File verifyFixResultFile = new File(getOutputDirectory(), FILENAME_VERIFY_FIX_RESULT);
574626
try (BufferedWriter writer = new BufferedWriter(new FileWriter(verifyFixResultFile))) {
575627
VerifyFixOutput verifyFixOutput = new VerifyFixOutput();
628+
verifyFixOutput.setWasNotVerfiable(wasNotVerfiable);
576629
verifyFixOutput.setWasExploited(wasExploited);
577630
verifyFixOutput.setWasBroken(wasBroken);
578631
String output = Utils.objectToJson(verifyFixOutput);
@@ -587,7 +640,7 @@ private boolean verifyFix(
587640
e.printStackTrace();
588641
}
589642

590-
return !wasExploited && !wasBroken;
643+
return !wasNotVerfiable && !wasExploited && !wasBroken;
591644
}
592645

593646
private boolean verifyFixes(
@@ -638,12 +691,18 @@ protected void processCommandLineArgs(String[] args) {
638691
Options options = new Options();
639692
options.addOption(
640693
Option.builder("b")
641-
.longOpt("beforeFixOutputDirectory")
642-
.desc("output directory of a previous crawl before fixes")
694+
.longOpt("unfixedSrcDirectory")
695+
.desc("source directory before fixes")
696+
.hasArg()
697+
.build());
698+
options.addOption(
699+
Option.builder("a")
700+
.longOpt("fixedSrcDirectory")
701+
.desc("source directory after fixes")
643702
.hasArg()
644703
.build());
645704
options.addOption(
646-
Option.builder("d")
705+
Option.builder("o")
647706
.longOpt("outputDirectory")
648707
.desc("output directory")
649708
.hasArg()
@@ -658,7 +717,7 @@ protected void processCommandLineArgs(String[] args) {
658717
options.addOption(Option.builder("h").longOpt("help").desc("Usage").build());
659718
options.addOption(
660719
Option.builder("j")
661-
.longOpt("json")
720+
.longOpt("generateJSONResults")
662721
.desc("generate json version of verification results")
663722
.build());
664723
// options.addOption("m", "verifyFixed", false, "verify fixed test suite");
@@ -682,16 +741,19 @@ protected void processCommandLineArgs(String[] args) {
682741
// Parse the command line arguments
683742
CommandLine line = parser.parse(options, args);
684743

685-
if (line.hasOption("b")) {
686-
beforeFixOutputDirectory = line.getOptionValue("b");
687-
} else {
688-
beforeFixOutputDirectory = defaultBeforeFixOutputDirectory;
689-
}
690-
if (line.hasOption("d")) {
691-
outputDirectory = line.getOptionValue("d");
744+
if (line.hasOption("o")) {
745+
outputDirectory = line.getOptionValue("o");
692746
} else {
693747
outputDirectory = defaultOutputDirectory;
694748
}
749+
// Required if in verifyFix mode
750+
if (line.hasOption("b")) {
751+
unfixedSourceDirectory = line.getOptionValue("b");
752+
}
753+
// Required if in verifyFix mode
754+
if (line.hasOption("a")) {
755+
fixedSourceDirectory = line.getOptionValue("a");
756+
}
695757
if (line.hasOption("f")) {
696758
this.crawlerFile = line.getOptionValue("f");
697759
File targetFile = new File(this.crawlerFile);
@@ -720,6 +782,11 @@ protected void processCommandLineArgs(String[] args) {
720782
if (line.hasOption("t")) {
721783
maxTimeInSeconds = (Integer) line.getParsedOptionValue("t");
722784
}
785+
786+
// The default is different if we are in verifyFix mode
787+
if (Boolean.parseBoolean(this.verifyFixed)) {
788+
outputDirectory = Paths.get(Utils.DATA_DIR, "fixstatus").toString();
789+
}
723790
} catch (ParseException e) {
724791
formatter.printHelp("BenchmarkCrawlerVerification", options);
725792
throw new RuntimeException("Error parsing arguments: ", e);
@@ -740,12 +807,16 @@ public void execute() throws MojoExecutionException, MojoFailureException {
740807
mainArgs.add("-f");
741808
mainArgs.add(this.pluginFilenameParam);
742809
if (this.outputDirectory != null) {
743-
mainArgs.add("-d");
810+
mainArgs.add("-o");
744811
mainArgs.add(this.outputDirectory);
745812
}
746-
if (this.beforeFixOutputDirectory != null) {
813+
if (this.unfixedSourceDirectory != null) {
747814
mainArgs.add("-b");
748-
mainArgs.add(this.beforeFixOutputDirectory);
815+
mainArgs.add(this.unfixedSourceDirectory);
816+
}
817+
if (this.fixedSourceDirectory != null) {
818+
mainArgs.add("-a");
819+
mainArgs.add(this.fixedSourceDirectory);
749820
}
750821
if (this.pluginTestCaseNameParam != null) {
751822
mainArgs.add("-n");

‎plugin/src/main/java/org/owasp/benchmarkutils/tools/VerifyFixOutput.java

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,9 +21,18 @@
2121

2222
@XmlRootElement
2323
public class VerifyFixOutput {
24+
private boolean wasNotVerfiable;
2425
private boolean wasExploited;
2526
private boolean wasBroken;
2627

28+
public boolean isWasNotVerfiable() {
29+
return wasNotVerfiable;
30+
}
31+
32+
public void setWasNotVerfiable(boolean wasNotVerfiable) {
33+
this.wasNotVerfiable = wasNotVerfiable;
34+
}
35+
2736
public boolean isWasExploited() {
2837
return wasExploited;
2938
}

0 commit comments

Comments
 (0)