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

[RFC] Clickjacking Mitigation #5639

Closed
tianleh opened this issue Dec 22, 2023 · 17 comments · Fixed by #5641
Closed

[RFC] Clickjacking Mitigation #5639

tianleh opened this issue Dec 22, 2023 · 17 comments · Fixed by #5641
Assignees
Labels
proposal RFC Substantial changes or new features that require community input to garner consensus.

Comments

@tianleh
Copy link
Member

tianleh commented Dec 22, 2023

(This RFC will now depend on #5796 and its content will be updated.)

Problem Statement #

Clickjacking is a vulnerability when an application allows its content to be loaded inside a frame of an attacker-controlled webpage. The attacker-controlled webpage contains UI elements aligned in such a way that when the customer thinks they are clicking on a UI element in the visible page, they are clicking on the invisible element from the framed page floating above it. Security testers find this vulnerability existing in OpenSearch Dashboards. The root cause is that OSD by default allows dashboards to be shared as embed code in iFrame (no x-frame-options HTTP headers or frame-ancestors in CSP rules). All downstream OSD offerings running on top of open source OSD inherit the same vulnerability.

We will evaluate multiple approaches to mitigate this issue by minimizing the impact to user experiences and avoiding duplicated work in downstream OSD offerings.

Desired State #

We want to resolve the vulnerability with the approval from Security testers. In the meanwhile, we continue to support the existing customers who use iFrame features. A self service option is preferred to minimize the operational load to OSD system administrator. Our solution shall simplify the effort for all downstream OSD offering.

Out of Scope #

We do not plan to backport the changes to versions already released to minimize impact to existing customers using iFrame features. Only new versions will have this mitigation.

We do not plan to provide new UIs as long as existing UIs could meet the requirements.

Challenges #

There are seven challenges for this project.

  1. OSD YML change requires a server restart to take effect. Any approach proposing YML change will have the risk of service availability interruption. Furthermore, for multi-tenancy service, it is infeasible to support dynamic customer configurations through the current YML file.

  2. The existing request handlers in core OSD only rely on the OSD YML file. However, a proposed approach may need to read OSD indices and thus requires an instance of OpenSearch client which may not be ready yet during service setup. The existing OSD Security plugin is able to perform authentication for every request. It gives potential directions to structure the new feature into a plugin (inside core) or postpone the registration of the new handler after the readiness of OpenSearch client.

  3. We need to define the order of precedence for the configurations from the proposed approach and the OSD YML file. There are already some existing practice at OpenSearch side where a persistent setting shall override YML settings. We need to properly implement the precedence to avoid confusions to customers.

  4. We shall provide customers the flexibility to enumerate allowed sources instead of simply a boolean option to either allow all sources or deny all sources. OSD YML currently supports two types of HTTP headers related to frame embedding, x-frame-options and Content Security Policy (CSP) rules. We will need to check their browser support and decide which one to use.

  5. When fine-grained access control (FGAC) is enabled, there shall be a way to manage the permission to read and write the configurations. Customers without permissions cannot alter the configuration leading to vulnerability. Also the configuration shall be consistent among all the tenants.

  6. Multiple data sources and metadata storage have decoupled from OpenSearch as database. Our approach shall be extensible for different types of configuration storage (e.g DynamoDB, Postgres) instead of hard coding the dependence on OpenSearch.

  7. A page load could trigger hundreds of requests (HTML, JavaScript, CSS, image, etc). If every request has to query a storage service, it will create spikes.

Proposed Approach #

Content Security Policy (CSP) is an added layer of security that helps to detect and mitigate certain types of attacks, including Cross-Site Scripting (XSS) and data injection attacks. We propose to introduce a new index (.opensearch_dashboards_config) to store the CSP frame-ancestors directive which specifies valid parents that may embed a page using frames. When it is set to 'self', it only allows embedding by the origin from which the protected page is being served. The directive with value 'none' would prevent any frame embedding. It can also be set to a list of allowed sources with wildcard to achieve flexibility.

The index won’t exist by default. When the OSD process reads it empty, the value constructed from OSD YML file will be returned. The default value from OSD YML will add frame-ancestors set to 'self' along with some other existing default values. We will implement APIs at OSD to allow customers to read and write the content of the index. Note that the OSD process won’t create this index and thus there is no concern for race condition. When FGAC is enabled, similar to how regular indices are access controlled, only customers with the write permission could perform the update. OSD admins could delegate the work to others by assigning them a role which has write permission to the new index. Below is the list of APIs customers can call.

Get the CSP rules.

curl 'http://localhost:5601/api/configuration/getCspRules'

Response:

{"cspRules":"script-src unsafe-eval self; worker-src blob: self; style-src unsafe-inline self; frame-ancestors self https://example-site.org22"}%

Check if CSP rules are configured.

curl 'http://localhost:5601/api/configuration/existsCspRules'

Response:
{"exists":true}%

Update CSP rules.


curl 'http://localhost:5601/api/configuration/updateCspRules' -X POST -H 'Accept: application/json' -H 'Content-Type: application/json' -H 'osd-xsrf: osd-fetch' -H 'Sec-Fetch-Dest: empty' --data-raw '{"value":"script-src 'unsafe-eval' 'self'; worker-src blob: 'self'; style-src 'unsafe-inline' 'self'; frame-ancestors 'self' https://example-site.com"}'

Response: 

{"updatedRules":"script-src unsafe-eval self; worker-src blob: self; style-src unsafe-inline self; frame-ancestors self https://example-site.com"}%

Delete the CSP rules configured.

curl 'http://localhost:5601/api/configuration/deleteCspRules' -X POST -H 'osd-xsrf: osd-fetch' -H 'Sec-Fetch-Dest: empty'

Response:

{"deletedCspRulesName":"csp.rules"}%

In terms of implementation, a new internal plugin (ConfigurationProvider) will be introduced to register a request handler to read the CSP rules from the new index and add them to the response head. (The existing HttpServiceSetup has a function registerOnPreResponse.) Since there is no new UI introduced by this plugin, it will only have server side and no client side. To avoid checking for each request, the handler will use the attribute sec-fetch-dest from request headers and only apply when the attribute’s value is a document or frame. Thus other requests with sec-fetch-dest set to image or script will not be applied.

To support generic storage, the configuration may not necessarily be stored into an OpenSearch index. Thus an interface abstracting operations for this configuration would be introduced. Below is the proposed interface.

export interface ConfigurationClient {
    existsCspRules(): Promise<boolean>;

    getCspRules(): Promise<string>;

    updateCspRules(cspRules: string): Promise<string>;

    deleteCspRules(): Promise<string>;
}

To illustrate each function, let’s use OpenSearch as an example. The function existsCspRules will return whether the document csp.rules exists in index opensearch_dashboards_config. The function getCspRules will return the CSP rules stored in the document. (e.g "frame-ancestors 'self'") The function updateCspRules will update the document with the input cspRules. The function deleteCspRules will delete the document from the new index.

A default implementation to this interface will be based on OpenSearch. The ConfigurationProvider plugin would expose a set function to allow other plugins (external to OSD core) to provide different implementations to this interface. Specifically, the ConfigurationProvider has a member variable configurationClient which is of the type ConfigurationClient mentioned above. It also has a member method setConfigurationClient to set the value of the configurationClient. This setConfigurationClient function would be exposed as return value of ConfigurationProvider’s setup function. The ConfigurationProvider has another member method getConfigurationClient to return the client to use. See below class skeleton for references.

// ConfigurationProviderPlugin
export class ConfigurationProviderPlugin{
    private configurationClient: ConfigurationClient;

    private getConfigurationClient(inputOpenSearchClient: IScopedClusterClient) {
        if (this.configurationClient) {
            // use whatever client if already set
            return this.configurationClient;
        }
        
        // Default OpenSearchConfigurationClient implements the interface ConfigurationClient which is 
        // based on OpenSearch.
        return new OpenSearchConfigurationClient(inputOpenSearchClient);
    }
    
    private setConfigurationClient(inputConfigurationClient: ConfigurationClient) {
        this.configurationClient = inputConfigurationClient;
    }
    
    public async setup(core: CoreSetup) {
  
        ...
        // addCspRulesPreResponseHandler is a request handler which will read 
        // the CSP rules and update the CSP headers
        core.http.registerOnPreResponse(this.addCspRulesPreResponseHandler(core));
        
        return {
            // use bind to preserve this
            setConfigurationClient: this.setConfigurationClient.bind(this),
        };
  
    }

  }

Now an external plugin, e.g MyConfigurationPlugin could inject its version of ConfigurationClient into the ConfigurationProviderPlugin in the following way.

It will import the type ConfigurationProviderPluginSetup from the directory of ConfigurationProviderPlugin and add to its own setup input.

# MyConfigurationPlugin   
  public setup(core: CoreSetup, { configurationProviderPluginSetup }: ConfigurationProviderPluginSetup) {
     ...
    # The function createClient provides an instance of ConfigurationClient which
    # could have a underlying DynamoDB or Postgres implementation.
    const myClient: ConfigurationClient = this.createClient();

    configurationProviderPluginSetup.setConfigurationClient(myClient);
     ...  
    return {};
  }

Based on the approach proposed above, we will explain how it addresses all the challenges mentioned in previous section.

For the first challenge, it doesn’t change the YML file and thus won’t require a restart for configuration change. For the second challenge, it resides in the plugin stage (different from those in core stage, e.g HttpService, OpenSearchService) and could acquire an OpenSearch client without worrying about its readiness. For the third challenge, it will use whatever the customers have configured through the new index. If there is no such index, it will use whatever the OSD YML has configured. If YML has no CSP related configurations, it will use the default values which prevents clickjacking. For the fourth challenge, it uses CSP frame-ancestors which could allow a list of sources and also have support in modern browsers. As a comparison, X-Frame-Options could only support either SAMEORIGIN or DENY because its value ALLOW-FROM which used to support allowed lists has been deprecated. For the fifth challenge, the new index would need write permission to be modified. Also it is the only index regardless of the number of tenants. As a comparison to the .kibana index, there will exist one such index for each tenant (e.g .kibana_{tenant name}) which will lead to data inconsistency. For the sixth challenge, different storage providers are supported through building an external storage plugin. For the seventh challenge, with the filtering of sec-fetch-dest, we basically only need to query the new index once in a page load. Based on an experiment of loading OSD home page, there are totally 69 requests and only 1 request (HTML) needs to query the new index.

While the approach addressed the challenges, it has several disadvantages. The first disadvantage is that for customers who want to specify their allowed sources, they will need to manually create or update the index directly. There is a risk that the page fails to load due to manual errors in the index document. (For customers without such need, the new index doesn’t need to exist at all and thus this disadvantage doesn’t apply.) To address the concern, we will add more exception handling around reading the index document in the request handler. Specifically when there is an exception, the OSD process shall continue smoothly without failing. The second disadvantage is that it introduces maintenance effort when it exists to exclude it from unintended index management workflows. (e.g ISM policies) Again for customers using the default experience, the disadvantage won’t apply. Even if in worst case where the new index is accidentally deleted, customers could always self service a recovery by creating it again. We will mention this in the public documents about the instruction.

Alternative Approaches #

Alternative Approach 1: Update OSD YML #

Currently OSD YML already supports setting up csp.rules value by using csp.rules: ["frame-ancestors 'self' https://example.com"]. By default, this configuration is empty. We can specify this configuration with value 'self'. Or we can modify the default value of csp.rules to add frame-ancestors 'self' without touching the YML file. When customers want to add allowed sources to csp.rules, they will need to modify the YML file and a server restart will happen. Customers who use to only access Kibana through an endpoint may now need to access the hosts running OSD processes to change YML file (or through contacting administrators). This approach fails to solve Challenge 1.

Alternative Approach 2: Use Advanced Settings #

The Advanced settings page allows customers to modify settings that govern OpenSearch Dashboards behavior. These settings can be used to customize the look and feel of the application, change the behavior of certain features, and more. This approach will modify OSD source code to introduce a new field in advanced settings to configure the value of frame-ancestors. The value will be saved into the current OSD index (.kibana). We will default the value to 'self' to avoid releasing a software with security issue. Customers have the control to modify the configuration if they want to take the security risk. By setting the value to DENY, the page cannot be displayed in a frame, regardless of the source attempting to do so. It will then need a request handler to read and apply the configuration which will be similar to the internal plugin in the proposed approach.

Since the approach goes through advanced settings, it doesn’t require a server restart. However, this approach will lead to inconsistent behavior when FGAC is enabled since each tenant has their own OSD index associated with their tenant name. This approach fails to solve Challenge 5.

Appendix #

POC #

We have a working POC tracked in this dev branch https://github.com/tianleh/OpenSearch-Dashboards/commits/cj-dev/

FAQ #

@tianleh
Copy link
Member Author

tianleh commented Dec 22, 2023

Hi, team @opensearch-project/opensearch-dashboards-core Please help take a look.

cc @seraphjiang

@tianleh
Copy link
Member Author

tianleh commented Dec 23, 2023

cc @wbeckler

@ashwin-pc ashwin-pc added proposal RFC Substantial changes or new features that require community input to garner consensus. and removed untriaged labels Dec 28, 2023
@ruanyl
Copy link
Member

ruanyl commented Jan 2, 2024

For the sixth challenge, different storage providers are supported through building an external storage plugin

That means there will be DynamoDBCspClient, PostgresCspClient besides OpenSearchCspClient?

@ruanyl
Copy link
Member

ruanyl commented Jan 2, 2024

To support generic storage, the configuration may not necessarily be stored into an OpenSearch index. Thus an interface abstracting operations for this configuration would be introduced. Below is the proposed interface.

Shall we expose a update API to encourage user to update the csp rules via a consistent API instead of interacting with the storage service directly like calling PUT .opensearch_dashboards_config/_doc/csp.rules?

@tianleh
Copy link
Member Author

tianleh commented Jan 3, 2024

That means there will be DynamoDBCspClient, PostgresCspClient besides OpenSearchCspClient?

Yes.

@tianleh
Copy link
Member Author

tianleh commented Jan 3, 2024

Shall we expose a update API to encourage user to update the csp rules via a consistent API instead of interacting with the storage service directly like calling PUT .opensearch_dashboards_config/_doc/csp.rules?

Good question. When FGAC enabled, the permission control at index level is easy to achieve through roles. However, for API level permission, we may need to build the API through an OpenSearch (backend) plugin or OpenSearch core. (Currently there is no direct permission control over OSD APIs.) It is technically possible but may require quite extra effort.

@ruanyl
Copy link
Member

ruanyl commented Jan 3, 2024

Shall we expose a update API to encourage user to update the csp rules via a consistent API instead of interacting with the storage service directly like calling PUT .opensearch_dashboards_config/_doc/csp.rules?

Good question. When FGAC enabled, the permission control at index level is easy to achieve through roles. However, for API level permission, we may need to build the API through an OpenSearch (backend) plugin or OpenSearch core. (Currently there is no direct permission control over OSD APIs.) It is technically possible but may require quite extra effort.

I see, in case of opensearch as storage, for index level permission control, when calling update API, we can actually solve the permission problem by using a scoped opensearch client based on the current user.

However, the problem remains if we consider to support other storages such as DynamoDB or Postgres. Supporting various storage service as the meta store for OSD is rather a large and different topic, I think you're right, perhaps for now, it's a wise choice to let user to update the csp data directly :)

@tianleh
Copy link
Member Author

tianleh commented Jan 5, 2024

However, the problem remains if we consider to support other storages such as DynamoDB or Postgres. Supporting various storage service as the meta store for OSD is rather a large and different topic, I think you're right, perhaps for now, it's a wise choice to let user to update the csp data directly :)

Yes. The current proposal makes a first step (abstraction of the client and allow external plugin to register) towards supporting more storage for this configuration. It will require more future effort to support updating through API/UX and related permission control.

@bandinib-amzn
Copy link
Member

bandinib-amzn commented Jan 12, 2024

We do not plan to backport the changes to versions already released to minimize impact to existing customers using iFrame features. Only new versions will have this mitigation.

Does that mean old versions and existing customers would be still vulnerable to clickjacking? Any future plans to mitigate the issue for existing customer?

Furthermore, for multi-tenancy service, it is infeasible to support dynamic customer configurations through the current YML file.

Is it infeasible to support dynamic configuration only for multi tenancy service? Is it achievable for single tenant service? Also I'm little bit confused here about the expected result. Suppose there are two users User A and OSD administrator. User A is using private tenant. OSD administrator specifies CSP rule. Does that CSP rule will also apply to User A who is using private tenant?

@Flyingliuhub
Copy link
Member

@tianleh did you schedule a meeting for new feature review, iirc, there are process to review the new feature.

