stoplightio / spectral-owasp-ruleset

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

Rules enhancements / api4:2019-string-restricted #24

Closed Tristan971 closed 8 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 api4:2019-string-restricted requires that:

Schema of type string must specify a format or pattern. To avoid unexpected values being sent or leaked, ensure that strings have either a format or a RegEx pattern. This can be done using format or pattern.

This is rather questionable in a lot of cases where freeform input might be meant (and thus anything short of a .+ allowed pattern will be incorrect).

Also, from a pure security standpoint, user-defined strings should instead always be considered as random text with 0 particular technical meaning. Otherwise you get all kinds of vulnerabilities like XSS, SQLi, etc.

The use of patterns is otherwise okay for pure business rules without particularly high risks if a malicious actor finds a bypass of some sort.

Current Behavior

As per Context section

Expected Behavior

Tristan971 commented 1 year ago

If you insist on having it, make it a warning, and make it an error to have a pattern that isn't bounded (ie ^pattern$) since that's by far the number one bypass reason of these

Also I would still strongly advocate against ever encouraging regex matching as a security measure. It's only a stopgap thing, and it's often incredibly difficult to have, even with bounds, a regex that isn't trivially bypassable after a few hours of looking around...

As long as it's for non-critical data, it remains an ok system, more as a way to catch typos by end-users for example.

philsturgeon commented 1 year ago

The false sense of security argument is confusing. If you're using this OpenAPI for contract testing and server-based validation then its not a false sense of security. Requests literally match this or they are rejected. If you're not doing this then you're unlikely to be using the OWASP ruleset because why would you care if this random artifact that vaguely resembles your API is secure or not if they're not actually the same thing.

I'm not personally a big fan of forcing format/pattern onto everything, but this is one of those Enterprise Requests that seemed to be a requirement.

If its good enough for the Italian Government it's probably good enough for others, and as always rules can be disabled or have their severity changed.

Does anyone else have any opinions about this rule either way?

Tristan971 commented 1 year ago

If you're using this OpenAPI for contract testing and server-based validation then its not a false sense of security

I'm aware of both, used both in the past, and still do use the latter. (slightly OT but I would be incredibly suspicious of anyone claiming that the former is ever actually viable, for behaviour compliance specifically that is, in anything but small PoCs).

Requests literally match this or they are rejected.

This is exactly the point I'm making. If you have any necessity to accept somewhat free-form-ish input (forum software for example, or even just a search field with non-ASCII input due to i18n), you end up with a bunch of ^.*$ rules that are entirely meaningless and merely look like security without being security at all. But hey, you ticked that box and it's now green. That's what I'm calling a false of security here. Of course for well-defined input space (say, a year field), then it's very much doable yes.

[...] because why would you care if this random artifact that vaguely resembles your API is secure or not if they're not actually the same thing.

That is a strawman, and somewhat related to the contract-testing component, where pretending that an OpenAPI spec perfectly captures your API in both syntax and behaviours is either a colossal task (if at all possible) or an illusion (most likely). But that is a slightly OT debate again, I guess.

as always rules can be disabled or have their severity changed.

I wasn't particularly arguing against the rule in general; it's not bad/nefarious.

But moving it: 1. to warning, and 2. with some more resilient approaches linked (like the OWASP CRS) would be (imo) better security advice.

savage-alex commented 1 year ago

Spectral has the ability for consumers to tune the rulesets to their levels. I do however agree that this feels like a warning (I wouldn't fail a build for it which some teams do when they see error level).

I think there should be another rule thats related to this stating that any patterns that are not wrapped properly should be raised as warnings otherwise the overall pattern length is infinite.