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

Sudo mode for sensitive data in the CMS #1898

Closed
5 tasks done
emteknetnz opened this issue Feb 10, 2025 · 8 comments
Closed
5 tasks done

Sudo mode for sensitive data in the CMS #1898

emteknetnz opened this issue Feb 10, 2025 · 8 comments

Comments

@emteknetnz
Copy link
Member

emteknetnz commented Feb 10, 2025

Sudo mode requires authenticated users to re-enter their password when dealing with sensitive data. This provides and additional layer of defense against XSS attacks, as well as peoples computers that have been left unattended while in a logged in state.

Investigate requiring all changes on Security related forms to require the use of the 'sudo' mode that the MFA module makes use of. This would cover:

  • Editing the existing Member
  • Creating new Member's
  • Changes to Groups
  • Changes to Roles
  • Any other sensitive data

Related issues

Acceptance criteria

  • Get a better understand the current MFA sudo mode behaviour
  • Come up with some options to prevent XSS attacks from modifying Members, Groups and Roles.
  • (optional) Full implementation - if that's done then create a new public issue minus lots of the detail in this private issue, e.g. no links to security issues, do not mention privilege escalation, simply say "security hardening" instead
  • Talk with whoever is not implementing this about the approach before proceeding too much
  • Have added deactivate to SudoModeServiceInterface and update mfa LoginHandler to use it in CMS 6 - see here

Not doing PRs

Kitchen sink CI

CMS 5 PRs

CMS 6 PRs

@GuySartorelli
Copy link
Member

GuySartorelli commented Feb 13, 2025

Doesn't seem to include protecting access controls for "these users/groups can edit/view" for pages and files, which I remember discussing as something we wanted to protect?
If we're protecting that for SiteConfig, we should also protect it for individual records.

@emteknetnz
Copy link
Member Author

emteknetnz commented Feb 13, 2025

Doesn't seem to include protecting access controls for "these users/groups can edit/view" for pages and files

I had a go at that in the PR I listed under "Not doing PRs" above - silverstripe/silverstripe-cms#3053. I've put this in the too hard for now basket.

If we're protecting that for SiteConfig, we should also protect it for individual records.

They're quite different, as SiteConfig is just a large single form so it's easy to protect, whereas as "these users/groups" and a group of fields on an otherwise unprotected form.

IMO protection for user/groups access on pages/files is quite a lot lower priority then protection for creating/editing/deleting member/group records themselves.

@kinglozzer
Copy link
Member

If we’re rolling this out more widely, is it worth a discussion around the terminology of this feature on the frontend? “Sudo” is not a term any of our clients would understand

@emteknetnz
Copy link
Member Author

emteknetnz commented Feb 16, 2025

I've mentioned here that I'm not keen to make this a frontend feature, at least not in this issue.

I agree that "Sudo" isn't a particularly great terminology because it's a linux thing, and not something that CMS users would understand, however it is the existing terminology. It's worth noting that the word "sudo" does not appear anywhere in UI, it's pretty much just a docs + code thing, which is aimed at devlopers. I think for these reasons we should just leave the "sudo" terminology as is for now

@GuySartorelli
Copy link
Member

I think by "frontend" Loz just means "the bit the user will see"

@GuySartorelli
Copy link
Member

A lot of my review comments in PRs other than the framework one were ignored - can you please look at all of them? It'll make the review cycle faster if we don't do one PR at a time.

@GuySartorelli
Copy link
Member

CMS 5 PRs merged, assigning to Steve for CMS 6 stuff

@GuySartorelli
Copy link
Member

PRs merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants