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

🐛 fix AWS Endpoints broken #11815

Open
wants to merge 18 commits into
base: bugfix
Choose a base branch
from
Open

🐛 fix AWS Endpoints broken #11815

wants to merge 18 commits into from
+31 −7

Conversation

quirinziessler
Copy link
Contributor

fix for #11814

@dryrunsecurity DryRunSecurity
Copy link

dryrunsecurity bot commented Feb 13, 2025 •
edited
Loading

DryRun Security Summary

The PR standardizes AWS endpoint host naming by replacing special characters with underscores across multiple files, addressing data transformation risks, improving input sanitization, and managing potential information disclosure in documentation.

Expand for full summary

The PR updates AWS-related parsers and migrations to standardize endpoint host naming by replacing special characters with underscores across multiple files. Security findings include:

  1. Data Transformation Risks in DB Migration (dojo/db_migrations/0222_aws_sechub_update_endpoints.py):

    • Potential for unintended data corruption during ARN transformation
    • Broad filter might inadvertently capture unintended records
    • No validation of AWS ARN structure before modification

  2. Input Sanitization Improvements (multiple files):

    • Prevents potential injection risks in endpoint generation
    • Removes special characters that could cause issues in endpoint naming
    • Mitigates risks associated with special characters in resource identifiers

  3. Potential Information Disclosure (docs/content/en/connecting_your_tools/parsers/file/awssecurityhub.md):

    • Documentation reveals ARN transformation process
    • Could provide insights into parser's internal workings

No critical vulnerabilities were identified, but careful testing is recommended to ensure data integrity and expected behavior.

Code Analysis

We ran 9 analyzers against 6 files and 1 analyzer had findings. 8 analyzers had no findings.

Analyzer Findings
Configured Codepaths Analyzer 1 finding

Overall Riskiness

🔴 Risk threshold exceeded.

We've notified @mtesauro, @grendel513.

View PR in the DryRun Dashboard.

@Maffooch
Copy link
Contributor

@quirinziessler is the main issue here that endpoints cannot be edited in the UI after they are broken? If so, I am not sure if fixing this parser bug would be worth creating duplicate endpoints for all users using these parsers. For example, if users had 100 endpoints with ARNs containing colons, now they will have another 100 endpoint with underscores instead

@quirinziessler
Copy link
Contributor Author

@Maffooch the main issue is that within imports errors are thrown and running into an HTTP 500 in the worst case (depending on finding amount). Furthermore the endpoints can not be migrated.

Would a migration script solve your concerns?

@quirinziessler
Copy link
Contributor Author

@Maffooch any news / decision here?

@Maffooch
Copy link
Contributor

Hi @quirinziessler yes, I think a migration would definitely help users out. Thanks!

@github-actions github-actions bot added the New Migration Adding a new migration file. Take care when merging. label Feb 18, 2025
@quirinziessler
Copy link
Contributor Author

Hi @quirinziessler yes, I think a migration would definitely help users out. Thanks!

done @Maffooch

dependabot bot and others added 7 commits February 18, 2025 22:07
@dependabot @quirinziessler
Bump cryptography from 44.0.0 to 44.0.1 (DefectDojo#11795)
8124f3b 
Bumps [cryptography](https://github.com/pyca/cryptography) from 44.0.0 to 44.0.1.
- [Changelog](https://github.com/pyca/cryptography/blob/main/CHANGELOG.rst)
- [Commits](pyca/cryptography@44.0.0...44.0.1)

---
updated-dependencies:
- dependency-name: cryptography
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@valentijnscholten @quirinziessler
Duplicate Delete errors: catch IntegrityErrors (A) (DefectDojo#11739)
52b8d61 
* option a: catch IntegrityErrors

* linting

* fix object creation

* fix object creation

* bugfixing

* bugfixing

* Update dojo/api_v2/serializers.py
@Maffooch @quirinziessler
Importer Close Old Findings: Accommodate different dedupe algorithms (D…
817d407 
…efectDojo#11729)

* Importer Close Old Findings: Accommodate different dedupe algorithms

* Rename close_old_findings_report_Line31.json to close_old_findings_report_line31.json
@Maffooch @quirinziessler
Hash Codes: Ensure Vulnerability IDs are used in calculations when ad…
9b59510 
…ded outside of import (DefectDojo#11732)
@kevin-vuong99 @quirinziessler
fix aria-label in search box (DefectDojo#11769)
14d1379 
Co-authored-by: Kevin Vuong <[email protected]>
@quirinziessler
Update versions in application files
c3dbaf7
@quirinziessler
Revert "Bugfix "
82f98b3 
This reverts commit bc1785f.
@github-actions GitHub Actions
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions GitHub Actions
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@github-actions github-actions bot removed the helm label Feb 18, 2025
@github-actions github-actions bot added the docs label Feb 19, 2025
@manuel-sommer
Copy link
Contributor

Would be awesome if we could get this on the road until next release @mtesauro and @Maffooch

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

@manuel-sommer manuel-sommer manuel-sommer approved these changes

At least 4 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
docs New Migration Adding a new migration file. Take care when merging. parser unittests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@quirinziessler @Maffooch @manuel-sommer @valentijnscholten @kevin-vuong99