@tianleh
Copy link
Member Author

tianleh commented Jan 18, 2024

@tianleh did you schedule a meeting for new feature review, iirc, there are process to review the new feature.

I have started a thread with PM for the process.

@tianleh
Copy link
Member Author

tianleh commented Jan 18, 2024

We do not plan to backport the changes to versions already released to minimize impact to existing customers using iFrame features. Only new versions will have this mitigation.

Does that mean old versions and existing customers would be still vulnerable to clickjacking? Any future plans to mitigate the issue for existing customer?

Furthermore, for multi-tenancy service, it is infeasible to support dynamic customer configurations through the current YML file.

Is it infeasible to support dynamic configuration only for multi tenancy service? Is it achievable for single tenant service? Also I'm little bit confused here about the expected result. Suppose there are two users User A and OSD administrator. User A is using private tenant. OSD administrator specifies CSP rule. Does that CSP rule will also apply to User A who is using private tenant?

  1. We did a likelihood analysis and consulted security reviewers. It is agreed to solve this in new versions. For existing customers, we would suggest them to upgrade to versions with this fix.

  2. User A will be applied the new rules. This is a system wide configuration.

@tianleh
Copy link
Member Author

tianleh commented Jan 22, 2024

1/22/2024 update:

We have made some changes during PR review. (The main content of the issue has been updated to reflect it. )

  1. We have introduced APIs to support read and write against the configurations. Although there is no new UI added, customers could call the API from curl to perform the updates.
  2. We have made the API input parameters irrelevant to the OpenSearch index or document name for ease of extension to support various types of storage.
  3. We have renamed the plugin to be generic instead of being coupled with CSP. This can help onboard more configurations into the new index.

@bbarani
Copy link
Member

bbarani commented Feb 6, 2024

@tianleh @Flyingliuhub @ruanyl can you please confirm if this change can be included in 2.x without breaking existing API? Basically can this change be added in a backward compatible manner in 2.x line?

We are evaluating if this change warrants a 3.0 release or can be included in 2.x line so need your inputs.

@tianleh
Copy link
Member Author

tianleh commented Feb 6, 2024

@tianleh @Flyingliuhub @ruanyl can you please confirm if this change can be included in 2.x without breaking existing API? Basically can this change be added in a backward compatible manner in 2.x line?

We are evaluating if this change warrants a 3.0 release or can be included in 2.x line so need your inputs.

We will make this feature disabled for 2.x (plan to release in 2.13.0). Thus it won't be a breaking change for 2.x.

For 3.0 release, we will make it enabled which will be a breaking change.

cc @seraphjiang

@tianleh
Copy link
Member Author

tianleh commented Feb 10, 2024

We are extracting configuration related logic (not specific to Clickjacking mitigation) to a separate RFC in #5796 Once we implement the application configuration service, we will onboard Clickjacking mitigation on to it. The scope of this current issue will be updated to only contain Clickjacking specific logic.

@tianleh
Copy link
Member Author

tianleh commented Feb 13, 2024

The content of this issue will be greatly updated once we have #5796 implemented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal RFC Substantial changes or new features that require community input to garner consensus.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants