Kibana: Issues with AngularJS fork

[kobelb]

The changes which were made to AngularJS here removed quite a few calls to encodePath.

This makes me rather uneasy from the security perspective, because any code-paths which were relying on the escaping to be done by AngularJS here is no longer being done. I'm no AngularJS expert, and there's potential that my security fears are unfounded since we're only using AngularJS's routing within the "fragment" portion of the URL. It definitely goes against the conventional wisdom of always encoding user specified input in URLs.

However, the change that was made does break the routing for any Dashboards/Visualizations which has a space in the ID and likely other characters which were previously escaped. This alone seems like justification to investigate alternative approaches.

[elasticmachine]

Pinging @elastic/kibana-platform

[epixa]

cc @eliperelman

[eliperelman]

It definitely goes against the conventional wisdom of always encoding user specified input in URLs

Agreed, but for parity between React and Angular, we need to push this from being handled by the framework to being the responsibility of those making changes to the URL.

From a security perspective, this is more of an issue if we are evaluating URL content, but I could not find any instances where that was the case.

the change that was made does break the routing for any Dashboards/Visualizations which has a space in the ID and likely other characters which were previously escaped

I tried to find instances where this occurred but had trouble finding them via tests and source-diving, so we will need to find and fix these.

This alone seems like justification to investigate alternative approaches.

If anyone has advice for another solution that solves #34300 and provides that parity between React and Angular please let me know so we can iterate on this.

[kobelb]

Agreed, but for parity between React and Angular, we need to push this from being handled by the framework to being the responsibility of those making changes to the URL.

Assuming we need to push this to the consumers changing the URL, we went and removed the automatic encoding of URL parts which consumers previously relied upon without changing the consumers. For example, the following in graph:

https://github.com/elastic/kibana/blob/5ae5e3d8c61e81aba6eeca961077b92628fe8b40/x-pack/plugins/graph/public/app.js#L210

Used to implicitly rely on the encodePath function in AngularJS encoding the ID, which is no longer being done. This allows one to maliciously change the ID of a graph worksheet, which then appears unencoded within the URL which we redirect the user to.

Even if we were to change the consumer of kbnUrl.change to encode the IDs before calling kbnUrl.change, the removal of the decodePath calls in AngularJS make it so that when we're reading the the parameters of the URL using facilities like $route.current.params, the parameters are no longer automatically decoded. For example, http://localhost:5601/app/kibana#/discover/awesome%20search used to cause $route.current.params.id to be awesome search, but now it's awesome%20search.

From a security perspective, this is more of an issue if we are evaluating URL content, but I could not find any instances where that was the case.

I'm not following. That's certainly not the case if we weren't performing encodeURIComponent for a part of a path outside of the fragment for a url. This would allow us to build URLs like https://some-server/kibana-instance-1/app/kibana/../../../some-other-potentially-more-secure-instance or https://some-server/kibana-instance-1/app/kibana?queryStringValue=somethingElseEntirely which could lead to some interesting forms of abuse. We might be safe here because we're dealing with the fragment portion of the URL, and I'm unaware of any attack vectors using the fragment directly. However, just because we aren't aware of any attack vectors doesn't mean we should be allowing it because browsers are weird and inconsistent and we're in uncharted waters.

I tried to find instances where this occurred but had trouble finding them via tests and source-diving, so we will need to find and fix these.

In old versions of Kibana, we used to allow users more first-class ways of controlling URLs, and we can still do so using the various Saved Object Management APIs. The easiest way I've found to mutate the IDs is to export the JSON of the saved object, modify the ID parameter, and then re-import it.

[eliperelman]

@kobelb do you recommend we roll this back until we can do a proper security assessment?

[kobelb]

@kobelb do you recommend we roll this back until we can do a proper security assessment?

I think that's the safest course of action. The security concerns I have are largely theoretical, but given the negative impact the change has on existing URLs which I've tried to outline above, it seems like the decision is worth re-considering.

[kobelb]

This revert PR wasn't backported properly, so we shipped 7.3 with the forked version of AngularJS and we're posed to ship 7.4 with the forked version of AngularJS.

[spalger]

I'm going to backport https://github.com/elastic/kibana/pull/46393 to 7.4 so that we can release 7.4.1 without the fork

[kobelb]

Closing, as Spencer was kind enough to backport this for us.