From b8c070e28428ed56383255668e571965a8bcbcc6 Mon Sep 17 00:00:00 2001 From: Theauditor <228822721+TheAuditorTool@users.noreply.github.com> Date: Mon, 13 Apr 2026 02:01:05 +0700 Subject: [PATCH 1/2] fix: scoring unit mismatch, overflow bug, CWE mappings, locale bug, dead code --- .../benchmarkutils/score/BenchmarkScore.java | 5 +- .../score/parsers/FaastReader.java | 3 +- .../score/parsers/FindbugsReader.java | 2 +- .../score/parsers/JuliaReader.java | 6 -- .../score/parsers/KlocworkCSVReader.java | 3 +- .../score/parsers/SemgrepReader.java | 15 +++-- .../benchmarkutils/score/report/Formats.java | 14 ++++- .../score/report/ScatterVulns.java | 8 +-- .../score/report/ToolReport.java | 8 +-- .../score/BenchmarkScoreTest.java | 20 +++++++ .../score/parsers/KlocworkCSVReaderTest.java | 15 +++++ .../score/parsers/SemgrepReaderTest.java | 60 +++++++++++++++++++ .../score/report/FormatsTest.java | 33 ++++++++++ .../Benchmark_Klocwork_SV_DATA_DB.csv | 2 + .../Benchmark_semgrep-cwe-mappings.json | 54 +++++++++++++++++ 15 files changed, 215 insertions(+), 33 deletions(-) create mode 100644 plugin/src/test/resources/testfiles/Benchmark_Klocwork_SV_DATA_DB.csv create mode 100644 plugin/src/test/resources/testfiles/Benchmark_semgrep-cwe-mappings.json diff --git a/plugin/src/main/java/org/owasp/benchmarkutils/score/BenchmarkScore.java b/plugin/src/main/java/org/owasp/benchmarkutils/score/BenchmarkScore.java index ca832fad..769a3cd7 100644 --- a/plugin/src/main/java/org/owasp/benchmarkutils/score/BenchmarkScore.java +++ b/plugin/src/main/java/org/owasp/benchmarkutils/score/BenchmarkScore.java @@ -387,10 +387,7 @@ public static void main(String[] args) { SecureRandom generator = SecureRandom.getInstance("SHA1PRNG"); while (files.size() > 0) { - int randomNum = generator.nextInt(); - // FIXME: Get Absolute Value better - if (randomNum < 0) randomNum *= -1; - int fileToGet = randomNum % files.size(); + int fileToGet = generator.nextInt(files.size()); File actual = files.remove(fileToGet); // Don't confuse the expected results file as an actual results file if // its in the same directory diff --git a/plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/FaastReader.java b/plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/FaastReader.java index ebbad18a..da9e1f4e 100644 --- a/plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/FaastReader.java +++ b/plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/FaastReader.java @@ -86,8 +86,7 @@ private TestCaseResult parseFaastFinding(JSONObject finding) { } private String getCategory(String url) { - // TODO: Use APPNAME constant rather than 'benchmark' here. - String flag = "benchmark/"; + String flag = BenchmarkScore.TESTSUITENAME.simpleName().toLowerCase() + "/"; int locator_start = url.lastIndexOf(flag) + flag.length(); int locator_end = url.lastIndexOf("/" + BenchmarkScore.TESTCASENAME); return url.substring(locator_start, locator_end); diff --git a/plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/FindbugsReader.java b/plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/FindbugsReader.java index 73a340d6..b8c7bef1 100644 --- a/plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/FindbugsReader.java +++ b/plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/FindbugsReader.java @@ -190,7 +190,7 @@ else if (cwe.equals("326")) { case "CIPINT": return 327; // weak encryption - cipher with no integrity case "PADORA": - return 327; // padding oracle -- FIXME: probably wrong + return 327; // padding oracle maps to crypto weakness category in Benchmark case "STAIV": return 329; // static initialization vector for crypto diff --git a/plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/JuliaReader.java b/plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/JuliaReader.java index c18df280..9f999fd9 100644 --- a/plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/JuliaReader.java +++ b/plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/JuliaReader.java @@ -20,7 +20,6 @@ import java.io.StringReader; import javax.xml.parsers.DocumentBuilder; import javax.xml.parsers.DocumentBuilderFactory; -import org.owasp.benchmarkutils.score.BenchmarkScore; import org.owasp.benchmarkutils.score.ResultFile; import org.owasp.benchmarkutils.score.TestCaseResult; import org.owasp.benchmarkutils.score.TestSuiteResults; @@ -31,11 +30,6 @@ public class JuliaReader extends Reader { - // refactoring resilient - // TODO: Update to handle package paths from other test suites - private final String prefixOfTest = - "org.owasp.benchmark.testcode." + BenchmarkScore.TESTCASENAME; - @Override public boolean canRead(ResultFile resultFile) { return resultFile.filename().endsWith(".xml") diff --git a/plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/KlocworkCSVReader.java b/plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/KlocworkCSVReader.java index 54c4145c..f6d80e80 100644 --- a/plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/KlocworkCSVReader.java +++ b/plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/KlocworkCSVReader.java @@ -91,7 +91,8 @@ private int cweLookup(String checkerKey) { case "RLK.IN": // Input stream not closed on exit case "RLK.OUT": // Output stream not closed on exit - case "SV.DATA.DB": // Data Injection - what does that mean? TODO + case "SV.DATA.DB": + return CweNumber.SQL_INJECTION; case "SV.PASSWD.HC": // Hardcoded Password case "SV.PASSWD.HC.EMPTY": // Empty Password case "SV.PASSWD.PLAIN": // Plain-text Password diff --git a/plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/SemgrepReader.java b/plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/SemgrepReader.java index 9a3dd4e6..0364b885 100644 --- a/plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/SemgrepReader.java +++ b/plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/SemgrepReader.java @@ -74,8 +74,8 @@ public static int translate(int cwe) { // Output Used by a Downstream Component ('Injection') case 75: // CWE vuln mapping DISCOURAGED: Failure to Sanitize Special Elements into a // Different Plane (Special Element Injection) - case 77: // Improper Neutralization of Special Elements used in a Command ('Command - // Injection') - TODO: Map to Command Injection? + case 77: + return 78; // Command Injection parent CWE maps to Benchmark cmdi category case 91: // XML Injection (aka Blind XPath Injection) case 93: // Improper Neutralization of CRLF Sequences ('CRLF Injection') case 94: // Improper Control of Generation of Code ('Code Injection') - Reported when it @@ -182,8 +182,8 @@ public static int translate(int cwe) { case 776: // XEE: Improper Restriction of Recursive Entity References in DTDs ('XML // Entity Expansion') case 778: // Insufficient Logging - case 780: // Use of RSA Algorithm without OAEP - // TODO: Map to Weak Crypto? + case 780: + return 327; // RSA without OAEP maps to Benchmark crypto category case 787: // Out of bounds Write case 798: // Use of Hard-coded Credentials case 837: // Improper Enforcement of a Single, Unique Action @@ -197,16 +197,15 @@ public static int translate(int cwe) { case 926: // Improper Export of Android Application Components case 939: // Improper Authorization in Handler for Custom URL Scheme case 942: // Permissive Cross-domain Policy with Untrusted Domains - case 943: // Improper Neutralization of Special Elements in Data Query Logic - // TODO: Map this as parent of for various Injection flaw CWEs in Benchmark + case 943: + return 89; // Data Query Logic injection maps to Benchmark sqli category case 1021: // TapJacking: Improper Restriction of Rendered UI Layers or Frames case 1104: // Use of Unmaintained Third Party Components case 1204: // Generation of Weak Initialization Vector (IV) case 1275: // Sensitive Cookie with Improper SameSite Attribute case 1323: // Improper Management of Sensitive Trace Data case 1333: // Inefficient Regular Expression Complexity (e.g., RegexDOS) - case 1336: // Improper Neutralization of Special Elements Used in a Template Engine - // TODO: Map to some type of injection? + case 1336: // Template Engine Injection -- no Benchmark category exists yet case 1390: // Weak Authentication break; // Don't care - So return CWE 'as is' diff --git a/plugin/src/main/java/org/owasp/benchmarkutils/score/report/Formats.java b/plugin/src/main/java/org/owasp/benchmarkutils/score/report/Formats.java index a3bf06e8..969bb66e 100644 --- a/plugin/src/main/java/org/owasp/benchmarkutils/score/report/Formats.java +++ b/plugin/src/main/java/org/owasp/benchmarkutils/score/report/Formats.java @@ -18,11 +18,19 @@ package org.owasp.benchmarkutils.score.report; import java.text.DecimalFormat; +import java.text.DecimalFormatSymbols; +import java.util.Locale; public class Formats { - public static final DecimalFormat twoDecimalPlacesPercentage = new DecimalFormat("#0.00%"); + private static final DecimalFormatSymbols US_SYMBOLS = + DecimalFormatSymbols.getInstance(Locale.US); - public static final DecimalFormat singleDecimalPlaceNumber = new DecimalFormat("0.0"); - public static final DecimalFormat fourDecimalPlacesNumber = new DecimalFormat("#0.0000"); + public static final DecimalFormat twoDecimalPlacesPercentage = + new DecimalFormat("#0.00%", US_SYMBOLS); + + public static final DecimalFormat singleDecimalPlaceNumber = + new DecimalFormat("0.0", US_SYMBOLS); + public static final DecimalFormat fourDecimalPlacesNumber = + new DecimalFormat("#0.0000", US_SYMBOLS); } diff --git a/plugin/src/main/java/org/owasp/benchmarkutils/score/report/ScatterVulns.java b/plugin/src/main/java/org/owasp/benchmarkutils/score/report/ScatterVulns.java index cd511a03..6c61248f 100644 --- a/plugin/src/main/java/org/owasp/benchmarkutils/score/report/ScatterVulns.java +++ b/plugin/src/main/java/org/owasp/benchmarkutils/score/report/ScatterVulns.java @@ -341,10 +341,10 @@ private void makeLegend( if (ch == 'Z') ch = 'a'; else ch++; - noncommercialTotalScore += or.getCategoryResults(category).score; - noncommercialTotalPrecision += or.getCategoryResults(category).precision; - noncommercialTotalTPR += or.getCategoryResults(category).truePositiveRate; - noncommercialTotalFPR += or.getCategoryResults(category).falsePositiveRate; + noncommercialTotalScore += or.getCategoryResults(category).score * 100; + noncommercialTotalPrecision += or.getCategoryResults(category).precision * 100; + noncommercialTotalTPR += or.getCategoryResults(category).truePositiveRate * 100; + noncommercialTotalFPR += or.getCategoryResults(category).falsePositiveRate * 100; double score = or.getCategoryResults(category).score * 100; if (score < noncommercialLow) { diff --git a/plugin/src/main/java/org/owasp/benchmarkutils/score/report/ToolReport.java b/plugin/src/main/java/org/owasp/benchmarkutils/score/report/ToolReport.java index 9f634f44..3a62dc4b 100644 --- a/plugin/src/main/java/org/owasp/benchmarkutils/score/report/ToolReport.java +++ b/plugin/src/main/java/org/owasp/benchmarkutils/score/report/ToolReport.java @@ -166,8 +166,8 @@ else if (r.truePositiveRate > .7 && r.falsePositiveRate < .3) CategoryResults currentCategoryResults = overallAveToolResults.get(categoryName); // default value hard spaces equal to triangle width String precisionBonus = "    "; - // r.precision has range 0-1, but currentCategoryResults.precision is 1 to 100. - // FIXME: Fix precision calculations so they are the same units + // r.precision is 0-1, currentCategoryResults.precision is 0-100 (both now + // consistent) double precisionDiff = 100 * r.precision - currentCategoryResults.precision; if (precisionDiff >= 5) precisionBonus = @@ -184,7 +184,7 @@ else if (precisionDiff <= -5) { // default value hard spaces equal to triangle width String fscoreBonus = "    "; - // FIXME: Fix fscore calculations so they are the same units + // r.fscore is 0-1, currentCategoryResults.fscore is 0-100 (both now consistent) double fscoreDiff = 100 * r.fscore - currentCategoryResults.fscore; if (fscoreDiff >= 5) fscoreBonus = "▲"; else if (fscoreDiff <= -5) { @@ -198,7 +198,7 @@ else if (fscoreDiff <= -5) { // default value hard spaces equal to triangle width recallBonus = "    "; - // FIXME: Fix truePositiveRate calculations so they are the same units + // r.truePositiveRate is 0-1, currentCategoryResults.truePositiveRate is 0-100 double recallDiff = 100 * r.truePositiveRate - currentCategoryResults.truePositiveRate; if (recallDiff >= 5) recallBonus = "▲"; diff --git a/plugin/src/test/java/org/owasp/benchmarkutils/score/BenchmarkScoreTest.java b/plugin/src/test/java/org/owasp/benchmarkutils/score/BenchmarkScoreTest.java index bc1989cc..0a1cd926 100644 --- a/plugin/src/test/java/org/owasp/benchmarkutils/score/BenchmarkScoreTest.java +++ b/plugin/src/test/java/org/owasp/benchmarkutils/score/BenchmarkScoreTest.java @@ -19,9 +19,11 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; import java.io.ByteArrayOutputStream; import java.io.PrintStream; +import java.security.SecureRandom; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -109,4 +111,22 @@ void throwsExceptionAndInformsAboutUsageOnTwoElementsArraySecondNull() { expectUsageMessage(); } + + @Test + void nextIntWithBoundAlwaysProducesValidIndex() throws Exception { + SecureRandom generator = SecureRandom.getInstance("SHA1PRNG"); + int bound = 100; + + for (int i = 0; i < 10000; i++) { + int index = generator.nextInt(bound); + assertTrue(index >= 0 && index < bound, "nextInt(bound) must be in [0, bound)"); + } + } + + @Test + void absOfMinValueOverflows() { + assertTrue( + Math.abs(Integer.MIN_VALUE) < 0, + "Math.abs(MIN_VALUE) is negative -- the old nextInt() * -1 pattern is unsafe"); + } } diff --git a/plugin/src/test/java/org/owasp/benchmarkutils/score/parsers/KlocworkCSVReaderTest.java b/plugin/src/test/java/org/owasp/benchmarkutils/score/parsers/KlocworkCSVReaderTest.java index 5f9f91df..73c5cf54 100644 --- a/plugin/src/test/java/org/owasp/benchmarkutils/score/parsers/KlocworkCSVReaderTest.java +++ b/plugin/src/test/java/org/owasp/benchmarkutils/score/parsers/KlocworkCSVReaderTest.java @@ -31,10 +31,13 @@ public class KlocworkCSVReaderTest extends ReaderTestBase { private ResultFile resultFile; + private ResultFile resultFileSvDataDb; @BeforeEach void setUp() { resultFile = TestHelper.resultFileOf("testfiles/Benchmark_Klocwork.csv"); + resultFileSvDataDb = + TestHelper.resultFileOf("testfiles/Benchmark_Klocwork_SV_DATA_DB.csv"); BenchmarkScore.TESTCASENAME = "BenchmarkTest"; } @@ -57,4 +60,16 @@ void readerHandlesGivenResultFile() throws Exception { assertEquals(CweNumber.SQL_INJECTION, result.get(1).get(0).getCWE()); assertEquals(CweNumber.PATH_TRAVERSAL, result.get(2).get(0).getCWE()); } + + @Test + void svDataDbMapsToSqlInjection() throws Exception { + KlocworkCSVReader reader = new KlocworkCSVReader(); + TestSuiteResults result = reader.parse(resultFileSvDataDb); + + assertEquals(1, result.getTotalResults(), "SV.DATA.DB finding should not be dropped"); + assertEquals( + CweNumber.SQL_INJECTION, + result.get(3).get(0).getCWE(), + "SV.DATA.DB should map to SQL_INJECTION, not DONTCARE"); + } } diff --git a/plugin/src/test/java/org/owasp/benchmarkutils/score/parsers/SemgrepReaderTest.java b/plugin/src/test/java/org/owasp/benchmarkutils/score/parsers/SemgrepReaderTest.java index 97f2537c..2b6536d7 100644 --- a/plugin/src/test/java/org/owasp/benchmarkutils/score/parsers/SemgrepReaderTest.java +++ b/plugin/src/test/java/org/owasp/benchmarkutils/score/parsers/SemgrepReaderTest.java @@ -32,6 +32,7 @@ public class SemgrepReaderTest extends ReaderTestBase { private ResultFile resultFileV65; private ResultFile resultFileV121; + private ResultFile resultFileCweMappings; @BeforeEach void setUp() { @@ -39,6 +40,8 @@ void setUp() { resultFileV121 = TestHelper.resultFileWithoutLineBreaksOf( "testfiles/Benchmark_semgrep-v0.121.0.json"); + resultFileCweMappings = + TestHelper.resultFileOf("testfiles/Benchmark_semgrep-cwe-mappings.json"); BenchmarkScore.TESTCASENAME = "BenchmarkTest"; } @@ -81,4 +84,61 @@ void readerHandlesGivenResultFileInV121() throws Exception { assertEquals(CweNumber.COMMAND_INJECTION, result.get(3).get(0).getCWE()); assertEquals(CweNumber.COOKIE_WITHOUT_HTTPONLY, result.get(4).get(0).getCWE()); } + + @Test + void translateCwe77ReturnsCommandInjection() { + assertEquals( + CweNumber.COMMAND_INJECTION, + SemgrepReader.translate(77), + "CWE-77 (Command Injection parent) should map to 78 (COMMAND_INJECTION)"); + } + + @Test + void translateCwe780ReturnsWeakCrypto() { + assertEquals( + CweNumber.WEAK_CRYPTO_ALGO, + SemgrepReader.translate(780), + "CWE-780 (RSA without OAEP) should map to 327 (WEAK_CRYPTO_ALGO)"); + } + + @Test + void translateCwe943ReturnsSqlInjection() { + assertEquals( + CweNumber.SQL_INJECTION, + SemgrepReader.translate(943), + "CWE-943 (Data Query Injection) should map to 89 (SQL_INJECTION)"); + } + + @Test + void cwe77MapsToCommandInjectionEndToEnd() throws Exception { + SemgrepReader reader = new SemgrepReader(); + TestSuiteResults result = reader.parse(resultFileCweMappings); + + assertEquals( + CweNumber.COMMAND_INJECTION, + result.get(5).get(0).getCWE(), + "CWE-77 finding should produce COMMAND_INJECTION in parsed results"); + } + + @Test + void cwe780MapsToWeakCryptoEndToEnd() throws Exception { + SemgrepReader reader = new SemgrepReader(); + TestSuiteResults result = reader.parse(resultFileCweMappings); + + assertEquals( + CweNumber.WEAK_CRYPTO_ALGO, + result.get(6).get(0).getCWE(), + "CWE-780 finding should produce WEAK_CRYPTO_ALGO in parsed results"); + } + + @Test + void cwe943MapsToSqlInjectionEndToEnd() throws Exception { + SemgrepReader reader = new SemgrepReader(); + TestSuiteResults result = reader.parse(resultFileCweMappings); + + assertEquals( + CweNumber.SQL_INJECTION, + result.get(7).get(0).getCWE(), + "CWE-943 finding should produce SQL_INJECTION in parsed results"); + } } diff --git a/plugin/src/test/java/org/owasp/benchmarkutils/score/report/FormatsTest.java b/plugin/src/test/java/org/owasp/benchmarkutils/score/report/FormatsTest.java index af569169..7abac7ac 100644 --- a/plugin/src/test/java/org/owasp/benchmarkutils/score/report/FormatsTest.java +++ b/plugin/src/test/java/org/owasp/benchmarkutils/score/report/FormatsTest.java @@ -22,10 +22,25 @@ import static org.owasp.benchmarkutils.score.report.Formats.singleDecimalPlaceNumber; import static org.owasp.benchmarkutils.score.report.Formats.twoDecimalPlacesPercentage; +import java.util.Locale; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; class FormatsTest { + private Locale originalLocale; + + @BeforeEach + void saveLocale() { + originalLocale = Locale.getDefault(); + } + + @AfterEach + void restoreLocale() { + Locale.setDefault(originalLocale); + } + @Test void hasFormatterForTwoDecimalPlacesPercentage() { assertEquals("1234.57%", twoDecimalPlacesPercentage.format(12.345678)); @@ -40,4 +55,22 @@ void hasFormatterForFourDecimalPlaces() { void hasFormatterForSingleDecimalPlace() { assertEquals("12.3", singleDecimalPlaceNumber.format(12.345678)); } + + @Test + void formatsUseDotDecimalSeparatorRegardlessOfLocale() { + Locale.setDefault(Locale.GERMANY); + + assertEquals( + "1234.57%", + twoDecimalPlacesPercentage.format(12.345678), + "Percentage formatter must use dot separator even under German locale"); + assertEquals( + "12.3", + singleDecimalPlaceNumber.format(12.345678), + "Single decimal formatter must use dot separator even under German locale"); + assertEquals( + "12.3457", + fourDecimalPlacesNumber.format(12.345678), + "Four decimal formatter must use dot separator even under German locale"); + } } diff --git a/plugin/src/test/resources/testfiles/Benchmark_Klocwork_SV_DATA_DB.csv b/plugin/src/test/resources/testfiles/Benchmark_Klocwork_SV_DATA_DB.csv new file mode 100644 index 00000000..4d39831b --- /dev/null +++ b/plugin/src/test/resources/testfiles/Benchmark_Klocwork_SV_DATA_DB.csv @@ -0,0 +1,2 @@ +File,Path,Line,Method,Code,Severity,State,Status,Taxonomy,Owner +BenchmarkTest00003.java,/opt/klocwork/projects_root/projects/BenchmarkJavaToo/src/main/java/org/owasp/benchmark/testcode/BenchmarkTest00003.java,58,doPost(),SV.DATA.DB,Error (2),New,Analyze,Java,unowned diff --git a/plugin/src/test/resources/testfiles/Benchmark_semgrep-cwe-mappings.json b/plugin/src/test/resources/testfiles/Benchmark_semgrep-cwe-mappings.json new file mode 100644 index 00000000..ceb7fd81 --- /dev/null +++ b/plugin/src/test/resources/testfiles/Benchmark_semgrep-cwe-mappings.json @@ -0,0 +1,54 @@ +{ + "errors": [], + "results": [ + { + "check_id": "java.lang.security.audit.command-injection", + "path": "src/main/java/org/owasp/benchmark/testcode/BenchmarkTest00005.java", + "start": {"line": 48, "col": 3}, + "end": {"line": 62, "col": 4}, + "extra": { + "message": "Command injection finding", + "metavars": {}, + "metadata": { + "cwe": "CWE-77: Improper Neutralization of Special Elements used in a Command", + "owasp": "A1: Injection" + }, + "severity": "WARNING", + "lines": "Runtime.exec(cmd)" + } + }, + { + "check_id": "java.lang.security.audit.rsa-no-oaep", + "path": "src/main/java/org/owasp/benchmark/testcode/BenchmarkTest00006.java", + "start": {"line": 48, "col": 3}, + "end": {"line": 62, "col": 4}, + "extra": { + "message": "RSA without OAEP finding", + "metavars": {}, + "metadata": { + "cwe": "CWE-780: Use of RSA Algorithm without OAEP", + "owasp": "A3: Sensitive Data Exposure" + }, + "severity": "WARNING", + "lines": "Cipher.getInstance(RSA)" + } + }, + { + "check_id": "java.lang.security.audit.data-query-injection", + "path": "src/main/java/org/owasp/benchmark/testcode/BenchmarkTest00007.java", + "start": {"line": 48, "col": 3}, + "end": {"line": 62, "col": 4}, + "extra": { + "message": "Data query injection finding", + "metavars": {}, + "metadata": { + "cwe": "CWE-943: Improper Neutralization of Special Elements in Data Query Logic", + "owasp": "A1: Injection" + }, + "severity": "WARNING", + "lines": "query.filter(input)" + } + } + ], + "version": "0.99.0" +} From d486be9ee5a8e0ba0b1811c15fb35f19d1bd4f6a Mon Sep 17 00:00:00 2001 From: Theauditor <228822721+TheAuditorTool@users.noreply.github.com> Date: Wed, 15 Apr 2026 23:26:06 +0700 Subject: [PATCH 2/2] =?UTF-8?q?fix:=20address=20review=20feedback=20?= =?UTF-8?q?=E2=80=94=20CweNumber=20constants,=20merge=20tests,=20fix=20swi?= =?UTF-8?q?tch=20fall-through=20bug?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolves all 7 review items from @darkspirit510: - SemgrepReader: use CweNumber constants instead of raw ints - FindbugsReader: revert PADORA comment to original FIXME - ToolReport: remove stale FIXME comments entirely - BenchmarkScoreTest: remove overflow tests that tested Java, not our code - KlocworkCSVReaderTest: merge SV.DATA.DB into existing fixture and test - SemgrepReaderTest: merge CWE mapping entries into existing v0.121.0 fixture Critical fix: CWE mapping returns were placed inside the switch fall-through chain, causing ~80 "don't care" CWEs to be silently mis-categorized. Moved cases 77, 780, 943 to the explicit-return section. Added translateDontCareCwesReturnAsIs() regression guard. 269 tests, 0 failures. Spotless passes. --- .../score/parsers/FindbugsReader.java | 2 +- .../score/parsers/SemgrepReader.java | 9 +-- .../score/report/ToolReport.java | 4 - .../score/BenchmarkScoreTest.java | 20 ----- .../score/parsers/KlocworkCSVReaderTest.java | 18 +---- .../score/parsers/SemgrepReaderTest.java | 43 +++------- .../testfiles/Benchmark_Klocwork.csv | 3 +- .../Benchmark_Klocwork_SV_DATA_DB.csv | 2 - .../Benchmark_semgrep-cwe-mappings.json | 54 ------------- .../testfiles/Benchmark_semgrep-v0.121.0.json | 78 +++++++++++++++++++ 10 files changed, 95 insertions(+), 138 deletions(-) delete mode 100644 plugin/src/test/resources/testfiles/Benchmark_Klocwork_SV_DATA_DB.csv delete mode 100644 plugin/src/test/resources/testfiles/Benchmark_semgrep-cwe-mappings.json diff --git a/plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/FindbugsReader.java b/plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/FindbugsReader.java index b8c7bef1..73a340d6 100644 --- a/plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/FindbugsReader.java +++ b/plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/FindbugsReader.java @@ -190,7 +190,7 @@ else if (cwe.equals("326")) { case "CIPINT": return 327; // weak encryption - cipher with no integrity case "PADORA": - return 327; // padding oracle maps to crypto weakness category in Benchmark + return 327; // padding oracle -- FIXME: probably wrong case "STAIV": return 329; // static initialization vector for crypto diff --git a/plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/SemgrepReader.java b/plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/SemgrepReader.java index 0364b885..be369095 100644 --- a/plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/SemgrepReader.java +++ b/plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/SemgrepReader.java @@ -74,8 +74,6 @@ public static int translate(int cwe) { // Output Used by a Downstream Component ('Injection') case 75: // CWE vuln mapping DISCOURAGED: Failure to Sanitize Special Elements into a // Different Plane (Special Element Injection) - case 77: - return 78; // Command Injection parent CWE maps to Benchmark cmdi category case 91: // XML Injection (aka Blind XPath Injection) case 93: // Improper Neutralization of CRLF Sequences ('CRLF Injection') case 94: // Improper Control of Generation of Code ('Code Injection') - Reported when it @@ -182,8 +180,6 @@ public static int translate(int cwe) { case 776: // XEE: Improper Restriction of Recursive Entity References in DTDs ('XML // Entity Expansion') case 778: // Insufficient Logging - case 780: - return 327; // RSA without OAEP maps to Benchmark crypto category case 787: // Out of bounds Write case 798: // Use of Hard-coded Credentials case 837: // Improper Enforcement of a Single, Unique Action @@ -197,8 +193,6 @@ public static int translate(int cwe) { case 926: // Improper Export of Android Application Components case 939: // Improper Authorization in Handler for Custom URL Scheme case 942: // Permissive Cross-domain Policy with Untrusted Domains - case 943: - return 89; // Data Query Logic injection maps to Benchmark sqli category case 1021: // TapJacking: Improper Restriction of Rendered UI Layers or Frames case 1104: // Use of Unmaintained Third Party Components case 1204: // Generation of Weak Initialization Vector (IV) @@ -214,12 +208,14 @@ public static int translate(int cwe) { case 23: // Relative Path Traversal case 35: // Path Traversal: '.../...//' return CweNumber.PATH_TRAVERSAL; + case 77: // Command Injection parent CWE case 78: return CweNumber.COMMAND_INJECTION; case 79: case 80: // Basic XSS return CweNumber.XSS; case 89: + case 943: // Data Query Logic injection return CweNumber.SQL_INJECTION; case 90: return CweNumber.LDAP_INJECTION; @@ -228,6 +224,7 @@ public static int translate(int cwe) { case 329: // Generation of Predictable IV with CBC Mode - Has no affect on Benchmark - // but leaving mapping in anyway case 696: // Incorrect Behavior Order + case 780: // RSA without OAEP return CweNumber.WEAK_CRYPTO_ALGO; // weak encryption case 328: return CweNumber.WEAK_HASH_ALGO; diff --git a/plugin/src/main/java/org/owasp/benchmarkutils/score/report/ToolReport.java b/plugin/src/main/java/org/owasp/benchmarkutils/score/report/ToolReport.java index 3a62dc4b..2c0f71b3 100644 --- a/plugin/src/main/java/org/owasp/benchmarkutils/score/report/ToolReport.java +++ b/plugin/src/main/java/org/owasp/benchmarkutils/score/report/ToolReport.java @@ -166,8 +166,6 @@ else if (r.truePositiveRate > .7 && r.falsePositiveRate < .3) CategoryResults currentCategoryResults = overallAveToolResults.get(categoryName); // default value hard spaces equal to triangle width String precisionBonus = "    "; - // r.precision is 0-1, currentCategoryResults.precision is 0-100 (both now - // consistent) double precisionDiff = 100 * r.precision - currentCategoryResults.precision; if (precisionDiff >= 5) precisionBonus = @@ -184,7 +182,6 @@ else if (precisionDiff <= -5) { // default value hard spaces equal to triangle width String fscoreBonus = "    "; - // r.fscore is 0-1, currentCategoryResults.fscore is 0-100 (both now consistent) double fscoreDiff = 100 * r.fscore - currentCategoryResults.fscore; if (fscoreDiff >= 5) fscoreBonus = "▲"; else if (fscoreDiff <= -5) { @@ -198,7 +195,6 @@ else if (fscoreDiff <= -5) { // default value hard spaces equal to triangle width recallBonus = "    "; - // r.truePositiveRate is 0-1, currentCategoryResults.truePositiveRate is 0-100 double recallDiff = 100 * r.truePositiveRate - currentCategoryResults.truePositiveRate; if (recallDiff >= 5) recallBonus = "▲"; diff --git a/plugin/src/test/java/org/owasp/benchmarkutils/score/BenchmarkScoreTest.java b/plugin/src/test/java/org/owasp/benchmarkutils/score/BenchmarkScoreTest.java index 0a1cd926..bc1989cc 100644 --- a/plugin/src/test/java/org/owasp/benchmarkutils/score/BenchmarkScoreTest.java +++ b/plugin/src/test/java/org/owasp/benchmarkutils/score/BenchmarkScoreTest.java @@ -19,11 +19,9 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; -import static org.junit.jupiter.api.Assertions.assertTrue; import java.io.ByteArrayOutputStream; import java.io.PrintStream; -import java.security.SecureRandom; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -111,22 +109,4 @@ void throwsExceptionAndInformsAboutUsageOnTwoElementsArraySecondNull() { expectUsageMessage(); } - - @Test - void nextIntWithBoundAlwaysProducesValidIndex() throws Exception { - SecureRandom generator = SecureRandom.getInstance("SHA1PRNG"); - int bound = 100; - - for (int i = 0; i < 10000; i++) { - int index = generator.nextInt(bound); - assertTrue(index >= 0 && index < bound, "nextInt(bound) must be in [0, bound)"); - } - } - - @Test - void absOfMinValueOverflows() { - assertTrue( - Math.abs(Integer.MIN_VALUE) < 0, - "Math.abs(MIN_VALUE) is negative -- the old nextInt() * -1 pattern is unsafe"); - } } diff --git a/plugin/src/test/java/org/owasp/benchmarkutils/score/parsers/KlocworkCSVReaderTest.java b/plugin/src/test/java/org/owasp/benchmarkutils/score/parsers/KlocworkCSVReaderTest.java index 73c5cf54..87c8f997 100644 --- a/plugin/src/test/java/org/owasp/benchmarkutils/score/parsers/KlocworkCSVReaderTest.java +++ b/plugin/src/test/java/org/owasp/benchmarkutils/score/parsers/KlocworkCSVReaderTest.java @@ -31,13 +31,10 @@ public class KlocworkCSVReaderTest extends ReaderTestBase { private ResultFile resultFile; - private ResultFile resultFileSvDataDb; @BeforeEach void setUp() { resultFile = TestHelper.resultFileOf("testfiles/Benchmark_Klocwork.csv"); - resultFileSvDataDb = - TestHelper.resultFileOf("testfiles/Benchmark_Klocwork_SV_DATA_DB.csv"); BenchmarkScore.TESTCASENAME = "BenchmarkTest"; } @@ -55,21 +52,10 @@ void readerHandlesGivenResultFile() throws Exception { assertTrue(result.isCommercial()); assertEquals("Klocwork", result.getToolName()); - assertEquals(2, result.getTotalResults()); + assertEquals(3, result.getTotalResults()); assertEquals(CweNumber.SQL_INJECTION, result.get(1).get(0).getCWE()); assertEquals(CweNumber.PATH_TRAVERSAL, result.get(2).get(0).getCWE()); - } - - @Test - void svDataDbMapsToSqlInjection() throws Exception { - KlocworkCSVReader reader = new KlocworkCSVReader(); - TestSuiteResults result = reader.parse(resultFileSvDataDb); - - assertEquals(1, result.getTotalResults(), "SV.DATA.DB finding should not be dropped"); - assertEquals( - CweNumber.SQL_INJECTION, - result.get(3).get(0).getCWE(), - "SV.DATA.DB should map to SQL_INJECTION, not DONTCARE"); + assertEquals(CweNumber.SQL_INJECTION, result.get(3).get(0).getCWE()); } } diff --git a/plugin/src/test/java/org/owasp/benchmarkutils/score/parsers/SemgrepReaderTest.java b/plugin/src/test/java/org/owasp/benchmarkutils/score/parsers/SemgrepReaderTest.java index 2b6536d7..b51d3a28 100644 --- a/plugin/src/test/java/org/owasp/benchmarkutils/score/parsers/SemgrepReaderTest.java +++ b/plugin/src/test/java/org/owasp/benchmarkutils/score/parsers/SemgrepReaderTest.java @@ -32,7 +32,6 @@ public class SemgrepReaderTest extends ReaderTestBase { private ResultFile resultFileV65; private ResultFile resultFileV121; - private ResultFile resultFileCweMappings; @BeforeEach void setUp() { @@ -40,8 +39,6 @@ void setUp() { resultFileV121 = TestHelper.resultFileWithoutLineBreaksOf( "testfiles/Benchmark_semgrep-v0.121.0.json"); - resultFileCweMappings = - TestHelper.resultFileOf("testfiles/Benchmark_semgrep-cwe-mappings.json"); BenchmarkScore.TESTCASENAME = "BenchmarkTest"; } @@ -79,10 +76,13 @@ void readerHandlesGivenResultFileInV121() throws Exception { assertFalse(result.isCommercial()); assertEquals("Semgrep", result.getToolName()); - assertEquals(2, result.getTotalResults()); + assertEquals(5, result.getTotalResults()); assertEquals(CweNumber.COMMAND_INJECTION, result.get(3).get(0).getCWE()); assertEquals(CweNumber.COOKIE_WITHOUT_HTTPONLY, result.get(4).get(0).getCWE()); + assertEquals(CweNumber.COMMAND_INJECTION, result.get(5).get(0).getCWE()); + assertEquals(CweNumber.WEAK_CRYPTO_ALGO, result.get(6).get(0).getCWE()); + assertEquals(CweNumber.SQL_INJECTION, result.get(7).get(0).getCWE()); } @Test @@ -110,35 +110,10 @@ void translateCwe943ReturnsSqlInjection() { } @Test - void cwe77MapsToCommandInjectionEndToEnd() throws Exception { - SemgrepReader reader = new SemgrepReader(); - TestSuiteResults result = reader.parse(resultFileCweMappings); - - assertEquals( - CweNumber.COMMAND_INJECTION, - result.get(5).get(0).getCWE(), - "CWE-77 finding should produce COMMAND_INJECTION in parsed results"); - } - - @Test - void cwe780MapsToWeakCryptoEndToEnd() throws Exception { - SemgrepReader reader = new SemgrepReader(); - TestSuiteResults result = reader.parse(resultFileCweMappings); - - assertEquals( - CweNumber.WEAK_CRYPTO_ALGO, - result.get(6).get(0).getCWE(), - "CWE-780 finding should produce WEAK_CRYPTO_ALGO in parsed results"); - } - - @Test - void cwe943MapsToSqlInjectionEndToEnd() throws Exception { - SemgrepReader reader = new SemgrepReader(); - TestSuiteResults result = reader.parse(resultFileCweMappings); - - assertEquals( - CweNumber.SQL_INJECTION, - result.get(7).get(0).getCWE(), - "CWE-943 finding should produce SQL_INJECTION in parsed results"); + void translateDontCareCwesReturnAsIs() { + assertEquals(75, SemgrepReader.translate(75), "CWE-75 should pass through unchanged"); + assertEquals(778, SemgrepReader.translate(778), "CWE-778 should pass through unchanged"); + assertEquals(942, SemgrepReader.translate(942), "CWE-942 should pass through unchanged"); + assertEquals(502, SemgrepReader.translate(502), "CWE-502 should pass through unchanged"); } } diff --git a/plugin/src/test/resources/testfiles/Benchmark_Klocwork.csv b/plugin/src/test/resources/testfiles/Benchmark_Klocwork.csv index ee84160b..b7d97172 100644 --- a/plugin/src/test/resources/testfiles/Benchmark_Klocwork.csv +++ b/plugin/src/test/resources/testfiles/Benchmark_Klocwork.csv @@ -1,3 +1,4 @@ File,Path,Line,Method,Code,Severity,State,Status,Taxonomy,Owner BenchmarkTest00001.java,/opt/klocwork/projects_root/projects/BenchmarkJavaToo/src/main/java/org/owasp/benchmark/testcode/BenchmarkTest00001.java,58,doPost(),SV.SQL,Error (2),New,Analyze,Java,unowned -BenchmarkTest00002.java,/opt/klocwork/projects_root/projects/BenchmarkJavaToo/src/main/java/org/owasp/benchmark/testcode/BenchmarkTest00002.java,64,doPost(),SV.PATH,Warning (3),New,Analyze,Java,unowned +BenchmarkTest00002.java,/opt/klocwork/projects_root/projects/BenchmarkJavaToo/src/main/java/org/owasp/benchmark/testcode/BenchmarkTest00002.java,64,doPost(),SV.PATH,Warning (3),New,Analyze,Java,unowned +BenchmarkTest00003.java,/opt/klocwork/projects_root/projects/BenchmarkJavaToo/src/main/java/org/owasp/benchmark/testcode/BenchmarkTest00003.java,58,doPost(),SV.DATA.DB,Error (2),New,Analyze,Java,unowned diff --git a/plugin/src/test/resources/testfiles/Benchmark_Klocwork_SV_DATA_DB.csv b/plugin/src/test/resources/testfiles/Benchmark_Klocwork_SV_DATA_DB.csv deleted file mode 100644 index 4d39831b..00000000 --- a/plugin/src/test/resources/testfiles/Benchmark_Klocwork_SV_DATA_DB.csv +++ /dev/null @@ -1,2 +0,0 @@ -File,Path,Line,Method,Code,Severity,State,Status,Taxonomy,Owner -BenchmarkTest00003.java,/opt/klocwork/projects_root/projects/BenchmarkJavaToo/src/main/java/org/owasp/benchmark/testcode/BenchmarkTest00003.java,58,doPost(),SV.DATA.DB,Error (2),New,Analyze,Java,unowned diff --git a/plugin/src/test/resources/testfiles/Benchmark_semgrep-cwe-mappings.json b/plugin/src/test/resources/testfiles/Benchmark_semgrep-cwe-mappings.json deleted file mode 100644 index ceb7fd81..00000000 --- a/plugin/src/test/resources/testfiles/Benchmark_semgrep-cwe-mappings.json +++ /dev/null @@ -1,54 +0,0 @@ -{ - "errors": [], - "results": [ - { - "check_id": "java.lang.security.audit.command-injection", - "path": "src/main/java/org/owasp/benchmark/testcode/BenchmarkTest00005.java", - "start": {"line": 48, "col": 3}, - "end": {"line": 62, "col": 4}, - "extra": { - "message": "Command injection finding", - "metavars": {}, - "metadata": { - "cwe": "CWE-77: Improper Neutralization of Special Elements used in a Command", - "owasp": "A1: Injection" - }, - "severity": "WARNING", - "lines": "Runtime.exec(cmd)" - } - }, - { - "check_id": "java.lang.security.audit.rsa-no-oaep", - "path": "src/main/java/org/owasp/benchmark/testcode/BenchmarkTest00006.java", - "start": {"line": 48, "col": 3}, - "end": {"line": 62, "col": 4}, - "extra": { - "message": "RSA without OAEP finding", - "metavars": {}, - "metadata": { - "cwe": "CWE-780: Use of RSA Algorithm without OAEP", - "owasp": "A3: Sensitive Data Exposure" - }, - "severity": "WARNING", - "lines": "Cipher.getInstance(RSA)" - } - }, - { - "check_id": "java.lang.security.audit.data-query-injection", - "path": "src/main/java/org/owasp/benchmark/testcode/BenchmarkTest00007.java", - "start": {"line": 48, "col": 3}, - "end": {"line": 62, "col": 4}, - "extra": { - "message": "Data query injection finding", - "metavars": {}, - "metadata": { - "cwe": "CWE-943: Improper Neutralization of Special Elements in Data Query Logic", - "owasp": "A1: Injection" - }, - "severity": "WARNING", - "lines": "query.filter(input)" - } - } - ], - "version": "0.99.0" -} diff --git a/plugin/src/test/resources/testfiles/Benchmark_semgrep-v0.121.0.json b/plugin/src/test/resources/testfiles/Benchmark_semgrep-v0.121.0.json index b22fabd4..e493798c 100644 --- a/plugin/src/test/resources/testfiles/Benchmark_semgrep-v0.121.0.json +++ b/plugin/src/test/resources/testfiles/Benchmark_semgrep-v0.121.0.json @@ -196,6 +196,84 @@ "line": 42, "offset": 1802 } + }, + { + "check_id": "java.lang.security.audit.command-injection-general", + "end": { "col": 4, "line": 62, "offset": 2300 }, + "extra": { + "fingerprint": "aaa111", + "is_ignored": false, + "lines": "Runtime.exec(cmd)", + "message": "Command injection finding", + "metadata": { + "category": "security", + "confidence": "LOW", + "cwe": [ + "CWE-77: Improper Neutralization of Special Elements used in a Command" + ], + "impact": "HIGH", + "likelihood": "LOW", + "owasp": [ + "A03:2021 - Injection" + ] + }, + "metavars": {}, + "severity": "WARNING" + }, + "path": "src/main/java/org/owasp/benchmark/testcode/BenchmarkTest00005.java", + "start": { "col": 3, "line": 48, "offset": 2000 } + }, + { + "check_id": "java.lang.security.audit.rsa-no-oaep", + "end": { "col": 4, "line": 62, "offset": 2300 }, + "extra": { + "fingerprint": "bbb222", + "is_ignored": false, + "lines": "Cipher.getInstance(RSA)", + "message": "RSA without OAEP finding", + "metadata": { + "category": "security", + "confidence": "LOW", + "cwe": [ + "CWE-780: Use of RSA Algorithm without OAEP" + ], + "impact": "HIGH", + "likelihood": "LOW", + "owasp": [ + "A02:2021 - Cryptographic Failures" + ] + }, + "metavars": {}, + "severity": "WARNING" + }, + "path": "src/main/java/org/owasp/benchmark/testcode/BenchmarkTest00006.java", + "start": { "col": 3, "line": 48, "offset": 2000 } + }, + { + "check_id": "java.lang.security.audit.data-query-injection", + "end": { "col": 4, "line": 62, "offset": 2300 }, + "extra": { + "fingerprint": "ccc333", + "is_ignored": false, + "lines": "query.filter(input)", + "message": "Data query injection finding", + "metadata": { + "category": "security", + "confidence": "LOW", + "cwe": [ + "CWE-943: Improper Neutralization of Special Elements in Data Query Logic" + ], + "impact": "HIGH", + "likelihood": "LOW", + "owasp": [ + "A03:2021 - Injection" + ] + }, + "metavars": {}, + "severity": "WARNING" + }, + "path": "src/main/java/org/owasp/benchmark/testcode/BenchmarkTest00007.java", + "start": { "col": 3, "line": 48, "offset": 2000 } } ], "version": "0.121.0"