I think that base64-encoded absolute server_filesystem path to the image file is the security hole (image.php using this path as imgp-parameter).

I propose to export only path, that relative to the brilliant gallery albums directory.
This path will be used in brilliant_gallery.module when we need to export it as part of the URL.
Then, relative path will be expanded to the absolute path in the image.php, using relative path value that was passed in base64-encoded imgp parameter.

For more security, the .htaccess file may be used to protect brilliant gallery albums directory from exterior HTTP access.

I thing, this is the critical issue.

Members fund testing for the Drupal project. Drupal Association Learn more


api87’s picture

Additional hole:
Attacker can form any absolute path, so he can view any image on the site (and HTTP-access-protected too): he only must know image's filesystem path. And when we use relative path, we must also check symbols as "..", because attacker can form correct path like "photos/../../protected_img/img.jpg" which means that he again gains access to any folder of the site.

I'm sorry, my English is not good.

drpchris’s picture

Version: 6.x-3.4 » 6.x-3.x-dev
Assigned: Unassigned » drpchris
Status: Active » Needs review
6.43 KB

this patch changes path passed to image.php to be based on the galleries folder (ex: /albums/albumname/file.jpg).

image.php checks for any '..' in the path and exits if it finds them.

This patch also incorporates the fixes proposed in http://drupal.org/node/566184 which checks that the image width and height params are valid (and incorporates comment #4 to check that the numbers make relative sense).

Use this patch instead of what's in http://drupal.org/node/566184

Made against the development branch, but should work fine against 6.x-3.4

Please test this out!

drpchris’s picture

Updated the diff files against the latest dev release.

drpchris’s picture

whoops, did the diffs the wrong way

these diffs go together as a set

Vacilando’s picture

Status: Needs review » Fixed

@api87; thanks for the reporting the findings.

@druchris1; huge thanks for the code. Since the dev changed a lot in the meantime, I had to apply it manually. Also, Picasa galleries did not work at all using the relative path encoding. Eventually I've made it compulsory to place the Picasa local cache into a subfolder of /files (just like the base BG gallery folder), and then fixed the paths, so things work now.

Committed to the dev version. If you say things work fine, I'll make a new version.

Vacilando’s picture

Version: 6.x-3.x-dev » 6.x-3.6
Assigned: drpchris » Vacilando

I've found numerous more problems; the gallery management system was broken, cron cache cleaning as well, etc.

Hopefully fixed it all.

Committed to 6.x-3.6. One of the best BG versions ever, I believe / hope!

Status: Fixed » Closed (fixed)

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