Open watson opened 5 years ago
Pinging @elastic/kibana-security (Team:Security)
I have a related PR here which disallows these specific properties for LP routes without any payload validation defined: https://github.com/elastic/kibana/pull/48753
One idea I had to secure route payloads is to enhance @kbn/config-schema
to disallow these properties by default, unless the caller specifically requests an unsafe version. Something like:
import { schema } from '@kbn/config-schema`
// protected by default
schema.any()
schema.object({})
// unprotected
schema.unsafeAny()
schema.unsafeObject({})
Protecting the config-schema
should automatically protect all NP routes, as the NP requires some sort of validation to be applied before it'll allow the payload to be accessed by the route handler.
Anyway, just something to consider -- I can continue with #48753 if you want, or I can leave it alone if you have other approaches you'd like to explore. #48753 is only concerned with legacy platform routes that don't specify any validation. It does not protect routes that declare their own (potentially unsafe) validation, and it also ignores all NP routes.
@legrego Good idea baking it into the validator. In general, I think that's a good solution. Playing devil's advocate for a second, I assume JSON parsing and validation are two separate steps right? If so, would it potentially be possible for say a Kibana plugin/app to sidestep the validation (by accident) or maybe have a need to inject some "middleware" in before the validation step that potentially could be attacked?
Playing devil's advocate for a second, I assume JSON parsing and validation are two separate steps right? If so, would it potentially be possible for say a Kibana plugin/app to sidestep the validation (by accident) or maybe have a need to inject some "middleware" in before the validation step that potentially could be attacked?
Yes, these are two separate steps. A plugin could in theory do something like this to sidestep my proposal
schema.object({
myJsonStringField: schema.string()
})
...
JSON.parse(payload.myJsonStringField)
That part would be covered by #49608 in which we want to harden JSON.parse
itself and provide JSON.parseUnsafe
as an opt-in alternative. This would obviously be a breaking change and some existing plugins might break because of this if they need this functionality.
That makes sense to me. As part of these hardening efforts, we'll want to make sure to communicate to all kibana area teams and solution teams that they need to be cognizant of how their payloads are used, and to understand when they need to use the unsafe escape hatches.
When @kobelb and I talked through #48753, we decided that we could potentially land that in 7.6
, which would give impacted teams enough time to inspect their routes and decide if they need to opt-out of the default protections. I expect that auditing JSON.parse
would be a larger effort though, and might need longer than a single minor release for all teams to complete that work.
@afharo's PR to allow custom validation in the NP should provide us the extension points to port the approach implemented in https://github.com/elastic/kibana/pull/48753 over to the NP.
Look into if it's feasible to disallow hapi request payloads to contain either
__proto__
orconstuctor.prototype
by default (it should probably be possible to allow these on a per-endpoint basis)