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

🐛 jake json output in cyclonedx not parsed #9873

Merged
merged 4 commits into from
Apr 15, 2024
Merged

🐛 jake json output in cyclonedx not parsed #9873

merged 4 commits into from
Apr 15, 2024

Conversation

manuel-sommer
Copy link
Contributor

@manuel-sommer manuel-sommer commented Apr 3, 2024 •
edited

Exisiting jake.json in cyclonedx is not considered in the parser.

@dryrunsecurity DryRunSecurity
Copy link

dryrunsecurity bot commented Apr 3, 2024 •
edited

Hi there 👋, @DryRunSecurity here, below is a summary of our analysis and findings.

DryRun Security Status Findings
AppSec Analyzer (beta) ✅ 0 findings
Secrets Analyzer (beta) ✅ 0 findings
Authn/Authz Analyzer ✅ 0 findings
Configured Codepaths Analyzer ✅ 0 findings
Sensitive Files Analyzer ✅ 0 findings

Note

🟢 Risk threshold not exceeded.

Tip

Get answers to your security questions. Add a comment in this PR starting with @DryRunSecurity. For example...

@dryrunsecurity What are common security issues with web application cookies?

Powered by DryRun Security

@manuel-sommer manuel-sommer reopened this Apr 3, 2024
@mtesauro
Copy link
Contributor

mtesauro commented Apr 3, 2024

@manuel-sommer We should be checking that CycloneDX files from tool vendors are actually valid per the spec before adjusting the CycloneDX parser. I just had a conversation with Steve Springett (person who created CycloneDX) about vendor tool support and his any my experience agree that vendors say "we output ClycloneDX" but the really output "CycloneDX-ish" files.

If I look at the the JSON unit test file for Jake at https://cyclonedx.github.io/cyclonedx-web-tool/validate, I get:
image

If it's not valid CycloneDX, it's probably better to create a Jake specific parser rather than make our CycloneDX parser parse invalid CycloneDX.

That validator above was the first online one I found - it may not be the best but I still think the CycloneDX parser should probably only parse valid versions and have vendor-specific parsers when the tool provides invalid formats.

I'll ask around and see if there's a better CycloneDX validator than that one I found online.

@grendel513
Copy link
Contributor

@manuel-sommer We should be checking that CycloneDX files from tool vendors are actually valid per the spec before adjusting the CycloneDX parser. I just had a conversation with Steve Springett (person who created CycloneDX) about vendor tool support and his any my experience agree that vendors say "we output ClycloneDX" but the really output "CycloneDX-ish" files.

If I look at the the JSON unit test file for Jake at https://cyclonedx.github.io/cyclonedx-web-tool/validate, I get: image

If it's not valid CycloneDX, it's probably better to create a Jake specific parser rather than make our CycloneDX parser parse invalid CycloneDX.

That validator above was the first online one I found - it may not be the best but I still think the CycloneDX parser should probably only parse valid versions and have vendor-specific parsers when the tool provides invalid formats.

I'll ask around and see if there's a better CycloneDX validator than that one I found online.

@manuel-sommer i suggest a specific parser for Jake be implemented instead.

@mtesauro
Copy link
Contributor

mtesauro commented Apr 3, 2024

OK. I got curious and ran this official CycloneDX validator from https://github.com/CycloneDX/cyclonedx-cli against the unit test files at django-DefectDojo/unittests/scans/cyclonedx

$ for i in `ls *.json`
> do
> echo "Checking $i"
> ../../../../cyclonedx validate --input-file "$i"
> done
Checking cyclonedx_cwe.json
Unable to validate against any JSON schemas.
BOM is not valid.
Checking issue_8022.json
BOM validated successfully.
Checking issue_9277.json
BOM validated successfully.
Checking jake2.json
BOM validated successfully.
Checking jake.json
Unable to validate against any JSON schemas.
BOM is not valid.
Checking log4j.json
BOM validated successfully.
Checking nested-component-log4j.json
BOM validated successfully.
Checking valid-vulnerability-1.4.json
BOM validated successfully.

So the net result of that was these are valid:

  • issue_8022.json
  • issue_9277.json
  • jake2.json
  • log4j.json
  • nested-component-log4j.json
  • valid-vulnerability-1.4.json

