Transform unsafe fields in Elasticsearch client payloads

[jportner]

The Elasticsearch Node.js client (ES client) has prototype pollution protection enabled by default; if an unsafe field is detected (constructor.prototype or __proto__), it throws an error. This protection can be optionally disabled by using the disablePrototypePoisoningProtection option.

Over the past 9 months we have slowly been moving all consumers to use this new ES client (#83910). This default protection has broken legitimate usage of ES documents with the __proto__ field in at least one instance (#101944). We considered a few options on how to handle this:

• If we use disablePrototypePoisoningProtection: true by default for the client, it will fix this issue but also put Kibana widely at risk for prototype pollution attacks (and possible remote code execution as a result). The attack surface is quite large and hard/impossible to audit or quantify. The problem is that downstream consumers can potentially use the ES client payloads in an insecure manner.
• If we instruct consumers to use disablePrototypePoisoningProtection: true on a case-by-case basis, this likely turns into a big game of whack-a-mole. In this instance, the index pattern would be able to load the fields correctly, but then any subsequent calls by other consumers to load the documents for that index would run into this problem.
• If we leave this protection in place, it puts Kibana widely at risk for denial-of-service attacks by clever manipulation of this protection. An attacker would need access to ingest a document or modify an index mapping to do so, but they could cause some or all of Kibana calls to ES to fail, so the risk is still concerning.

That leads me to proposing what seems like the least-bad approach:

1. Use disablePrototypePoisoningProtection: true for non-system requests (e.g., to data indices)
2. Add a "serialize/deserialize" transformation in the Core ElasticsearchClient to transform these problematic fields; we can add a prefix such as dangerousJsonField- to ES responses and remove it from ES requests

This allows all downstream consumers to continue functioning while keeping broad prototype pollution protection in place. The risk is that it breaks any existing filters, searches, visualizations, etc. that used the raw constructor.prototype or __proto__ fields back in old Kibana versions when these were allowed. I think it's safe to assume that usage of this is pretty rare.

This isn't an ideal solution but I don't see a better alternative without a massive refactoring effort (such as forcing consumers to use Map, which is a safe way to interact with these dangerous fields).

---

Side notes:

• The Core ElasticsearchClient currently only surfaces these error messages in the debug logs. If it encounters the "Object contains forbidden prototype property" error, it should surface these in error logs instead.
• There should be no legitimate first-party usage of constructor.prototype or __proto__ fields in Kibana -- in other words, system indices should not need to use these fields [citation needed]. We can probably leave the existing prototype poisoning protection in place for system requests. Alternatively, we can also use the proposed transformation approach.
• We should collect usage data on how often the constructor.prototype and __proto__ fields are encountered so we can better gauge if any follow-on work is needed. We already have the core-usage-stats saved object where we can add a counter that is incremented whenever dangerous fields are encountered/transformed.

[elasticmachine]

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

[pgayvallet]

We currently don't have any wrapper around the library's client, we're just using a vanilla client and only customizing it via the allowed configuration options. So before going any further, we need to make sure this proposal could be done by just client configuration.

Use disablePrototypePoisoningProtection: true for non-system requests (e.g., to data indices)

A system request being a request against a Kibana system index then, right?

Got some doubts about the technical feasability. I'm unsure we can override such options on a request basis ? I know we're using a custom Transport to override opaqueId on the fly, but I suspect that options such as disablePrototypePoisoningProtection may not work the same way, at it's used to instantiate the serializer, and I don't think one is created per request (cc @delvedor).

Add a "serialize/deserialize" transformation in the CoreElasticsearchClient to transform these problematic fields

The CoreWhat? :) there isn't such thing (at least atm).

I see the client does accept a Serializer?: typeof Serializer; option, but I'm not 100% sure it would allow us to do this. (I actually suspects we would need to manually implement the unsafe fields protection if we are to override the default serializer, @delvedor can you confirm)

export interface SerializerOptions {
  disablePrototypePoisoningProtection: boolean | 'proto' | 'constructor'
}

export default class Serializer {
  constructor (opts?: SerializerOptions)
  serialize(object: any): string;
  deserialize(json: string): any;
  ndserialize(array: any[]): string;
  qserialize(object: any): string;
}

Just to be clear: if from a security standpoint, this feature is considered mandator, can't be done via the plain client, and requires us to rethink the viability of a wrapper or proxy around the vanilla client, I'm not opposed to re-open the discussion. However, this would come at a performance (and development) cost, so I really want to make sure this is a necessity.

[delvedor]

Got some doubts about the technical feasability. I'm unsure we can override such options on a request basis ? I know we're using a custom Transport to override opaqueId on the fly, but I suspect that options such as disablePrototypePoisoningProtection may not work the same way, at it's used to instantiate the serializer, and I don't think one is created per request (cc @delvedor).

Currently you can't disable the protection on a per-request basis. As per discussion with @mshustov and @legrego, you can disable it client-wide, as we'll disable/remove that options in the version 8 of the client.

[mshustov]

If we use disablePrototypePoisoningProtection: true by default for the client, it will fix this issue but also put Kibana widely at risk for prototype pollution attacks (and possible remote code execution as a result).

To attack a Kibana instance, an abuser should have write access to Elasticsearch. But if a malicious actor has write access, then a deployment has got a bigger problem of data exposure and data loss. Doesn't it?

A system request being a request against a Kibana system index then, right?Got some doubts about the technical feasibility.

Kibana provides two interfaces to interact with the Elasticsearch server:

• asInternalUser is meant to be used for accessing system indices
• asCurrentUser provides access to data indices

So, it sounds like we can set disablePrototypePoisoningProtection: true for asCurrentUser client instance then. Did I miss anything?

The risk is that it breaks any existing filters, searches, visualizations, etc. that used the raw constructor.prototype or proto fields back in old Kibana versions when these were allowed. I think it's safe to assume that usage of this is pretty rare.

But if we allow users to supply data with constructor.prototype or __proto__ fields, it means they might want to create visualizations, filters, searches relying on these.

[jportner]

A system request being a request against a Kibana system index then, right?

A system request being the usage of the ElasticsearchClient that is configured as the internal user:

kibana/src/core/server/elasticsearch/client/cluster_client.ts

Line 68 in 5480c4d

this.asInternalUser = configureClient(config, { logger, type, getExecutionContext });

I didn't mean to suggest that we attempt to change the config on a per-request basis. Though, if this is getting removed in version 8 of the client anyway, I guess we don't need to make a distinction.

The CoreWhat? :) there isn't such thing (at least atm).

Sorry, I meant the Core ElasticsearchClient, not the CoreElasticsearchClient.

Edit: forgot to respond to this too

Just to be clear: if from a security standpoint, this feature is considered mandator, can't be done via the plain client, and requires us to rethink the viability of a wrapper or proxy around the vanilla client, I'm not opposed to re-open the discussion. However, this would come at a performance (and development) cost, so I really want to make sure this is a necessity.

I believe we can do everything we need on a per-client basis using the Serializer overrides as suggested 👍 I can spend some time trying this out to be sure. If that doesn't give us what we need, I think we will need to reevaluate this enhancement against our other priorities.

---

To attack a Kibana instance, an abuser should have write access to Elasticsearch. But if a malicious actor has write access, then a deployment has got a bigger problem of data exposure and data loss. Doesn't it?

The concern here is that granting a user write access to a single ES index may result in them being able to take over the Kibana machine entirely via RCE.

So, it sounds like we can set disablePrototypePoisoningProtection: true for asCurrentUser client instance then. Did I miss anything?

Yeah, that's what I was trying to suggest!

But if we allow users to supply data with constructor.prototype or __proto__ fields, it means they might want to create visualizations, filters, searches relying on these.

Yeah, that would be the tradeoff here. They can still create visualizations or filters but they would need to use the "transformed" field name. If we implemented my suggestion above, that would be constructor.dangerousJsonField-prototype and dangerousJsonField-__proto__, respectively.

[mattkime]

I'm not sure I'm 100% following the suggestion to transform dangerous field names but generally speaking I'm not so crazy about the idea as it seems like it would require extra handling when creating queries. I assume they'd need to be transformed back at some point.

IMO it seems that we should be avoiding object mutation since thats the mechanism by which these problematic field names have unwanted behavior. Its as simple as using new Map instead of {} - although changing habits can be difficult.

[pgayvallet]

Use disablePrototypePoisoningProtection: true for non-system requests (e.g., to data indices)

So, it sounds like we can set disablePrototypePoisoningProtection: true for asCurrentUser client instance then. Did I miss anything?

both asCurrentUser and asInternaUser are used to query the kibana indices AFAIK. Not sure why we would want to distinguish and have them behave differently? From my understanding, the distinction was more on data vs system indices?

[jportner]

generally speaking I'm not so crazy about the idea as it seems like it would require extra handling when creating queries. I assume they'd need to be transformed back at some point.

@mattkime Yes that's true, we could blindly remove all instances of the prefix such as dangerousJsonField- but that could be problematic. That does really throw a wrench in the works...

IMO it seems that we should be avoiding object mutation since thats the mechanism by which these problematic field names have unwanted behavior.

Agreed, and I'd like to do that in the long term, it's just figuring out how we get there that's the hard part. As mentioned we don't have a viable way to audit for such object mutation.

both asCurrentUser and asInternaUser are used to query the kibana indices AFAIK.

@pgayvallet Can you point to an example of this? The .kibana index at the very least should only be used by the internal user.

[legrego]

To attack a Kibana instance, an abuser should have write access to Elasticsearch. But if a malicious actor has write access, then a deployment has got a bigger problem of data exposure and data loss. Doesn't it?

Not necessarily. I'm sure that most of us indirectly have write access to clusters somewhere by virtue of agents that are installed on our machines. They ship information about a runtime that is under our control (our laptops), to a cluster that is not within our control.

Can you point to an example of this? The .kibana index at the very least should only be used by the internal user.

The event log is a good example of a system (hidden?) index that is accessed by both the internal and current users.

---

I tend to agree with @mattkime -- rewriting these fields for the sake of safety is probably going to cause more problems than it solves, and could lead to confusion ("why is my field named dangerousJsonField-proto? That's not what it's called in my index!")

IMO it seems that we should be avoiding object mutation since thats the mechanism by which these problematic field names have unwanted behavior. Its as simple as using new Map instead of {} - although changing habits can be difficult.

I think this is the right answer, but it will probably take years to get to the point where this is possible -- and even then, it won't be possible to enforce this.

Maybe it makes sense to expose disablePrototypePoisoningProtection as an option to consumers when creating the ES Client instance in the short-term, and allow plugins to opt-out where it makes sense to do so? I don't love this approach, but it might be the more pragmatic solution. If we move forward with this, then we need to work with teams to audit/test that __proto__ and friends work correctly, so that we have a consistent experience across all of Kibana.

[jportner]

Maybe it makes sense to expose disablePrototypePoisoningProtection as an option to consumers when creating the ES Client instance in the short-term, and allow plugins to opt-out where it makes sense to do so? I don't love this approach, but it might be the more pragmatic solution. If we move forward with this, then we need to work with teams to audit/test that __proto__ and friends work correctly, so that we have a consistent experience across all of Kibana.

I am worried about the "whack-a-mole" effect of this. We can fix things on a case-by-case basis but doing this may uncover additional scenarios where these fields break other app experiences. Then we've got multiple Kibana versions that handle this differently for different plugins, and support becomes a nightmare.

If we can't stomach the transformation (seemed like a palatable idea at first but I think the concerns are quite valid), perhaps we should just go with the original plan, disable this across Kibana, and redouble our efforts to catch these prototype pollution vulnerabilities.

[mshustov]

Maybe it makes sense to expose disablePrototypePoisoningProtection as an option to consumers when creating the ES Client instance in the short-term, and allow plugins to opt-out where it makes sense to do so?

Considering @mattkime wants to land a fix in 7.15, we can apply this simplest option if we agree that we can leave with a few drawbacks:

• we will have to create custom instances for all the API dealing with user-supplied data (or wait for a problem to be reported by users)
• it's easy to misuse a wrong client instance in the plugin code (for example, pass an instance created by Core, instead of a custom client instance).

[jportner]

@legrego , @mshustov , @pgayvallet and I met today to discuss this proposal and other options. Here's where we landed:

• The transformation as proposed is too complex and can break the UX in potentially unknown ways
• We don't want to have consumers selectively disabling this protection, potentially kicking off a nightmarish whack-a-mole game where we run into these types of issues and fix them across different Kibana versions
• We don't want to disable this protection only for non-internal users (e.g., leave it enabled for the internal user) because users can still access system indices in certain situations and we open ourselves up to hard-to-triage DoS attacks that way

So, the safest and most reasonable thing to do is to disable disablePrototypePoisoningProtection for all ES clients, and our security posture is no worse off than it was before Nov 2020.

[kobelb]

I get that protecting against prototype pollution is super hard. However, non-dot-prefixed indices contain documents that we should effectively be treating as unsafe user input. We need some solution here. It doesn't have to be this very instant, but we should not assume that if a user can write a document to Elasticsearch that they are a trusted user who should be able to execute arbitrary code on the server that runs Kibana.

We don't want to disable this protection only for non-internal users (e.g., leave it enabled for the internal user) because users can still access system indices in certain situations and we open ourselves up to hard-to-triage DoS attacks that way

Do you mind elaborating on this? Starting in 8.0, users will have to explicitly circumvent the protections that we've put in place to prevent users from reading/writing Kibana's system-indices.

[jportner]

we should not assume that if a user can write a document to Elasticsearch that they are a trusted user who should be able to execute arbitrary code on the server that runs Kibana.

Agreed.

Do you mind elaborating on this? Starting in 8.0, users will have to explicitly circumvent the protections that we've put in place to prevent users from reading/writing Kibana's system-indices.

The two examples that were brought up to me were the event log and ML jobs. There are only two ES clients provided by Core, one for the internal user and one for the current user. There is overlap between the indices that both of these clients access.

[kobelb]

The two examples that were brought up to me were the event log and ML jobs. There are only two ES clients provided by Core, one for the internal user and one for the current user. There is overlap between the indices that both of these clients access.

I can only speak definitively about the event log, but Kibana is the only thing that should be writing documents to the event log; however, end-users can read these documents directly if they need "free form analysis". While it's technically possible that an end-user could write documents to the event log indices if they were given privileges to do so, they would break functionality in Kibana that reads the documents from the event log using the system user.

This is a nitpick and doesn't impact the situation, but the event log indices are technically "hidden indices" not "system indices".

[jportner]

Makes sense. Sorry, I was trying to clarify but I didn't outright state: we don't have the ability to dynamically change the ES client config based on whether the consumer is accessing a system index or not. We only have the ability to change the ES client config based on whether the internal user is being used or not. So while the internal user is generally used for system indices, it is not always -- some of the indices it accesses are not system indices, like you mentioned about the event log. It follows then that if we attempted to simply leave this protection in place for the client that is used by the internal user, there can be cases where regular users can manipulate some of these "overlapping" indices and cause a DoS.

[jportner]

We decided to move forward and merge #109425.

As @legrego mentioned in #109425 (comment), we need to prioritize a cohesive plan for addressing prototype pollution, and we'll do so in #58040.