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

[Patch] Graphite SSRF patch #331

Closed
1 of 2 tasks
boktorbb opened this issue May 5, 2021 · 0 comments · Fixed by #392
Closed
1 of 2 tasks

[Patch] Graphite SSRF patch #331

boktorbb opened this issue May 5, 2021 · 0 comments · Fixed by #392
Labels
enhancement New feature or request migration Any plans, changes, or enhancements needed for migration
Milestone

Comments

@boktorbb
Copy link
Contributor

boktorbb commented May 5, 2021

Problem Statement

Dashboards contains a server side request forgery (SSRF) flaw in the graphite integration for Timeline visualizer. An attacker with administrative access could set the timeline:graphite.url configuration option to an arbitrary URL. This could possibly lead to an attacker accessing external URL resources as the Dashboards process on the host system.

The proposed open source solution is a hybrid approach that makes use of both the default allowlist that already exists in Dashboards and a blocklist for added security

Configuration: Create a graphite_blocklist setting in opensearch_dashboards.yml that is a list of blocked IPs

The blocklist can be checked in graphite.js . If the list is empty or null, the default allowlist approach of Dashboards can be used for graphite, otherwise we validate the inputted graphite url against the urls present in graphite_blocklist

This allows for default allowlisting behavior for those users who want that approach but still address concerns for cloud providers and platforms that give customers access to Dashboards but are still concerned for internal security and access to APIs that customers shouldn’t have

We also need to make sure that for the HTTP client Dashboards uses that we block follow redirect and have defaults to false, which makes it not vulnerable to an attacker using a fake address and redirecting to IDMS or any other service.

Code flow

The code flow in graphite.js after the blocklist is populated with values is as follows:

  • Aggregate the url/urls/ips being used for graphite by customer (whether it’s written in text box or added in allowlist)
  • Create a function that resolves host-names to IP addresses
  • Verify that the url given is a valid url using below scenarios:
    • block-list enabled and allow-list enabled
      • url is valid only if not in blocklist and in allowlist
    • blocklist enabled and allowlist disabled
      • url is valid only if not present in blocklist
    • blocklist disabled and allowlist enabled
      • url is valid only when present in allowlist
    • blocklist disabled and allowlist disabled
      • urls will not be valid and must enable one of/both solutions
  • Make request if url/urls are valid making sure to set redirect value to error

UI Impact

The UI will need a new component that supports both a drop-down list like current Dashboards allowlisting solution and some form of text input that allows customers to enter a url that will be checked against the block-list

The proposal is a combined component that is a text box that allows for a drop down menu as well. Since we would have both the option for a block-list and allow-list, we would want the textbox portion of the component to be active and allow input only when the block-list setting is not empty or null. This stops the same SSRF CVE from being exploited

  • Design Doc
  • Implementation
@boktorbb boktorbb added enhancement New feature or request migration Any plans, changes, or enhancements needed for migration labels May 5, 2021
@boktorbb boktorbb added this to the 1.x release milestone May 5, 2021
@ananzh ananzh mentioned this issue Jun 1, 2021
5 tasks
@boktorbb boktorbb linked a pull request Jun 3, 2021 that will close this issue
5 tasks
Hailong-am pushed a commit to Hailong-am/OpenSearch-Dashboards that referenced this issue Apr 18, 2024
…6234 (opensearch-project#331)

* remove unnecessary changes due to the refactor in 6234

Signed-off-by: Yulong Ruan <[email protected]>

* fix snapshot

Signed-off-by: Yulong Ruan <[email protected]>

---------

Signed-off-by: Yulong Ruan <[email protected]>
raintygao pushed a commit to raintygao/OpenSearch-Dashboards that referenced this issue Apr 19, 2024
…6234 (opensearch-project#331)

* remove unnecessary changes due to the refactor in 6234

Signed-off-by: Yulong Ruan <[email protected]>

* fix snapshot

Signed-off-by: Yulong Ruan <[email protected]>

---------

Signed-off-by: Yulong Ruan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request migration Any plans, changes, or enhancements needed for migration
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant