stoplightio / spectral-owasp-ruleset

Improve the security of your API by detecting common vulnerabilities as defined by OWASP and enforced with Spectral.
70 stars 11 forks source link

Rules enhancements / api1:2019-no-numeric-ids #21

Closed Tristan971 closed 9 months ago

Tristan971 commented 1 year ago

After a few comments in https://github.com/apisyouwonthate/style-guide/issues/40, I'll raise a few questions/objections to the current OWASP ruleset. It's not so much a criticism but sharing some perspective we may or may not agree on.


Context

The rule api1:2019-no-numeric-ids more or less enforces usage of UUIDs as public resource identifiers, to avoid unsecured APIs to allow malicious users to guess URIs of resources they shouldn't access.

I would oppose the following to the current rule:

  1. UUIDs' randomness isn't security, as they're not a cryptographically secure source of randomness
  2. Numeric identifiers aren't any more insecure, so long as they are behind proper permission checks

Current Behavior

As per Context section

Expected Behavior

  1. Allow numeric resource identifiers, so long as they're gated behind an authentication schema
  2. Optionally: Allow some way to flag a resource endpoint as explicitly public and meant to be traversable, such that numeric identifiers aren't unnecessarily flagged (imagine something like /announcements/1, /announcements/2, /announcements/3 for a public announcement system)
philsturgeon commented 1 year ago

This rule was taken from advice on apisecurity.io.

image

https://apisecurity.io/encyclopedia/content/owasp/api1-broken-object-level-authorization

UUIDs are not on their own security, but they do stop people downloading your entire database whether or not they have credentials, so I was happy to include the advice from this wiki.

I wrote about the importance of things like UUID over auto incrementing IDs here.

https://phil.tech/2015/auto-incrementing-to-destruction/

philsturgeon commented 1 year ago

Oh it's not just in that third party site, its in the API Security Top 10 itself.

image

https://github.com/OWASP/API-Security/blob/master/2019/en/src/0xa1-broken-object-level-authorization.md

Tristan971 commented 1 year ago

I wrote about the importance of things like UUID over auto incrementing IDs here.

https://phil.tech/2015/auto-incrementing-to-destruction/

Yes UUIDs are better than numeric ids as a kind of first-line defense, but they are one of many approaches, and come with severe performance drawbacks as your article's update notes, unless special care is given to the problem (and even in such cases, they .

Oh it's not just in that third party site, its in the API Security Top 10 itself. [...]

Article which also notes using a proper access control mechanism as way to prevent it...? Especially the example they give is much more fundamental than numeric/UUID identifier schemes, and rather about not having literally 0 authorization mechanism... You can't genuinely argue that UUIDs or not are the focus of the point they're making. And the 3 reference links they put with it are ALL about authorization, and not about identifiers.


In the end, this isn't so much that I disagree with UUIDs, and [we] do use them, but they are very much a tradeoff that really isn't always a good one for a lot of reasons from performance to SEO concerns, and still aren't security on their own.

The "problem" (opposition, perhaps?) is that the rule is at the error level by default, rather than the warning one, and doesn't allow an alternative of secure scheme + $whateverelse ids.

philsturgeon commented 1 year ago

The OWASP document having the word "Prefer" at the start could be used as the basis for knocking this down from error to warning if anyone fancies firing off a PR.

DavidBiesack commented 1 year ago

Requiring UUID is too specific The ID values in an API should be secure, opaque strings, but many formats that are not UUIDs are possible and should be allowed. For example, prefixing a UUID with a resource-type prefix should be allowed; this helps a developer looking at the IDs to do a sanity check that the ID is an ID of the correct (type of) resource. Another example: taking a UUID and encoding in Base36 is functionally equivalent to a UUID, but is about 12 characters shorter. In JJ Gewax's "API Design Patterns" book, JJ presents a algorithm for generating DX friendly resource IDs (avoid 0 and O characters, etc.) This rule would not allow such strings. I think this error (warning) should not be triggered at all if the type is still string. I agree triggering this for type: integer is still helpful as a likely security flaw.

DavidBiesack commented 1 year ago

(IOW, the rule is "no-numeric-ids", so it is misleading to trigger this on an ID defined as a string.)

philsturgeon commented 9 months ago

Would love PRs or suggestions, so far it's not clear what people actually want, something less rigid but potentially less helpful? Let's try and strike a balance between rigid and vague and throw over a PR as I've got some time to work on this thanks to SmartBear.

github-actions[bot] commented 8 months ago

:tada: This issue has been resolved in version 2.0.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: