Issue details on the advisory

Hello,

Some part of the security issue has been fixed already with the attached patch, if somebody wants to finish he can start with it.

-- Yvan

Added check_plain to encode special characters, should fix XSS.

Should/Can we treat the DOS/coordinate bounds issue as a separate issue and get this reviewed and committed? The bounds problem will be mitigated by the fact that the users needs permissions to edit the coordinates. This way we already have a fix out for the XSS/CSRF vulnerabilities.

Here is a patch for the coordinates DOS issue. It checks the coordinates were not set larger than the current images dimensions before saving.

Apologies wrong patch uploaded here is the working version....

1. +++ b/website/sites/all/modules/contrib/epsacrop/epsacrop.module
   @@ -954,7 +954,24 @@ function _epsacrop_add_file($fid, $coords) {
   +function _epsacrop_save_coords($fid, $coords) {// Check fid is for a valid file.
   

   Does not meet standards, can we move the comment into the function doc?

2. +++ b/website/sites/all/modules/contrib/epsacrop/epsacrop.module
   @@ -954,7 +954,24 @@ function _epsacrop_add_file($fid, $coords) {
   +        || $field_coords['x2'] >= $file->width
   

   That looks better! :)

@Bart that patch looks good.

We can apply both patches and then create a mumma patch with both issues fixed.

@alexfarr can you remove #5?

Updating patch for coordinates, fixes issue with comment and failure of exact coordinate test.

Looks the goods! Well done!

I'm happy to test this as soon as a single patch is created as I've got two production sites currently using this that I have had to disable this module on. I've also got another site that is due for launch in the next two weeks using this where I can easily apply any patches.

I was in the process of switching over to Manual Crop but would rather carry on using this module!

Thanks for all the speedy work so far everyone.

All patches merged.

I got an error trying to apply which highlighted a spelling error

➜  epsacrop git:(develop) ✗ git apply -v  2710653-xss_csrf_dos-12.patch
sites/all/modules/contrib/epsacrop/2710653-xss_csrf_dos-12.patch:54: trailing whitespace.
        // If the json_decode returns null that means we got an invalid JSON or it reachs the
warning: 1 line adds whitespace errors.

Should be "reaches" .

I will try to apply using patch command.

Fixed the spelling error.

Hidden 2710653-xss_csrf_dos-12.patch.

Fixed comments (punctuation & should not be longer than 80 chars).

My editor was a bit too aggressive there...

Can the access checks not be moved to the access callbacks?
Besides that everything looks good, path applies, module keeps working and I was unable to reproduce the XSS or CSRF, did not test the DOS yet.

Last year we used this patch to solve this issue, perhaps it can still be of any use

I'm having an issue in _epsacrop_save_coords, the $file object doesnt have the height and width. Am I missing something?

Moved token validation for the AJAX callback in an access callback as per #19. Patch uses #18 as basis.

@wolfereast: Try this one (based on #22).

+++ b/epsacrop.module
@@ -168,7 +170,7 @@ function epsacrop_dialog($entity_name, $field_name, $bundle, $fid) {
-function epsacrop_ajax($op, $fid) {
+function epsacrop_ajax($op, $fid, $token) {

I think $token should be removed if you moved it to access callback ?

Beat me to the punch @Jelle_s, image_get_info works like a charm. The attached patch is based on the opatch in 22. It adds image_get_info and checks all the field coords instead of just the first.

#24 is right, removed $token in the page callback. Patch based #23.

Updated 26 to test all coordinates passed, not just the first.

@wollereast That patch has an variable miss name error on lines 981 and 982 variable names need to be changed to $info from $file_info in No #27.

Oops, thats what I get for merging too quickly. Updated variable names

Here is a patch which shifts the coordinate data from the current textarea in the dialog and into a urlencoded data- attribute of the wrapping div.

#30 applied cleanly for me.

I think this patch is good to go, security-wise. From a UX point of view #2634692: Coords should only be saved when node gets saved would be a better solution. If coords only get saved on form submit, this problem goes away altogether, but I don't know how much of a rewrite that would be, and I think we should get a security release out ASAP.

+1 on this is ready (security wise), we can do a follow up and fix other issues with the module, but for now making a security release should be minimal solution

Agree with #32 but having tested patch #30 last night it does look good from a security point of view. I hope someone from security team can review this quickly!

Well done all.

I can't vouch for the security aspect of it, but the patch in #30 applied for me and I'm not seeing any issues with the use of the cropping interface or the resulting images after it was applied.

Thanks to all for the work!

@yvmarques, do we need to contact security team and ask them to review this?

I'm wondering if it should be left as 'to review' so security team will see it?

I've emailed Heine to get feedback on how we get this reviewed.

Hello,

I notified the security team on the first place time where we had this discussion.

-- Yvan

+1 for the patch working.

+1 this patch working.

Does anyone know if this effects the 6.x version as well?

Hi @jphelan, no this was for 7.x only.

+1 this patch working.

@yvmarques any word from the security team on the progress of this patch?

@jphelan Most likely the Drupal 6 version of the module is also affected by the security issue, unfortunately due to the fact that Drupal 6 isn't anymore supported, we will not fix the security on that version as well. An issue on d6lts has been open to see if any company wants to fix it. https://www.drupal.org/node/2710755

@alexfarr Not yet, updated the security issue to bring it up.

I emailed Heine on the 25th. No response.

Setting to Needs Review because it may be missed if it is RTBC.

Looks like we need to follow the actions here: https://www.drupal.org/node/101497

I'm not sure how much of that has been done ...

I've just emailed the security team to arrange a review time.

Thanks everybody. The process for re-supporting an unsupported module can be found at https://www.drupal.org/node/251466.

@cbiggins Please create a new security issue by reporting an issue to the security team from the module's homepage and someone from the team will work with you to review and resolve the issue.

Thanks,
David Stoline on behalf of the Drupal Security team

Thank you @dstol!

All other contributors in this issue, I'll create the security issue and work with the security team to get the module cleared with the patch that has been created by you guys.

I'll report updates here.

Hi all

Any update?

L

I can't talk with the security team about the issue because I don't have the permissions on the module.

I've asked Yvan for the access but he has stopped responding to me.

I'm going to try and contact the security team about this again to see if I can get anything moving.

I'll definitely update this issue with any progress.

Would be very nice to see an update on this! Please let us know if we can do anything to help.

@cbiggins I contacted Yvan who mentioned that he's given you permissions now.

Let me know if there's anything I can do to help. Would be great to get this module back on track.

Thanks for you help

Luke

Thanks Luke, will update when I have anything.

Got an update - spoke to the security team member (mlhess) who is looking after the review (not the one who lodged the issue) and they are going to chat about giving me the user role I require to deal with them on the patch.

This is the big hurdle, if we can get over this then we'll be able to have more open communication with the security team on this issue and any others.

Any news?

Not yet, still with security team.

FWIW, the security team worked on this issue in private for a good bit before abandoning the private process and pushing it to the public queue. So, yes, it's waiting on the security team at this point. It was the reverse for a while ;)

I've given this a cursory review and it seems like it's at least headed in the right direction.

I suggest adding some automated tests to confirm at least the CSRF code is working properly and perhaps the xss.

Patch #30 has been committed to 7.x-2.x.

Sorry @greggles, only just saw your reply.

We've released 7.x-2.4 containing this patch.

mlhess is going to review it and if all is good will remove the security warning and restrictions.

FIngers crossed everybody, thanks to all of you.

This is fixed guys. 2.4 is available now and security statement removed!

Thanks to all involved, well done!