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

[WIP] Prevent iframe embedding by default #8807

Closed

Conversation

eternoendless
Copy link
Member

@eternoendless eternoendless commented Feb 26, 2018

Questions Answers
Branch? 1.7.3.x
Description? Currently it's possible to embed a PrestaShop instance in an iframe, which exposes it to click-jacking attacks. This PR makes iframe embedding from another domain disabled by default.
Type? bug fix
Category? CO
BC breaks? no
Deprecations? no
Fixed ticket? http://forge.prestashop.com/browse/BOOM-4917
How to test? Put your shop's URL here: http://savanttools.com/test-frame/

Many thanks to Arif Khan for having pointed this out!


This change is Reviewable

@eternoendless eternoendless added this to the 1.7.3.1 milestone Feb 26, 2018
@LittleBigDev
Copy link
Contributor

:lgtm:


Reviewed 1 of 1 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@LittleBigDev
Copy link
Contributor

Rebase needed

@kpodemski
Copy link
Contributor

hey @eternoendless

what about all demos on Addons? :P

@eternoendless
Copy link
Member Author

eternoendless commented Mar 14, 2018

Oh thanks @kpodemski I hadn't thought of that

🤔

Maybe we should add an option to configure the allowed domains somewhere in the BO.

@eternoendless eternoendless removed this from the 1.7.3.1 milestone Mar 14, 2018
@mickaelandrieu mickaelandrieu added Bug Type: Bug 1.7.x Branch labels Apr 3, 2018
@mickaelandrieu mickaelandrieu added 1.7.3.x Branch and removed 1.7.x Branch labels May 7, 2018
@jolelievre
Copy link
Contributor

has this been forgotten? or is it purposely ignored?

@pehelwan
Copy link

pehelwan commented Aug 21, 2018 via email

Copy link

@payloadartist payloadartist left a comment

Choose a reason for hiding this comment

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

The CSP and X-Frame-Options headers successfully block this attack from happening.

@jolelievre
Copy link
Contributor

yes but from what I understand it is not that simple as the demos on Addons need to display PrestaShop in a frame, so we can't block it all the time

@pehelwan
Copy link

pehelwan commented Aug 21, 2018 via email

@pehelwan
Copy link

pehelwan commented Aug 21, 2018 via email

@jolelievre
Copy link
Contributor

Yes, I think that's what @eternoendless suggested, it should be configurable it the BO
Maybe the default value would be to block any frame, but this could be overriden by a specified allowed domain
I lack knowledge about the addons platform actually, so I am not totally sure

@pehelwan
Copy link

pehelwan commented Aug 21, 2018 via email

@@ -2218,6 +2218,10 @@ public static function generateHtaccess($path = null, $rewrite_settings = null,
fwrite($write_fd, "# .htaccess automaticaly generated by PrestaShop e-commerce open-source solution\n");
fwrite($write_fd, "# http://www.prestashop.com - http://www.prestashop.com/forums\n\n");

// prevent click-jacking
fwrite($write_fd, "Header append Content-Security-Policy \"frame-ancestors 'self';\"\n");
fwrite($write_fd, "Header append X-Frame-Options SAMEORIGIN\n\n");
Copy link
Member

Choose a reason for hiding this comment

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

This should be surrounded by <IfModule mod_headers.c>, which already exist in this .htaccess.

Please move these lines further in the method, as all environment does not have the "header" module enabled (for instance, the docker images).

@Quetzacoalt91 Quetzacoalt91 changed the base branch from 1.7.3.x to develop November 26, 2018 10:17
@Quetzacoalt91 Quetzacoalt91 added Waiting for author Status: action required, waiting for author feedback develop Branch and removed 1.7.3.x Branch labels Nov 26, 2018
@Quetzacoalt91 Quetzacoalt91 changed the title Prevent iframe embedding by default [WIP] Prevent iframe embedding by default Nov 26, 2018
@prestonBot prestonBot added the 1.7.3.x Branch label Nov 26, 2018
@molearnik
Copy link

molearnik commented Dec 17, 2018

I'd strongly recommend to think about adding "Security Enhancements" link to e.g. BO/Configure/Advanced Parameters/Security Enhancements where Administrator could enable HTTP response headers like:

X-Frame-Options: SAMEORIGIN
X-Content-Type-Options: nosniff 
X-XSS-Protection: "1; mode=block" 
Strict-Transport-Security "max-age=31536000; includeSubDomains" 
Cache-Control: no-cache='set-cookie'

Vulnerability scanners complain about omission of these headers by default.
Sure, PrestaShop Administrators could do that by tweaking their web servers configurations.
However,

  1. That requires some technical knowledge and it is less convenient
  2. .htaccess mechanism is not recommended for performance reasons (https://www.nginx.com/resources/wiki/start/topics/examples/likeapache-htaccess/#)
    Q: Would it be possible to add these headers via PrestaShop hooks like actionOutputHTMLBefore?
  3. Adding these optional headers from PrestaShop code would be cleaner and web server agnostic.
  4. You could turn this into advantage over competition.
  5. You could add additional security enhancement to the prestashop pages e.g. this script fragment:
    https://www.owasp.org/index.php/Clickjacking_Defense_Cheat_Sheet#Best-for-now_Legacy_Browser_Frame_Breaking_Script

Regarding demonstrating AddOns: PrestaShop websites used for demonstration purposes could have some security options disabled (should the "X-Frame-Options: SAMEORIGIN" break the demo).

@marionf
Copy link
Contributor

marionf commented Oct 22, 2019

@eternoendless

Does this PR is still relevant ?

@Progi1984 Progi1984 added the WIP Status: Work In Progress label Dec 6, 2019
@matks
Copy link
Contributor

matks commented Apr 8, 2020

@PierreRambaud I think we should update this PR in the scope of our security reinforcement for PrestaShop

@PierreRambaud
Copy link
Contributor

@PierreRambaud I think we should update this PR in the scope of our security reinforcement for PrestaShop

Woot didn't see this notification.
I'm not sure it should be handle by PrestaShop installation, but why the host configuration. Otherwise we will not be able to use demo.prestashop.com anymore 🙄

@eternoendless
Copy link
Member Author

Like I said before:

Maybe we should add an option to configure the allowed domains somewhere in the BO.

I'm closing this PR out of staleness, but @PierreRambaud as @matks said, this is something that could be done as a security feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.7.3.x Branch Bug Type: Bug develop Branch Waiting for author Status: action required, waiting for author feedback WIP Status: Work In Progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet