Skip to content

Conversation

AlexMaxHorkun
Copy link
Contributor

Problem

CSP not implemented in Magento

Solution

Implement CSP in Magento in a backward compatible way and allow extension developers to configure it

Requested Reviewers

@joni-jones @paliarush @melnikovi

@AlexMaxHorkun AlexMaxHorkun mentioned this pull request Jul 8, 2019
@DrewML
Copy link
Contributor

DrewML commented Jul 8, 2019

To support a (useful) CSP in the current state of Magento, we'd have to allow unsafe-inline and unsafe-eval, because:

  • Magento relies on (and has encouraged) inline script tags, so they're all over both core and 3rd party extensions
  • Magento's templating variable replacement and bundling features both use eval (awful, awful, awful, but that's the state of things).

Those 2 escape hatches effectively nullify (most of) the value of a CSP. (Opinion) Any plan for a CSP in core should include a plan for tackling those problems.

@DrewML
Copy link
Contributor

DrewML commented Jul 8, 2019

Magento will provide a default endpoint to receive these reports.
Since there is no way to authenticate a genuine report and on a live store they can fill up quickly the number of
reports stored in the database will be limited to 10000 deleting old ones when the limit is reached.

I'm not sure if this strategy will provide value as a default. As an attacker, it shouldn't be hard to spam the endpoint with dummy data 10k times to hide more nefarious activity in the logs. Developers can obviously customize this default, but good defaults are important for a security feature.

Imagine an attacker running this code locally (pseudo-code):

for i = 1 to 10000:
   http.post('${magento-store-url}/csp-endpoint', fakePayload)
   ## logs of any prior bad activity are now purged
end for

@AlexMaxHorkun
Copy link
Contributor Author

To support a (useful) CSP in the current state of Magento, we'd have to allow unsafe-inline and unsafe-eval, because:

  • Magento relies on (and has encouraged) inline script tags
  • Magento's templating variable replacement and bundling features both use eval (awful, awful, awful, but that's the state of things).

Those 2 escape hatches in CSP effectively nullify the value of a CSP. Any plan for a CSP in core should include a plan for tackling those problems.

I agree that our need to allow unsafe-inline and unsafe-eval reduces CSP usefulness but it would still help mitigate certain types of XSS attacks laying the groundwork for further adoption of more strict policies

@AlexMaxHorkun
Copy link
Contributor Author

Magento will provide a default endpoint to receive these reports.
Since there is no way to authenticate a genuine report and on a live store they can fill up quickly the number of
reports stored in the database will be limited to 10000 deleting old ones when the limit is reached.

I'm not sure if this is a good default. As an attacker, it shouldn't be hard to spam the site with automated browser instances and trigger 10k events to hide more nefarious activity in the logs. Developers can obviously customize this default, but good defaults are important for a security feature.

The proposed limit is only there to limit the storage usage and prevent DoS attacks. It is assumed that developers that care about CSP will use restricted mode instead of report-only mode so there would be no incentive for an attacker to flood the reports to hide their attacks since their attacks would be blocked by CSP anyway.

@YevSent
Copy link
Contributor

YevSent commented Jul 9, 2019

The proposed limit is only there to limit the storage usage and prevent DoS attacks. It is assumed that developers that care about CSP will use restricted mode instead of report-only mode so there would be no incentive for an attacker to flood the reports to hide their attacks since their attacks would be blocked by CSP anyway.

Enabling SCP in restricted mode depends on not only merchants and their SI but on 3rd party extensions installed on merchant's websites as they should be updated to support SCP before enabling restricted mode.

we set event listener via attributes and we use eval in template.js so we'd have to also allow
'unsafe-inline' and 'unsafe-eval'. Magento has integrations with multiple 3rd party services like vimeo, youtube,
google analytics and various payment systems which we must whitelist additionally.
That will be the default whitelist Magento provides out of the box packaged alongside related modules via _csp\_whitelist.xml_.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each module should provide own scp_whitelist.xml for 3rd party resources.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that is what I meant


CSP API/SPI will consist of the following interfaces:

* Policy DTO. The interface has only policy ID because not all CSP policies look like _\<policy name\> \<value\> \<value\>_,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you provide some details about different policy formats? I did not find any information about it in RFC. Or are you talking about serialized SCP?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

examples would include plugin-types, report-to, sandbox and some other policies that either require additional headers to work or at least accept different values than URIs and preserved 'self'/'unsafe-inline' words like *-src policies do

Copy link
Contributor

@YevSent YevSent Jul 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably I'm not clear understand but plugin-types, report-to, sandbox have the same format <policy name/> <value/> <value/>;

Copy link
Contributor Author

@AlexMaxHorkun AlexMaxHorkun Jul 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got confused, thought the question was related to the limitation of available policies through csp_whitelist.xml - for that file allowed options for values are important as well. report-to requires additional header and Subresource Integrity may involve Access-Control-Allow-Origin header to be implemented

}
```

* A repository of violations logs collected when CSP reporting is enabled
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to think how to make reporting works. I don't think we can't use Web API to capture the data, it would be just too much load. May need to build a separate service for capturing reports data.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can use a regular controller as the report endpoint for browsers if our web API mechanism would be difficult to fit for this

@YevSent
Copy link
Contributor

YevSent commented Jul 10, 2019

AngularJs provides a possibility to define SCP on a view level (directly in HTML), maybe this approach would allow us to avoid global usage of unsafe-inline and unsafe-eval?

@AlexMaxHorkun
Copy link
Contributor Author

The proposed limit is only there to limit the storage usage and prevent DoS attacks. It is assumed that developers that care about CSP will use restricted mode instead of report-only mode so there would be no incentive for an attacker to flood the reports to hide their attacks since their attacks would be blocked by CSP anyway.

Enabling SCP in restricted mode depends on not only merchants and their SI but on 3rd party extensions installed on merchant's websites as they should be updated to support SCP before enabling restricted mode.

I understand that but if a merchant wants to enable CSP they can compensate for unsafe extensions by whitelisting resources those extension use themselves or make other changes to active policies

@AlexMaxHorkun
Copy link
Contributor Author

AngularJs provides a possibility to define SCP on a view level (directly in HTML), maybe this approach would allow us to avoid global usage of unsafe-inline and unsafe-eval?

there is no way to use only certain policies for a portion of a page. If in the future we disable 'unsafe-inline' and a 3rd party-developer would need to reenable it for a specific page they would be able to do so with CSPAwareActionInterface

@ericerway
Copy link

@zetlen can we make sure this is incorporated into our approach for PWA? Better to do this sooner than later for the product since this affects content. Thanks!

@DrewML
Copy link
Contributor

DrewML commented Jul 10, 2019

re: whitelisting, Google has a research paper where they analyzed and documented the effectiveness of deployed CSPs on the web.

I'm not finished reading through it yet (but will be :D), but this quote in the intro stood out:

Based on the results of our study, we conclude that maintaining a secure whitelist for a complex application is infeasible in practice

https://static.googleusercontent.com/media/research.google.com/en//pubs/archive/45542.pdf

Note that the alternative proposed in the paper's intro (strict-dynamic + nonces) is part of CSP Level 3, so I'd recommend looking into the feasibility of an implementation using that instead of whitelists.

@AlexMaxHorkun
Copy link
Contributor Author

re: whitelisting, Google has a research paper where they analyzed and documented the effectiveness of deployed CSPs on the web.

I'm not finished reading through it yet (but will be :D), but this quote in the intro stood out:

Based on the results of our study, we conclude that maintaining a secure whitelist for a complex application is infeasible in practice

https://static.googleusercontent.com/media/research.google.com/en//pubs/archive/45542.pdf

Note that the alternative proposed in the paper's intro (strict-dynamic) is part of CSP Level 3, so I'd recommend looking into the feasibility of that.

that is an interesting point. This document is more focused on introducing CSP to Magento and allowing merchants/developers to configure policies, actual policies supplied by default with core Magento are up for debate

@paliarush paliarush self-assigned this Jul 16, 2019
@nathanjosiah
Copy link
Contributor

To support a (useful) CSP in the current state of Magento, we'd have to allow unsafe-inline and unsafe-eval, because:

  • Magento relies on (and has encouraged) inline script tags, so they're all over both core and 3rd party extensions
  • Magento's templating variable replacement and bundling features both use eval (awful, awful, awful, but that's the state of things).

Those 2 escape hatches effectively nullify (most of) the value of a CSP. (Opinion) Any plan for a CSP in core should include a plan for tackling those problems.

I don't think this comment has received enough attention. As it stands now, if the user has to enable these options and effectively open a huge hole in the security (and therefore the purpose) of this feature, is it worth implementing now without a solid plan to go with it?

@AlexMaxHorkun
Copy link
Contributor Author

Added a bit about strategy going forward and whitelisting with nonce

@AlexMaxHorkun
Copy link
Contributor Author

@DrewML @joni-jones do you have any objections?

@nathanjosiah
Copy link
Contributor

Even with the added changes to the plan moving forward, I still have concerns about the false sense of security that this feature would provide in its current state. I understand that something is better than nothing in many cases but I think this would leave users way more vulnerable than they would feel they are. A large number of the security bugs that have been resolved in the current security remediation effort were related to eval. If we are knowingly using vulnerable libraries and the desire is to avoid the vulnerable feature, we should have an active plan to mitigate the risk.

We deprecated the entire Magento Backups feature because of similar concerns. We shouldn't be in such a rush to push a feature forward that we compromise on security especially when that is the entire point of the changes.

As a side note, I think it would be good to have the security team review security-related PR's as well as they may have some existing research to share.


It is possible to combine nonce and domain-list based whitelisting for CSP.

Magento will have to generate unique nonce (using _Magento\Framework\Math\Random_) for each response and developers will
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is Magento/Framework/Math/Random cryptographically secure?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, at the core it uses random_int which is.

To make it easier for developers and don't force them to include the nonce-helper to all of their blocks it will
be available inside _.phtml_ templates via local `$nonce` variable.

Additional effort will have to go to ensure proper nonce used/regenrated when page cache is enabled. Perhaps blocks
Copy link
Contributor

@DrewML DrewML Jul 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to spend more time on this section before merging this.

Additional effort will have to go to ensure proper nonce used/regenrated when page cache is enabled

Magento only has good performance in production with Varnish, and the nonce must be delivered in the response. If we were to release this without a full solution for caching, most merchants will take the speed and security they have today, rather than more security and massive performance regressions.

We need a plan for how we're going to deliver a unique nonce per request without disabling Varnish.


## Future steps
To allow merchants to use whitelisting with _nonce_ we have to get reed of event handlers via HTML attributes and
style attributes in our templates. There is no way to disable `unsafe-eval` since we use it for UI components and
Copy link
Contributor

@DrewML DrewML Jul 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no way to disable unsafe-eval since we use it for UI components and
some of the front-end libraries we employ need it (like jQuery).

There 100% is a way to do this, it's just a breaking change. If the work is scheduled, it can be done

Copy link
Contributor

@nathanjosiah nathanjosiah Jul 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I stated in my comment above referring to this exact line

If we are knowingly using vulnerable libraries and the desire is to avoid the vulnerable feature, we should have an active plan to mitigate the risk.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getting reed of eval() in Magento front-end is a separate topic, obviously it has to be done but describing how would take another document.

style attributes in our templates. There is no way to disable `unsafe-eval` since we use it for UI components and
some of the front-end libraries we employ need it (like jQuery).

The work on refactoring templates to remove event handlers and _style_ attributes will be done after CSP is introduced
Copy link
Contributor

@DrewML DrewML Jul 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The work on refactoring templates to remove event handlers and style attributes will be done after CSP is introduced
gradually

This feels like the wrong order to me. How can we expect merchants to find the time to update their stores for CSP if we, Magento, can't find the time to do it first?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A good example of this is the @escapeNotVerified security work done several years ago.

https://magento.stackexchange.com/questions/92963/magento-2-escapenotverified

We still haven't completed this work in core, and it's been > 4 years.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still haven't completed this work in core

Not to take away from your point, we just did it in the last few months. It will be delivered in the next patch releases.

To actually add to your point, it took the work of ALL core teams over 2 months of solid work to properly implement and test those changes. Totally unrealistic effort level for a typical merchant.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

even with inline styles/JS present in templates we can utilize origin whitelisting for remote resources

@DrewML
Copy link
Contributor

DrewML commented Jul 22, 2019

Because any work involving security is super important and super high impact, I'm going to list 2 requirements that I think are critical before this plan gets approved:

  1. Review/approval from a member of the security team
  2. A list of a few attack vectors that show that the proposed plan for CSP in Magento core would have prevented any of them (most I've gone through so far would not have been solved by this proposal)

@piotrekkaminski
Copy link

Couple other things:

  • i'm still waiting for info from Fastly if they could support nonces. Seems Varnish has a module to do that, but adding a regex parsing into Varnish might bring performance issues. https://docs.varnish-software.com/varnish-cache-plus/vmods/xbody/#javascript-csp-nonce

  • @szurek and @chandramadobes please review and comment

  • I'm pretty strongly against creating reporting functionality in Magento. We should integrate with report-uri.com which has a nice free plan and reasonable commercial plans. This will reduce project complexity and report handling is not a core commerce feature. We could consider some cooperation with them for cloud (and include XSS Auditor as well)

  • agree with @DrewML - it has to prove being able to stop typical attack (card skimmer loaded from weird site or inline JS directly in MiscHTML field

@zetlen
Copy link
Contributor

zetlen commented Jul 22, 2019

For PWA Studio, I've created magento/pwa-studio#1453 to track our own progress. We don't have legacy code to migrate, so our job here will be much easier. @ericerway The rest of my comment will be about the more general case.

I agree with other commenters that a CSP with unsafe-eval in it is worse than nothing, because it leads to a false sense of security. I also agree that relying on empirical reporting to collect a list of exploit paths is an unreliable mechanism. It's better to enable the CSP only once the site is fully migrated.

However, we shouldn't wait to start building the "plumbing" for a CSP as this PR describes. That work has to happen anyway, and we don't need to advertise improved security while we're building the prototype.

We should support admin-configurable whitelisting. Whitelisting is less secure than nonce or hash, but as long as unsafe-eval and unsafe-inline are turned off, whitelisting is a measurable improvement which we can demonstrate. But it must be controllable by the system administrator, not assembled by merging extension configuration. It needs to be visible--if an admin doesn't recognize an approved domain, that's already a problem.

Once the plumbing is built, we should create a microsite that demonstrates some core Magento 2 functionality, like a single-product checkout. We should cut scope and functionality until it works with a properly secure CSP (no unsafe-eval or unsafe-inline. Then we should demonstrate its resistance to attack vectors.

Then we should iteratively add functionality to the microsite, fixing the script blocks where necessary.

Question for @DrewML, @piotrekkaminski, @AlexMaxHorkun , @nathanjosiah and others: Nonce-based whitelisting is expensive and potentially a cache buster. But what about hash-based whitelisting? https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/script-src#Unsafe_inline_script Hashes can be generated at static deploy time, and they are guaranteed not to allow unexpected code to pass through.


CSP mode will be set with a config path not appearing in the admin configuration page so merchants
would have to execute a CLI command in order to change it.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds great:
It would protect the settings against changes via Admin (admins, hacked accounts, admin laptops with malware, ...). Only Dev/DevOps level could change it.
Is there a way to protect the configuration against badly written extensions, too?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any possible problems with that on Magento Cloud? Related to read-only file system.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would need to be written to the env.php file during the build process before deployment completes.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the hash comment is very good. can it be pre-generated once during deployment or the inline scripts are dynamic - if the latter it might cause caching issues too? definitely seems like an easier approach than nonces.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@piotrekkaminski I've added a bit about hashes

_Magento\Framework\View\Page\Config\Renderer_ will add nonce to all css/js assets being added to a page. With _nonce_
disabled the presentation class used inside templates will not add nonce when called and _Renderer_ will not do it as well.

To make it easier for developers and don't force them to include the nonce-helper to all of their blocks it will
be available inside _.phtml_ templates via local `$nonce` variable.
Additional effort will have to go to ensure proper nonce used/regenrated when page cache is enabled. It is possible to
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it would be good to have a more concrete understanding that this can actually be done. As stated by @DrewML Varnish support is integral to the success of this feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nathanjosiah this helper will be used for hashes/dynamic origin whitelisting depending on the CSP mode selected by merchant so we'd have to have it. There is no conflict between hashes/whitelisted origins and using varnish

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually, Varnish stores the gzipped content in the cache. It means that Varnish will be forced to decompress every cache entry, modify it and compress again for each HTTP request. This will definitely consume more system resources.

Also, we need to support third-party caching providers like Fastly.

Thus, please expand this section with a strategy for third-party providers and another impact on env, setup, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using nonce alongside page cache doesn't seem viable right now, updated the section

@paliarush paliarush assigned kokoc and unassigned paliarush Aug 5, 2019
There is no way to whitelist an event handler defined via an attribute though (like _onclick_) or styles defined
with _style_ attribute that's why nonce will be
disabled by default and all `unsafe-inline` scripts/styles will be allowed. Merchants who'll be successful in getting
reed of all event handlers/styles defined using HTML attributes will be able to enable _nonce_ via configuration
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spelling, reed = rid, multiple occurrences in the document.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

_Magento\Framework\View\Page\Config\Renderer_ will add nonce to all css/js assets being added to a page. With _nonce_
disabled the presentation class used inside templates will not add nonce when called and _Renderer_ will not do it as well.

To make it easier for developers and don't force them to include the nonce-helper to all of their blocks it will
be available inside _.phtml_ templates via local `$nonce` variable.
Additional effort will have to go to ensure proper nonce used/regenrated when page cache is enabled. It is possible to
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually, Varnish stores the gzipped content in the cache. It means that Varnish will be forced to decompress every cache entry, modify it and compress again for each HTTP request. This will definitely consume more system resources.

Also, we need to support third-party caching providers like Fastly.

Thus, please expand this section with a strategy for third-party providers and another impact on env, setup, etc.

@piotrekkaminski
Copy link

piotrekkaminski commented Aug 13, 2019

Fastly responded with a proof of concept of handling nonces through an ESI, however it would be best to use checksums or something similar instead of ESI. I assume the nonce/checksum doesn't have to change until there is a cache update (due to new code/changes being inserted on a page).

# INIT
# Generate nonce to be used in CSP and script tag
declare local var.nonce STRING;
set var.nonce = digest.base64(req.xid);
set req.http.x-nonce = var.nonce;

# Generate synthetic for filling in nonce
if (req.url ~ "900") {
  error 990;
}

# Disable compression for ESI handling
unset req.http.Accept-Encoding;

#FETCH
# Turn on esi
if (req.url ~ "/status/200") {
  esi;
}

#DELIVER
# Set CSP with nonce value generated in recv
if (req.esi) {
  set resp.http.Content-Security-Policy = "script-src 'nonce-"req.http.x-nonce"'";
}

#ERROR
# fill in esi tag with nonce value generated in recv
if (obj.status == 990 ) {
  set obj.http.Content-Type = "text/html";
  set obj.status = 200;
  set obj.response = "OK";
  synthetic {""}req.http.x-nonce{""};
}

@AlexMaxHorkun
Copy link
Contributor Author

Fastly responded with a proof of concept of handling nonces through an ESI, however it would be best to use checksums or something similar instead of ESI. I assume the nonce/checksum doesn't have to change until there is a cache update (due to new code/changes being inserted on a page).

# INIT
# Generate nonce to be used in CSP and script tag
declare local var.nonce STRING;
set var.nonce = digest.base64(req.xid);
set req.http.x-nonce = var.nonce;

# Generate synthetic for filling in nonce
if (req.url ~ "900") {
  error 990;
}

# Disable compression for ESI handling
unset req.http.Accept-Encoding;

#FETCH
# Turn on esi
if (req.url ~ "/status/200") {
  esi;
}

#DELIVER
# Set CSP with nonce value generated in recv
if (req.esi) {
  set resp.http.Content-Security-Policy = "script-src 'nonce-"req.http.x-nonce"'";
}

#ERROR
# fill in esi tag with nonce value generated in recv
if (obj.status == 990 ) {
  set obj.http.Content-Type = "text/html";
  set obj.status = 200;
  set obj.response = "OK";
  synthetic {""}req.http.x-nonce{""};
}

If I understand correctly what they propose is for Varnish to dynamically replace a certain placeholder for every response but that is not secure since nothing will stop attackers from using the placeholder in their XSS attacks and have Varnish conveniently whitelist their scripts. This problem may be solvable but I think we should proceed without nonce-whitelisting. If we do to decide to include it later it will be just a matter of changing the configuration for merchants.

@chandramadobes
Copy link

Approved with future roadmap to address:

  1. unsafe-eval and inline scripts
  2. change from report only to strict mode.
    Phased approach towards implementing CSP completely starting with report only mode.

@kokoc kokoc merged commit 9c0378b into magento:master Aug 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.