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

hapi request payload hardening #49609

Open
watson opened this issue Oct 29, 2019 · 7 comments
Open

hapi request payload hardening #49609

watson opened this issue Oct 29, 2019 · 7 comments
Labels
Feature:Hardening Harding of Kibana from a security perspective R&D Research and development ticket (not meant to produce code, but to make a decision) Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more!

Comments

@watson
Copy link
Contributor

watson commented Oct 29, 2019

Look into if it's feasible to disallow hapi request payloads to contain either __proto__ or constuctor.prototype by default (it should probably be possible to allow these on a per-endpoint basis)

@watson watson added Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! R&D Research and development ticket (not meant to produce code, but to make a decision) Feature:Hardening Harding of Kibana from a security perspective labels Oct 29, 2019
@watson watson self-assigned this Oct 29, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-security (Team:Security)

@legrego
Copy link
Member

legrego commented Oct 29, 2019

I have a related PR here which disallows these specific properties for LP routes without any payload validation defined: #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.

@watson
Copy link
Contributor Author

watson commented Oct 29, 2019

@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?

@legrego
Copy link
Member

legrego commented Oct 29, 2019

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)

@watson
Copy link
Contributor Author

watson commented Oct 29, 2019

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.

@legrego
Copy link
Member

legrego commented Oct 29, 2019

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.

@kobelb
Copy link
Contributor

kobelb commented Jan 7, 2020

@afharo's PR to allow custom validation in the NP should provide us the extension points to port the approach implemented in #48753 over to the NP.

@exalate-issue-sync exalate-issue-sync bot added impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:small Small Level of Effort labels Aug 5, 2021
@legrego legrego removed EnableJiraSync loe:small Small Level of Effort impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. labels Aug 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Hardening Harding of Kibana from a security perspective R&D Research and development ticket (not meant to produce code, but to make a decision) Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more!
Projects
None yet
Development

No branches or pull requests

4 participants