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

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

Closed
Tristan971 opened this issue Dec 28, 2022 · 8 comments
Closed

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

Tristan971 opened this issue Dec 28, 2022 · 8 comments
Labels

Comments

@Tristan971
Copy link

After a few comments in apisyouwonthate/style-guide#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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Author

Tristan971 commented Jan 1, 2023

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
Copy link
Contributor

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
Copy link
Contributor

DavidBiesack commented Apr 4, 2023

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
Copy link
Contributor

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

@philsturgeon
Copy link
Contributor

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.

philsturgeon added a commit that referenced this issue Feb 5, 2024
* Removes duplicated test case

* Move API3 rules that focused on defining 400, 401, 500 responses out into API8.

* add unevaluatedproperties rule for OAS 3.1

* added owasp-api3-2023-constrained-additional/unevaluated tests

* updated year on api1 and api2 unchanged

* renamed api4:2019 to api4:2023 only

* added owasp:api2:2023-write-restricted and owasp:api2:2023-read-restricted

* Fixes #25: adds owasp:api5:2023-admin-security-unique

* fixes #21 and makes no-nimeric-ids support any string

* added support for no-server-http to use relative path.

* partially fixes #52: Require servers use x-internal true/false

to explicitly explain what is public or internal for documentation tools

* fixes #52: Servers, define which environment is the API running in

---------

Co-authored-by: Ricagraca <[email protected]>
Copy link

github-actions bot commented Mar 5, 2024

🎉 This issue has been resolved in version 2.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

No branches or pull requests

3 participants