As an admin user I can create states at admin/structure/registration/registration_states that include javascript (or other code) which is then executed in the browser of users who visit registration_registrations_page
The same code executes when viewing an individual registration at registration/%registration in registration_view which just uses the entity buildContent function.
The attached patch fixes this by using filter_xss_admin to allow in some HTML (e.g. bold or italics). This feels appropriate since the label can only be modified by someone with "administer registrations" permission.
| Comment | File | Size | Author |
|---|---|---|---|
| registration_xss_fix.patch | 1.18 KB | greggles |
Comments
Comment #1
ezra-g commentedMarking as "needs review".
Comment #2
levelos commentedCommitted, thanks guys.
Comment #4
mrfelton commentedI think this probably should have been marked as a security release, as it has security implications.
Comment #5
gregglesI could go either way on whether it needs a "security update" tag on the release node. It only allowed admins with "Administer registration states" permission to inject xss - that permission is marked as "restrict access" => TRUE so it gives a big warning in the permissions page saying it should only be granted to highly trusted people. On top of that this is a beta module, so there is an implied state of "each new release will contain critical bugfixes and people should update fast."
@levelos - if you want the security update tag please comment here and I can do it for you (only users with certain d.o permissions can do it).
Comment #6
levelos commentedI appreciate the consideration and am happy to have the tag added if it would benefit the community. I'm personally ambivalent, although seems like water under the bridge at this point ....
Comment #7
gregglesThere's ~400 users who are using an old version. Adding the security update tag would give them a red warning encouraging them to update.
As I write this I'm remembering that when an admin permission (i.e. with restrict access) is a mitigating factor that we typically discourage the use of the security update tag so we don't create unnecessary "noise" for site admins. So...yeah, I'm now opposed to the idea of adding the tag.