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.

CommentFileSizeAuthor
registration_xss_fix.patch1.18 KBgreggles

Comments

ezra-g’s picture

Status: Active » Needs review

Marking as "needs review".

levelos’s picture

Status: Needs review » Fixed

Committed, thanks guys.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

mrfelton’s picture

I think this probably should have been marked as a security release, as it has security implications.

greggles’s picture

I 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).

levelos’s picture

I 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 ....

greggles’s picture

There'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.

  • levelos committed fc0ff8c on 7.x-1.x, panels, any-entity, slots, integrations, hold_state
    #1661428 by greggles: Insufficient filtering of registration states.