And these are invalid:

  • cyclonedx_cwe.json
  • jake.json

I also repeated for XML with these results (all XML unit test files are valid)

  • cyclonedx_bom.xml
  • dd_1_15_0.xml
  • grype_dd_1_14_1.xml
  • grype.xml
  • jake.xml
  • log4j.xml
  • retirejs.latest.xml
  • spec1_lowfirst.xml
  • spec1.xml
  • valid-vulnerability-1.4.xml

@mtesauro
Copy link
Contributor

mtesauro commented Apr 3, 2024

@manuel-sommer So I love that you were fixing an issue with Jake's CycloneDX parser (Thanks for that 🙏 ) but it looks like the right path forward is to:

(A) Look at jake.json and figure out how we got an invalid CycloneDX file as a unit-test file. I'd not spend a bunch of time on that but whatever is discovered either:

  1. Remove jake.json and update related unit tests to use a valid file
  2. Run Jake against something to generate a valid jake.json file and update the unit test to use this newly created json output.

2 is the best since we're sure we're getting recent & valid output from Jake. I have no clue how old/state jake.json is but that's one possibility.

(B) Remove the invalid json files and update the unit tests to use valid files instead.

Ideally, you could use the file generated in 2 above to replace the invalid ones as well. I've not had a chance to look at what unit tests use those invalid files but they'll need to be updated once the invalid JSON files are removed so they keep passing when getting called.

@github-actions github-actions bot removed the parser label Apr 9, 2024
@manuel-sommer
Copy link
Contributor Author

I removed jake.json. I don't think I will make a PR to support jake.json as there also exists a jake2.json file which is valid. Maybe, jake.json was an outdated version of jake.

Regarding cyclonedx_cwe.json I was not able to detect on why the schema is invalid. Furthermore, it would be an option to include cyclonedx-cli to validate cyclonedx directly on the input, but it has no python package. I found https://pypi.org/project/cyclonedx-editor-validator/ but that validator is inconsistent and does not look like being stable.

@mtesauro
Copy link
Contributor

@manuel-sommer

I removed jake.json. I don't think I will make a PR to support jake.json as there also exists a jake2.json file which is valid. Maybe, jake.json was an outdated version of jake.

Regarding cyclonedx_cwe.json I was not able to detect on why the schema is invalid. Furthermore, it would be an option to include cyclonedx-cli to validate cyclonedx directly on the input, but it has no python package. I found https://pypi.org/project/cyclonedx-editor-validator/ but that validator is inconsistent and does not look like being stable.

Understandable about not making a separate Jake parser based on that invalid jake.json file.

I also didn't find a good Python equivalent to cyclonedx-cli in Python. Something we can do in the future.

So this PR now just removes that invalid jake.json file 👍

@mtesauro
Copy link
Contributor

@manuel-sommer Before we approve & merge this PR, would you mind removing the cyclonedx_cwe.json file and it's matching unit test so all the invalid CycloneDX files are removed in this PR?

See https://github.com/search?q=repo%3ADefectDojo%2Fdjango-DefectDojo%20cyclonedx_cwe.json&type=code

@manuel-sommer
Copy link
Contributor Author

Done @mtesauro

mtesauro
mtesauro approved these changes Apr 10, 2024
View reviewed changes
Copy link
Contributor

@mtesauro mtesauro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved

@Maffooch Maffooch merged commit 01914af into DefectDojo:bugfix Apr 15, 2024
123 checks passed
@manuel-sommer manuel-sommer deleted the jake_cyclonedx branch April 15, 2024 16:52
hblankenship pushed a commit to hblankenship/django-DefectDojo that referenced this pull request Apr 26, 2024
@manuel-sommer @hblankenship
🐛 jake json output in cyclonedx not parsed (DefectDojo#9873)
4b2f29a 
* 🐛 jake json output in cyclonedx not parsed

* remove jake as it is invalid

* flake8

* remove cyclonedx_cwe
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grendel513 grendel513 grendel513 approved these changes

@mtesauro mtesauro mtesauro approved these changes

@Maffooch Maffooch Maffooch approved these changes

@blakeaowens blakeaowens blakeaowens approved these changes

Assignees
No one assigned
Labels
unittests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@manuel-sommer @mtesauro @grendel513 @Maffooch @blakeaowens