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

Nil pointer dereferences in builtin OWASP ruleset if schema is missing optional components field #428

Closed
TheTeaCat opened this issue Jan 24, 2024 · 3 comments

Comments

@TheTeaCat
Copy link
Contributor

TheTeaCat commented Jan 24, 2024

According to the OpenAPI spec the components field isn't required:

The OpenAPI document MUST contain at least one paths field, a components field or a webhooks field.

So, for clarity, this spec requires you have at least one of the following properties: paths, components or webhooks. You don't have to have all three. You could just have paths, or just have components, or just have webhooks, or any combination of the three.

There's several OWASP rules that assume a components property exists without checking first, which is problematic as we've established it's optional in the specification as long as you have at least a paths or webhooks property instead. For example in auth_insecure_schemes.go:

ss := context.DrDocument.V3Document.Components.SecuritySchemes

If you try to generate a spectral report for a spec with no components property you'll get a panic due to a nil pointer dereference:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x2 addr=0x38 pc=0x101321c6c]

goroutine 242 [running]:
github.com/daveshanley/vacuum/functions/owasp.AuthInsecureSchemes.RunRule({}, {0x30?, 0x101cbbda0?, 0x200001400025c978?}, {0x14000514030, 0x1400020e3c0, {0x101b8ce20, 0x101e202f0}, {0x0, 0x0}, ...})
        /Users/josh/Documents/vacuum/functions/owasp/auth_insecure_schemes.go:32 +0x5c
github.com/daveshanley/vacuum/motor.buildResults({0x1400020e3c0, 0x140001b2e60, 0x140001b2e60, {0x101e2d4e0, 0x14000487980}, 0x14000194e70, 0x1400011a0c0, 0x140003a0a00, 0x140001d6a50, 0x14000119350, ...}, ...)
        /Users/josh/Documents/vacuum/motor/rule_applicator.go:769 +0x588
github.com/daveshanley/vacuum/motor.runRule({0x1400020e3c0, 0x140001b2e60, 0x140001b2e60, {0x101e2d4e0, 0x14000487980}, 0x14000194e70, 0x1400011a0c0, 0x140003a0a00, 0x140001d6a50, 0x14000119350, ...}, ...)
        /Users/josh/Documents/vacuum/motor/rule_applicator.go:691 +0x910
created by github.com/daveshanley/vacuum/motor.ApplyRulesToRuleSet.func7 in goroutine 128
        /Users/josh/Documents/vacuum/motor/rule_applicator.go:554 +0x21c

You might get a panic in a different rule for the same reason - they're all run in different goroutines. The affected rules afaik are:

  • AuthInsecureSchemes
  • JWTBestPractice
  • NoApiKeyInUrl
  • NoBasicAuth
  • NoApiKeyInUrl

I'll try and get a patch PR out for this this afternoon/tomorrow morning (GMT).

@daveshanley
Copy link
Owner

I already have this fixed, it's arriving in a patch shortly. I discovered it myself during testing also.

@TheTeaCat
Copy link
Contributor Author

Oh wow OK that saves me some time! Thanks ❤️

daveshanley added a commit that referenced this issue Jan 24, 2024
Signed-off-by: quobix <dave@quobix.com>
@daveshanley daveshanley mentioned this issue Jan 24, 2024
daveshanley added a commit that referenced this issue Jan 24, 2024
Signed-off-by: quobix <dave@quobix.com>
@daveshanley
Copy link
Owner

just needed a nil check in the owasp rules, should be fixed in v0.8.3, once it's finished moving through the pipeline.

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

No branches or pull requests

2 participants