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

CWE taxonomy in SARIF report #6373

Closed
Jeeppler opened this issue May 11, 2023 · 23 comments · Fixed by #6839
Closed

CWE taxonomy in SARIF report #6373

Jeeppler opened this issue May 11, 2023 · 23 comments · Fixed by #6839
Labels
community Community contribution feature New feature

Comments

@Jeeppler
Copy link
Contributor

Description

Common Weakness Enumerations (CWE) are a general taxonomy for classes of security issues. They are commonly used in security products. For example, some static application security testing tools providing CWE IDs: Checkmarx SAST, FindSecurityBugs and GoSec.

The SARIF output of Kics does not contain CWE IDs. It would be nice to have the CWE taxonomy implemented in Kics. The SARIF standard contains an element for taxonomies in general and the CWE taxonomy in particular: https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317536.

@Jeeppler Jeeppler added community Community contribution feature New feature labels May 11, 2023
@haerter-tss
Copy link

Are there any news or updates about this feature request? Are you considering putting this into KICS or is this completely out of scope?

@kaplanlior
Copy link
Contributor

Thank you guys for this request (and ping).

Adding the CWE field to the SARIF output is the easy part, adding the CWE IDs to each query is the meticulous work. If you are willing to contribute to that (even partially), we can start a community effort to cover all the queries.

@Jeeppler
Copy link
Contributor Author

@kaplanlior

Adding the CWE field to the SARIF output is the easy part

Generally, I agree. However, please keep in mind SARIF uses taxonomies for CWE.

adding the CWE IDs to each query is the meticulous work. If you are willing to contribute to that (even partially), we can start a community effort to cover all the queries.

We are willing to contribute. However, could you give us some hint/direction on how we should get started?

As far as I understood, all the queries are written in Rego. This rule for example: https://github.com/Checkmarx/kics/blob/master/assets/queries/k8s/weak_tls_cipher_suites/query.rego

It could be categorized as CWE-326. How/where should we add the CWE information to the query?

@kaplanlior
Copy link
Contributor

That's great, looking forward to work on this together. And of course would guide you as needed.

The CWE would be added to the metadata file of each query - metadata.json in the same directory as the rego file.
To follow up on the query you referenced the metadata file is https://github.com/Checkmarx/kics/blob/master/assets/queries/k8s/weak_tls_cipher_suites/metadata.json

I'll update here after we add that field and ready to start adding the metadata to the queries.

@Jeeppler
Copy link
Contributor Author

@kaplanlior thanks, for working on this.

I saw that in the metadata field you have already a field called "category" which I think could be used to map it to "CWE" categories.

Is there a way to get a list of all the categories (for example by using: kics, a rego tool or bash/python script/command)?

@kaplanlior
Copy link
Contributor

That field is used to classify our queries and documentation.

See https://docs.kics.io/latest/queries/ for the current values.

@Jeeppler
Copy link
Contributor Author

Jeeppler commented Dec 6, 2023

@kaplanlior any updates?

@anterosilva1985
Copy link
Contributor

HI @Jeeppler , We are currently working on adding the CWE field to the queries structure. On the end of next week it should be available.
After that you can update the corresponding CWE on each query and we'll review if afterwards.

@Jeeppler
Copy link
Contributor Author

@anterosilva1985 awesome, thanks for the update.

@anterosilva1985
Copy link
Contributor

Hi @Jeeppler , we've added the CWE structure into the queries on the following PR #6829
we'll be able to work on adding the corresponding CWE into the queries now.

@Jeeppler
Copy link
Contributor Author

@anterosilva1985 perfect 💯

@Jeeppler
Copy link
Contributor Author

@anterosilva1985 I created a pull-request, adding some CWE numbers: #6839.

@Jeeppler
Copy link
Contributor Author

@anterosilva1985 we would like to have the CWE information as Taxonomy in the SARIF report. See my first comment: #6373 (comment). What files would we have to look at? How can we start implementing this feature? Or do you plan to implement this feature?

@anterosilva1985
Copy link
Contributor

Hi @Jeeppler we are already working of this https://github.com/Checkmarx/kics/pull/6845/files#diff-dae007130536ae0dee5d54a30b674656c37a838875ad885f6f1f2bff3371e768R150
would this implementation work for you?

@Jeeppler
Copy link
Contributor Author

@anterosilva1985 no, you just add the CWE information as property (a property can be anything). We would like to see the CWE as taxonomy as described here: https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317536.

Here is an example of a taxonomy created by GoSec: https://github.com/mercedes-benz/sechub/blob/develop/sechub-pds-solutions/gosec/docker/mocks/mock.sarif.json#L1060

The tool -> rules contains the information about each rule. Each rule is linked to a CWE through a relationship: https://github.com/mercedes-benz/sechub/blob/develop/sechub-pds-solutions/gosec/docker/mocks/mock.sarif.json#L1171.

This is a bit more involved, but has a number of important advantages:

  • structured CWE information inside the report
  • CWE is versioned (MITRE releases new CWE versions every few months)

