CWE number as enum #31

darkspirit510 · 2022-11-01T15:53:52Z

Migrated CweNumber to enum to ensure there is no magic number stuff going on. Could remove a lot of mapping code that just attempts to map string to number. Since the code has to go through CweNumber's lookup method (which informs about request of unknown number), this (at least somehow) handles #6 and #7. It obviously does not handle default blocks for mapping of any tools internal namings/ids to CweNumbers.

plugin/src/main/java/org/owasp/benchmarkutils/score/CweNumber.java

davewichers · 2022-11-01T18:24:25Z

plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/AcunetixReader.java

-        // Add any 'new' CWEs ever found to switch above so we know they are mapped properly.
-        System.out.println("INFO: Found following CWE which we haven't seen before: " + cweNum);
-        return Integer.parseInt(cweNum);
+        return CweNumber.lookup(cweNum);


This change doesn't allow us to manually decide how to map these. This can be especially important for DAST tools as they frequently report 1 type of injection as another. I'm thinking in cases like this we should leave the manual mapping the way it was. What do you think?

As discussed: #31 (comment)

davewichers · 2022-11-01T18:31:07Z

plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/BurpReader.java

+                return CweNumber
+                        .SENSITIVE_INFORMATION_IN_BROWSER_CACHE; // Information Exposure Through
+                // Browser Caching-Cacheable HTTPS
+                // Response


Can you merge the above 2 lines into a single line comment?

Already fixed

davewichers · 2022-11-01T18:33:23Z

plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/CASTAIPReader.java

-        }
-        return 0000;
+
+        return CweNumber.lookup(name.trim());


Again, I think its better to leave the manual mapping here so we can map in anything 'new'.

As discussed: #31 (comment)

davewichers · 2022-11-01T18:35:46Z

plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/CheckmarxESReader.java

            case 36:
            case 23:
                return CweNumber.PATH_TRAVERSAL;
            case 338:
                return CweNumber.WEAK_RANDOM;
        }
-        return cwe;
+


I think we should add a warning like the others, e.g., "something new seen here not mapped before". Take exact message from other parser that already has this warning.

As discussed: #31 (comment)

davewichers · 2022-11-01T18:39:51Z

plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/CheckmarxReader.java

            case 36:
            case 23:
                return CweNumber.PATH_TRAVERSAL;
            case 338:
                return CweNumber.WEAK_RANDOM;
        }
-        return cwe;
+


Can you add new/missing mapping warning here?

As discussed: #31 (comment)

davewichers · 2022-11-01T18:42:07Z

plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/CodeQLReader.java

-                                + " for rule: "
-                                + ruleName);
+    private CweNumber mapCWE(Integer cweNumber) {
+        if (cweNumber == 335) {


We should retain the explanatory comment of why we are doing this.

davewichers · 2022-11-01T18:42:48Z

plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/CodeQLReader.java

        }
-        return 0; // Not mapped to anything
+
+        return CweNumber.lookup(cweNumber);


I prefer that we keep the manual mapping so we can get the warning of 'new' types of issues not yet mapped.

As discussed: #31 (comment)

davewichers · 2022-11-01T19:15:18Z

plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/HorusecReader.java

}
+

Can we restore what we had so we can flag reported CWEs not yet mapped?

As discussed: #31 (comment)

davewichers · 2022-11-01T19:17:49Z

plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/InsiderReader.java

-                int cwe = cweNumber(finding);
-                tcr.setCWE(cwe);
+                String cwe = finding.getString("cwe").substring(4);
+                tcr.setCWE(CweNumber.lookup(cwe));


Restore what we had so we can flag reported CWEs not yet mapped. And there is one mapping here that doesn't go straight up, 326 to Weak Crypto.

davewichers · 2022-11-01T19:20:09Z

plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/NJSScanReader.java

        }

-        return cwe;
+        return CweNumber.lookup(cwe);


Can we restore what we had so we can flag reported CWEs not yet mapped?

As discussed: #31 (comment)

davewichers · 2022-11-01T19:23:53Z

plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/SemgrepReader.java

        }
-        return cwe;
+        return CweNumber.lookup(cwe);


Can we restore what we had so we can flag reported CWEs not yet mapped?

As discussed: #31 (comment)

davewichers · 2022-11-01T19:26:00Z

plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/ShiftLeftReader.java

            case "xss":
-                return 79;
+                return CweNumber.XSS;
            default:
                throw new RuntimeException("Unknown category: " + category);


Maybe change this to a simple system out println? Runtime exception seems harsh.

I can change this, but I didn't touch this in my PR.

davewichers · 2022-11-01T19:37:23Z

plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/SnappyTickReader.java

        }
-        return 0;
+


Can we restore what we had so we can flag reported CWEs not yet mapped?

As discussed: #31 (comment)

plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/SonarQubeReader.java

plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/VisualCodeGrepperReader.java

davewichers · 2022-11-01T20:51:48Z

plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/WapitiReader.java

        Node refs = getNamedChild("references", vuln);
        List<Node> references = getNamedChildren("reference", refs);
        for (Node ref : references) {
            String title = getNamedChild("title", ref).getTextContent();
            if (title.startsWith("CWE-")) {
                String cweNum = title.substring("CWE-".length(), title.indexOf(":"));
-                cwe = cweLookup(cweNum);
+                cwe = CweNumber.lookup(cweNum);


Can we restore the old cweLookup so we can still get the warning "WARNING: Wapiti-Unmapped CWE number: " + cwe);"?

As discussed: #31 (comment)

davewichers · 2022-11-01T20:53:38Z

plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/ZapJsonReader.java

}
+

Can we restore the old cweLookup so we can still get the warning "WARNING: ZAP CWE not mapped to expected test suite CWE: " + cwe);

As discussed: #31 (comment)

plugin/src/test/java/org/owasp/benchmarkutils/score/CweNumberTest.java

davewichers · 2022-11-01T20:56:10Z

@darkspirit510 - Thanks so much for all this refactoring work! I provided some minor comments in a few dozen of the files changed. Can you address those comments and we should be good to go.

darkspirit510 · 2022-11-06T18:25:10Z

Sorry for the late reply :-) Since most of your comments are about the "missing" default message, one general reply why I removed them:

After checking all mapping methods in all Reader classes, you can divide them into three groups:

Mapping from tool-internal string/code/whatever to CWE number. This can definitly not be automated and is untouched from me (except, of course, the enum conversion)
Mapping from one CWE number to another. This is "hacky", because Benchmark has a limited set of expected CWE numbers, although there are some more detailed CWE numbers describing the same thing (e. g. CWE-89 "SQL injection" and CWE-564 "Hibernate SQL Injection"). Since Benchmark does not have any fuzzyness/forgiveness and only accepts a specific CWE numbers, some readers contain mappings to "fix"/map one CWE number to another. This is sad, but has to stay in the reader.
The only type of mappings I removed (which for some classes means remove the mapping code completly) is mapping one CWE number (sometimes as String) to exactly the same CWE number. If a given number did not exist in a mapping method, there was a log message. What's the value of this, if it is just mapping a number on itself? If a tool already reports CWE numbers, why not just pass them to the enum and only warn if the enum does not (yet) contain this number.

davewichers · 2022-11-11T18:38:18Z

As you address issues per my comments above, can you add comments to each issue you've addressed so I can resolve comments on fixed issues?

darkspirit510 · 2022-11-12T07:50:14Z

If you accept my explaination, i can write a short note to every comment.

davewichers · 2022-11-14T15:13:47Z

@darkspirit510 - that works. Thanks.

davewichers · 2023-03-07T19:39:07Z

@darkspirit510 - Did you ever work on my comments (per above)? Also, there is a branch conflict now, w/SemgrepReader. Can you resolve that too?

darkspirit510 · 2023-03-07T20:50:47Z

@davewichers most of the comments should be explained in comment #31 (comment). But I actually missed some comments, I'll go through them within the next days.

darkspirit510 · 2023-03-10T22:23:52Z

I fixed/commented all the things. If you're still not sure about this PR, I think about closing and redoing it in smaller chunks.

davewichers · 2023-04-25T13:07:45Z

@darkspirit510 - There was a lot of work done here, but might have been overcome by events. Do you want finish this one? Or start over and do this in smaller chunks?

davewichers · 2023-06-05T15:04:32Z

@darkspirit510 - There was a lot of work done here, but might have been overcome by events. Do you want finish this one? Or start over and do this in smaller chunks?

@darkspirit510 - Nudge :-)

darkspirit510 · 2023-06-06T17:03:00Z

Since this is way too old, I'll redo it in smaller chunks 😅

darkspirit510 added 3 commits November 1, 2022 10:54

refactor CweNumber to enum

2c3b32e

revert missing line

5a88f84

lots of missing CWE numbers

c739069

davewichers reviewed Nov 1, 2022

View reviewed changes

plugin/src/main/java/org/owasp/benchmarkutils/score/CweNumber.java Show resolved Hide resolved

davewichers reviewed Nov 1, 2022

View reviewed changes

plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/SonarQubeReader.java Outdated Show resolved Hide resolved

davewichers reviewed Nov 1, 2022

View reviewed changes

plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/VisualCodeGrepperReader.java Outdated Show resolved Hide resolved

davewichers reviewed Nov 1, 2022

View reviewed changes

plugin/src/test/java/org/owasp/benchmarkutils/score/CweNumberTest.java Show resolved Hide resolved

restore file header

4efc94c

darkspirit510 added 3 commits November 6, 2022 19:28

add/revert header; print caller class in warn message

c8809e7

fix comments; linting

20cff4d

handle changes in semgrep result file structure

84e6fbf

darkspirit510 mentioned this pull request Nov 8, 2022

handle changes in semgrep result file structure #33

Merged

darkspirit510 added 2 commits December 28, 2022 11:52

Merge branch 'OWASP-Benchmark:main' into main

91af174

Merge branch 'OWASP-Benchmark:main' into main

e4653cd

darkspirit510 added 5 commits March 10, 2023 23:00

restore / adapt mapping comment

798597b

restore mapping

50c4169

Merge branch 'OWASP-Benchmark:main' into main

cb52942

revert wildcard import

0951323

merge main, fix conflicts

e23a899

darkspirit510 closed this Jun 6, 2023

CWE number as enum #31

CWE number as enum #31

Conversation

darkspirit510 commented Nov 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davewichers commented Nov 1, 2022

darkspirit510 commented Nov 6, 2022

davewichers commented Nov 11, 2022

darkspirit510 commented Nov 12, 2022

davewichers commented Nov 14, 2022

davewichers commented Mar 7, 2023

darkspirit510 commented Mar 7, 2023

darkspirit510 commented Mar 10, 2023

davewichers commented Apr 25, 2023

davewichers commented Jun 5, 2023

darkspirit510 commented Jun 6, 2023

darkspirit510 commented Nov 1, 2022 •

edited

Loading