There are several places where information collected from supporters is displayed without being run through check_plain or filter_xss first - either to other users through tags like [cc-c-email_standard_canada:supporters:last:field_citcon_last_name] or in the Citizens Connected dashboard.

I quickly tested that it's not filtering anywhere by successfully injecting link spam in the last visitor name fields, and I'm pretty sure that I could build a cross-site scripting exploit with only a small amount of work. (It put

tags in the generated source, but there's some jquery weird that's getting my initial alert tags not called. But it's including the tags, and from there it's just knowing jquery better than I do to make it work.) I can do a run through these as well - for all of the user fields, they should just be converted to plain text with the check_plain function, since I don't think we would want to allow non-text content for any of the user fields. (Thoughts on that, though?)

Comments

sebsebseb123’s picture

sebsebseb123 commented

Yes yes... this is bad... creating a patch.

sebsebseb123’s picture

sebsebseb123 commented

I've added a patch to add filtering at a few places... this should cover the worst of it... but, wondering what other exploits you've found? You mention "several places".

Also, the patch cleans up some whitespace and removes some commented debug code :S

sebsebseb123’s picture

sebsebseb123 commented
Status: Active » Needs review
damien_vancouver’s picture

damien_vancouver commented
Assigned: ian clysdale » damien_vancouver
Priority: Major » Critical
Status: Needs review » Needs work

Confirmed that this bug is still active and critical in 7.x-1.x-dev with these steps:

  • Edited the body of my campaign to contain some user inputted tokens:
    [cc-c-citizens_connected_full:supporters:last:field_citcon_first_name] [cc-c-citizens_connected_full:supporters:last:field_citcon_last_name]
  • Signed the petition with a first name containing an HTML hyperlink, and a last name containing some javascript (<script language="Javascript">window.alert('hi');>/script<).
  • Viewed the petititon again with the view tab. A javascript window saying "hi" pops up and the link is reproduced verbatim on the campaign page.

This is obviously a critical security issue - only -raw tokens should be returning raw data like this, and other stuff should be using safe_value or manually running through check_plain or filter_xss. Assigning this to me and marking it critical.

damien_vancouver’s picture

damien_vancouver commented
Priority: Critical » Normal
Status: Needs work » Needs review
Issue tags: +citcon-sprint

We found the missing patches From Seb and I've gone through all the token code with a fine tooth comb and added a bunch more validation. The fixes are committed to 7.x-1.x-dev in cb7216f.

However I noticed that if I put an HTML link in my "First Name" field when signing the petition it is still not filtered on the token. That is because we are filtering the value fields with filter_xss() and the fields themselves appear to be accepting HTML. They should maybe be plain text? Setting to needs review until we figure that out.

mgifford’s picture

mgifford
he/him
Primary language English
commented

@damien shouldn't that be listed as Needs Work then? There's no patch to review, but good to hear the patches didn't get lost.

EDIT: Mostly just checking on the status of things here and identifying loose ends.

damien_vancouver’s picture

damien_vancouver commented
Assigned: damien_vancouver » Unassigned
Issue summary: View changes
Status: Needs review » Needs work

@mgifford Correct, it;s a new task so should have been back to "needs work". I've corrected that unassigned it to me in case someone else can get this fixed first.

To close this issue off all the way, we need to verify that fields in the supporter types that are set to plain text are not displaying as HTML when they are output, including in tokens.

mgifford’s picture

mgifford
he/him
Primary language English
commented

Thanks. Hopefully we'll be able to help push this ahead in the coming months.