@ArturRibeiro-CX
Copy link
Contributor

ArturRibeiro-CX commented Jan 19, 2024

Hi @Jeeppler,
Can you kindly check these KICS sarif output files :
Cwe field completed: file079 (e2e/fixtures/E2E_CLI_079_RESULT).
Cwe field empty: file080 (e2e/fixtures/E2E_CLI_080_RESULT).

Be aware of the 2 examples presented in these files:

  • Lines 37 to 43 of the previous file079 mentioned above provides an example of a Relationship with the field CWE completed on a query (id: "22" as an example for the completed CWE filed);
  • Lines 37 to 43 of the previous file080 mentioned above provides an example of a Relationship with the field CWE empty on a query;

Note: Examples of empty CWE fields will not exist in the future, as we will complete the values of all queries with the corresponding CWE field as soon as possible. This is just an example for testing purposes in the current KICS version.

As you can see in lines 273 to 298, we have information about the taxonomies field and each taxa field related to the CWE item.

Does this align with your expectation for the output sarif file?

Thanks for the feedback.

@Jeeppler
Copy link
Contributor Author

@ArturRibeiro-CX thanks for the detailed explanation. Helped me to understand quickly what the files are about.

Generally, it goes into the right direction. However, I validated the files using: https://sarifweb.azurewebsites.net/Validation. I got the following error message: JSON1001: An instance has a type that is not permitted by the schema's 'type' property..

The issue is this section:

"relationships": [
                {
                  "target": [
                    {
                      "id": "CAT007",
                      "index": 2,
                      "toolComponent": {
                        "name": "Categories",
                        "guid": "58cdcc6f-fe41-4724-bfb3-131a93df4c3f"
                      }
                    },
                    {
                      "id": "",
                      "toolComponent": {
                        "name": "",
                        "guid": ""
                      }
                    }
                  ]
                }
              ]

The correct would be, that there are multiple relationships. Each relationship has a target: https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317872. With other words: the target is not an array.

@ArturRibeiro-CX
Copy link
Contributor

Thanks for the feedback!

This is the implementation we want to achieve, having in mind your latest suggestion (in the image below you can see the full relationships list):

image
  • As you can see in lines 27 to 36, target is no longer an array, but we do need 2 target's since KICS provide information about the query Categories field and now, CWE field as well. What is your perspective with this solution?

Note: We used the Sarif Validator to assure the relationships field was valid.

@Jeeppler
Copy link
Contributor Author

@ArturRibeiro-CX the structure in the screenshot looks good.

However, I would need to have the entire sarif report, otherwise it is difficult to give a definitive answer.

@ArturRibeiro-CX
Copy link
Contributor

Hi Jeeppler, I appreciate your prompt and agile responses!

After reviewing the code and implementing the necessary changes, I wanted to share the updated SARIF report format with you:

  • For the cases where the CWE field is populated (e.g., CWE-ID 22 in this example), please refer to the report labeled E2E_CLI_079_RESULT. In this section, lines 26 to 44 showcase an example of relationships containing information about the Categories field and the CWE field. Additionally, lines 273 to 300 illustrate examples of taxonomies, where the taxa field holds information about the identified CWE in the file.
  • On the other hand, for instances where the CWE field is empty (denoted as " "), you can examine the report labeled E2E_CLI_080_RESULT. In these cases, we opted not to include the CWE field in the relationships. However, the taxonomies still retain information about the CWE field; since no CWE was found, the taxa field remains empty. This can be observed in lines 240 to 257.

Note1: It's important to note that these decisions were made following validations with the SARIF validator and Nist Software. While some issues may surface during these validations, we believe that the overall structure and the final SARIF report align with our expectation. We welcome any further discussion on potential improvements or adjustments based on your feedback.
Note2: Please note that the current example includes instances of empty CWE fields for testing purposes in the current KICS version. However, moving forward, we plan to complete the values of all queries with the corresponding CWE field as soon as possible.

Thank you for your insights, and I look forward to hearing from you soon.

@Jeeppler
Copy link
Contributor Author

@ArturRibeiro-CX looks good to me. Checked it and tested it with SecHub. Works for us.

In addition, thanks for the link to the NIST Validator.

gabriel-cx added a commit that referenced this issue Feb 7, 2024
feat(query): added CWE infos to common and dockerfile queries #6373
@gabriel-cx
Copy link
Collaborator

Hi @Jeeppler @haerter-tss ,

Thank you for your feedback and contributions!

Just to let you know that the PRs in charge to add this feature into KICS were merged.
If you scan with KICS repository master content, you will be able to get information regarding CWE for Common and Docker queries on CLI and SARIF reports (we are working now to add CWE information for the rest of KICS reports).

In the next weeks we will release a new KICS version (v1.7.13), and that version will contain this new feature as well.

@Jeeppler
Copy link
Contributor Author

Jeeppler commented Feb 8, 2024

@gabriel-cx thank you for the collaboration 🥇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Community contribution feature New feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants