Uploaded images are being overwritten

VERIFIED FIXED in 0.8.1

Status

task
--
major
VERIFIED FIXED
11 years ago
3 years ago

People

(Reporter: cilias, Assigned: lorchard)

Tracking

unspecified
0.8.1

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: tiki_fixed)

Attachments

(4 attachments, 1 obsolete attachment)

If someone uploads an image that is the same name as an image that is already uploaded (and used in another article), the new image overwrites the old one.

Possible solutions:
- uploads must be specific to each article.
- add a "-n" to the file name.
- reject the upload, and say an image with that name already exists.
Severity: normal → major
Target Milestone: --- → 0.6.3
I'd suggest we generate the image name to be unique and random.  Using image names that are suggested by the user may open us up to attacks.
Group: websites-security
Assignee: nobody → laura
Pushing to 0.6.4
Target Milestone: 0.6.3 → 0.6.4
Target Milestone: 0.6.4 → 0.7
Target Milestone: 0.7 → 0.8
Group: websites-security
Group: websites-security
This one looks annoying too.  Grabbing from Laura's plate
Assignee: laura → lorchard
This patch causes uploaded pictures to be saved with a filename generated on the server.  This results in a file name which has a prefix common to the page, but with the rest of the filename being unique to the image.  Repeated uploads of the same image should end up with completely unique names, regardless of the name suggested by the user's browser.

Since these new filenames are so nonsensical and user-hostile, I've used the page-specific filename prefix to provide a simple interface listing all the images uploaded for the current page.  It's quick & dirty, but provides a way to click and insert image markup for previously uploaded images without needing to remember the filenames.

Note that this UI will work only with images uploaded from the point of applying this patch and forward, since it relies on the page name and locale hash prefix on image filenames.

This can also result in a pile-up of images as new or replacement images are uploaded.  But, this should be addressed soon in bug 449440
Attachment #351458 - Flags: review?(laura)
Comment on attachment 351458 [details] [diff] [review]
patch (-p1) to generate image filenames on the server, with a simple UI for selecting per-page images

Can we not put the randname_prefix in the form?  This seems exploitable.
Attachment #351458 - Flags: review?(laura) → review-
(In reply to comment #6)
> (From update of attachment 351458 [details] [diff] [review])
> Can we not put the randname_prefix in the form?  This seems exploitable.

Currently, when an image is chosen for upload, markup for that image is inserted at the end of the text area for the page.  This lets you pick an image and then edit the page to include it all in one step, before hitting "preview" or "save".

If I don't put randname_prefix in the form, there's no way for that image markup to be inserted by JS.  I was trying not to break that existing functionality.

But, I could insert the markup on the server-side after a "preview" or "save" - or just skip doing that altogether and make people use the new image selection UI I threw together.  The downside of that is that you can't pick an image for upload and edit the page all at once.

The server side does validate the format of the prefix, but I see I also forgot to include code that explicitly prevents overwriting files.  I think doing that would keep it from being exploited - or, I could just remove the whole shared prefix thing and break the markup insertion feature.
I'm ok with breaking markup insertion for security.  David, Chris, Cww?
Based on chat with Les and Laura in #sumodev, this seems like our best bet to fix this security issue. 

For the future, I would propose a more streamlined Wordpress-like user interaction where you simply click an [Add screenshot] button, which shows you an "inline pop-up" dialog asking for the file to upload. That dialog would then submit the file, get a response from the server with the created file name, and then return to the parent window with the proper markup inserted automatically.

(Add to this a WYSIWYG editor and we have a winner! ;))

a=djst on general direction here, but Laura still needs to review patch I'm guessing?
Okay - I'll need to make some tweaks to the patch to excise the potentially exploitable bits for markup insertion, which shouldn't take too long.
I'm a little confused. Does this mean that uploading an image is completely separate from editing an article? Does it force a preview? I guess I need to see the UI. :-)
Here's a new patch that disables markup insertion at the time of file selection.  I might just check it in since the changes are minor, but figured I'd upload it for completeness' sake.
Attachment #351458 - Attachment is obsolete: true
Attachment #352133 - Flags: review?(laura)
Oh, and to be clear about what this new patch does:

* To use in a page, first select each of them and hit "preview" to upload them.

* The server will save them with names of its choosing, and display thumbnails of all images uploaded for the page.

* Clicking on one of these thumbnails will insert the markup for the image into the page source text area.
Er, that first point above should be:

* To include images in a page, first select each of them with "Upload Image" in the editing form and hit "preview" to upload them.
just to be clear, the markup doesn't have the image name?  So scanning through the wikimarkup will no longer tell you which image is which?
Excerpt from irc to clarify the new (temporary, until we release SUMO 1.0) behavior:

djst: so the way it [will work] is that you enter the screenshot file path as usual, but you click preview
[18:24] djst: then you're getting back to the preview and you see a thumbnail of the newly added screenshot
[18:24] djst: then you click on that thumbnail and you can then click preview again to see it added?
[18:26] djst: was that a fair description of how your patch changes the behavior?
[18:26] lorchard: oh, yeah, the patch will show thumbnails of images uploaded for the page.  clicking on one inserts the markup
[18:26] djst: so the workflow is: 1) upload image, 2) click preview, 3) see thumbnail; click thumbnail, 4) click preview or save
lorchard: yeah, that sounds right
Attachment #352133 - Flags: review?(laura) → review+
Comment on attachment 352133 [details] [diff] [review]
tweaked patch (-p1) to simply generate filename on server and not share prefix with client

yay.
Checked into trunk as r20698

To exercise the changes in the patch:

* Edit a page, eg: 

http://support.mozilla.com/tiki-editpage.php?locale=en-US&page=*Installing+Firefox+on+Mac

* Select one or more images for upload, hit "preview" to perform the upload.

* After the upload completes and page reloads, you should see thumbnails of the images you uploaded underneath the Upload Picture section of the editing form.

* Clicking on one of the thumbnails should cause markup for the image to appear in the page source text area.

* The filename for the image should not be the same as what was uploaded.  That is, if you uploaded foobar.jpg, the name of the image on the server should be completely different.

* Try uploading the same image several times, and it should appear as multiple thumbnails with unique filenames, preventing overwriting of existing images.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Verified FIXED using http://support-stage.mozilla.org/en-US/kb/Clearing+Location+bar+history as a testbed, but I obviously didn't commit my test changes.
Status: RESOLVED → VERIFIED
This didn't make it to prod. Can anyone else confirm on <https://support.mozilla.com/en-US/kb/Article+Sandbox>?
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Looks like lorchard forgot to merge to prod branch?  Easy fixed.
Target Milestone: 0.8 → 0.8.1
Will this be merged for 0.8.1 instead?
Yes.
In prod branch r21431.
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
I'm verifying this as fixed, since we do that with bugs whose fixes have landed in both staging and production (and this has already been verified in staging).  Just a matter of this being pushed to prod, which will happen on the 13th of this month.
Status: RESOLVED → VERIFIED
Whiteboard: tiki_triage
The direction we are going is to manage these images in the File Gallery. To avoid  images being overwritten, but also to have an image picker. This is started in Tiki3 and improved in Tiki4.

Would this cause performance issues on SUMO?
Whiteboard: tiki_triage → tiki_fixed
These bugs are all resolved, so I'm removing the security flag from them.
Group: websites-security
You need to log in before you can comment on or make changes to this bug.