`owasp-protection-global-unsafe` rule is failing to detect previously recognised cases

[TheTeaCat]

I'm not sure what's going on here but in the example appspec, where security has been removed from the DELETE /pets/{petId} endpoint, the owasp-protection-global-unsafe doesn't pick it up any more:

openapi: 3.0.1
info:
  title: Security disabled for specific endpoint
  version: "0.1"
servers:
  - url: https://example.com
paths:
  /pets:
    post:
      summary: "create a pet"
      responses:
        "200":
          description: some descriptive text
  /pets/{petId}:
    parameters:
      - name: petId
        in: path
        required: true
        schema:
          type: integer
    get:
      summary: "get a pet"
      responses:
        "200":
          description: some descriptive text
    delete:
      summary: "delete a pet"
      responses:
        "200":
          description: some descriptive text
      security: []
components:
  securitySchemes:
    jwt:
      type: http
      scheme: bearer
      bearerFormat: JWT
security:
  - jwt: []

Relevant part of the OpenAPI spec, under the Operation Object see the description for the security field:

To remove a top-level security declaration, an empty array can be used.

This was detected in earlier versions of vacuum.

I'm guessing there's something funky in the logic here:

https://github.com/daveshanley/vacuum/blob/0e30ba4eacac7b5694e3c65cc23f79a5a5394b7a/functions/owasp/check_security.go#L104-L105

When I've played around with it it starts erroneously reporting that security is also missing from the POST /pets endpoint too though.

[TheTeaCat]

I think it boils down to the value of opValue.Security being nil in both the cases where no security property is present (so the global security applies) and when a security: [] is present (overriding the global security to disable it).

https://github.com/pb33f/doctor/blob/3644e80c85901727e8082eea4aa5e4dac2acf25b/model/high/v3/operation.go#L20C15-L20C44

[TheTeaCat]

Bingo, if security is [] then the for loop here iterates zero times, so o.Security is never assigned to and remains nil, making security: [] indistinguishable from having no security property at all:

        if operation.Security != nil {
                for i, security := range operation.Security {
                        security := security
                        s := &drBase.SecurityRequirement{}
                        s.Parent = o
                        s.IsIndexed = true
                        s.Index = i
                        wg.Go(func() { s.Walk(ctx, security) })
                        o.Security = append(o.Security, s)
                }
        }

Quick fix:

        if operation.Security != nil {
        o.Security = []*drBase.SecurityRequirement{}
                for i, security := range operation.Security {
                        security := security
                        s := &drBase.SecurityRequirement{}
                        s.Parent = o
                        s.IsIndexed = true
                        s.Index = i
                        wg.Go(func() { s.Walk(ctx, security) })
                        o.Security = append(o.Security, s)
                }
        }

The logic is still a bit wonky in vacuum/functions/owasp/check_security.go though. Should just be opValue.Security != nil && len(opValue.Security) <= 0.

[daveshanley]

Would you mind submitting a PR? I would be most grateful.

[daveshanley]

I re-wrote all those rules pretty quickly, I am sure there are a few other gaps.

[TheTeaCat]

No worries, I'm on a lil holibob until Monday so it'll be a few days.

Otherwise if anyone else wants some easy pickings in the meantime feel free, I'd appreciate it!

On Thu, 25 Jan 2024, 14:59 quobix, @.***> wrote:

I re-wrote all those rules pretty quickly, I am sure there are a few other gaps.

— Reply to this email directly, view it on GitHub https://github.com/daveshanley/vacuum/issues/431#issuecomment-1910381384, or unsubscribe https://github.com/notifications/unsubscribe-auth/AE3FYHLTDMIBHOITPYZTFCLYQJXN3AVCNFSM6AAAAABCKKVTHKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMJQGM4DCMZYGQ . You are receiving this because you authored the thread.Message ID: @.***>

[TheTeaCat]

@daveshanley see PRs above