Skip to content
This repository has been archived by the owner on Apr 10, 2023. It is now read-only.

Are the csrf patches still needed on Plone 4.3.8? #19

Open
mauritsvanrees opened this issue Mar 3, 2016 · 4 comments
Open

Are the csrf patches still needed on Plone 4.3.8? #19

mauritsvanrees opened this issue Mar 3, 2016 · 4 comments

Comments

@mauritsvanrees
Copy link
Member

I think I know the answer to this one, but let's ask it anyway. We can point others with the same question to this issue.

The biggest thing that plone4.csrffixes does, is it adds a dependency on plone.protect 3.x. Is it possible to ignore the plone4.csrffixes package on Plone 4.3.8 and in your buildout only add a version pin for plone.protect 3.x?
Let's see.

The pending Plone 4.3.8 has several updated packages, which means that most patches in plone4.csrffixes are no longer needed. I have checked all patches in patches.py and they are no longer needed as they have been included in the relevant packages. The patches then have the effect that alsoProvides(request, IDisableCSRFFixes) is called twice, but that is fine. The superfluous patches are not harmful.

What is left, is the added transform. It blesses a few read-on-writes, adds protect.js, and protects ZMI urls. Without this, I at least see some errors in the logs for scales created when adding an image in TinyMCE, but everything seems to go fine anyway. The ZMI works fine as far as I see, but I may not be testing the 'right' forms or links.

So I think the answer is: if you want the full csrf protection on Plone 4.3.8, updating plone.protect to the latest 3.x will work, but to avoid some corner cases you should still add plone4.csrffixes.

And even with that package, there may be other corner cases in Plone, and certainly in add-ons, where you will still get the confirmation page warning you about a possible csrf attack.

@mauritsvanrees
Copy link
Member Author

Current answer: yes they are still needed.
I have just released plone4.csrffixes 1.1, which depends on plone.protect 3.0.19 or higher, which inserts the protect.js file that was previously in plone4.csrffixes.
The only thing that is then still missing in plone.protect 3, is plone/plone.protect#48
If that gets merged and a new plone.protect 3.x released, then that plone.protect version would be enough, and you would no longer need plone4.csrffixes.

But: the versions.cfg of Plone 4.3 on dist.plone.org will always keep pointing to plone.protect version 2.x. The full csrf fixes will never be included in 4.3 core. You will need plone.protect 3.x or need to block ZMI access, like described here: https://old.plone.org/products/plone/security/advisories/security-vulnerability-20151006-csrf

@mauritsvanrees
Copy link
Member Author

I am reopening this question. Several fixes have been merged to plone.protect. Is plone4.csrffixes still needed? I have updated the docs with my latest knowledge: 78f5da1.

Let's repeat the most important part here in this comment.

If you use Plone 4.3.8 or higher with plone.protect 3.0.21 there is not much that would still need plone4.csrffixes. It may still help avoid a few confirmation pages, because it has this code which is extra:

  1. It checks the referer. If the previous page is within the Plone Site, no cross site checks are done.
  2. If the current page is a ZMI page all links are rewritten to have a CSRF token.
  3. Several other links get the CSRF token appended, for example in the Actions dropdown (Copy, Delete, etcetera).

So this blesses (or grudgingly accepts) lots of code that does a write-on-read, so on a GET request.

Note that the extra code is in the plone4.csrffixes transform, which is executed before the plone.protect transform. So their goal is a bit different.

Questions then are:

  1. Is this extra code really still needed?
  2. Should that code be added in plone.protect 3?

The referer check makes sense to me: if the previous page is on the same site, then by definition it is not a cross site request. The other two then seem unneeded. Or am I missing the point of a csrf check here?

Aha:

The real point may be: are we doing protection against csrf or protection against write-on-read?

And with that point stated, I think it then boils down to:

  1. plone.protect 3 does automatic csrf protection and protection against write-on-read. On Plone 5 we want both.
  2. plone4.csrffixes now only has extra code left to opt out of the write-on-read protection because there is too much code in Plone 4 that does that. That may be even more true for add-ons.

Conclusion:

  1. plone4.csrffixes does not interfere with the csrf checking: you have the same auto csrf protection as with pure plone.protect 3.
  2. plone4.csrffixes accepts most write-on-read situations.
  3. On Plone 4.3.8 or higher It is best to only use plone.protect 3.0.21 or higher. But if don't want to fix too much code, or don't want your users to get so many confirmation pages when simply browsing the site, then you should use plone4.csrffixes.

@mauritsvanrees
Copy link
Member Author

@vangheem What do you think?

@hvelarde
Copy link
Member

hvelarde commented Oct 5, 2016

thanks! FYI @idgserpro

hvelarde added a commit to hvelarde/smdu.portal that referenced this issue Oct 5, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants