450548 - Uploaded images are being overwritten

Status: VERIFIED FIXED

(support.mozilla.org :: Knowledge Base Software, task)

Description

[Chris Ilias [:cilias]]

Attachment: overwritten image in article

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.

Comment 1

[Chris Ilias [:cilias]]

Attachment: URL of overwritten image

Comment 2

[Laura Thomson :laura]

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.

Comment 3

[Laura Thomson :laura]

Pushing to 0.6.4

Comment 4

[Les Orchard [:lorchard]]

This one looks annoying too.  Grabbing from Laura's plate

Comment 5

[Les Orchard [:lorchard]]

Attachment: patch (-p1) to generate image filenames on the server, with a simple UI for selecting per-page images

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

Comment 6

[Laura Thomson :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.

Comment 7

[Les Orchard [:lorchard]]

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

Comment 8

[Laura Thomson :laura]

I'm ok with breaking markup insertion for security.  David, Chris, Cww?

Comment 9

[David Tenser [:djst]]

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?

Comment 10

[Les Orchard [:lorchard]]

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.

Comment 11

[Chris Ilias [:cilias]]

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

Comment 12

[Les Orchard [:lorchard]]

Attachment: tweaked patch (-p1) to simply generate filename on server and not share prefix with client

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.

Comment 13

[Les Orchard [:lorchard]]

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.

Comment 14

[Les Orchard [:lorchard]]

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.

Comment 15

[[:Cww]]

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?

Comment 16

[David Tenser [:djst]]

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

Comment 17

[Laura Thomson :laura]

Comment on attachment 352133 [details] [diff] [review]
tweaked patch (-p1) to simply generate filename on server and not share prefix with client

yay.

Comment 18

[Les Orchard [:lorchard]]

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.

Comment 19

[Les Orchard [:lorchard]]

Er, this might be a better URL for editing a page on stage:

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

Comment 20

[Stephen Donner [:stephend] Not actively reading bugmail]

Attachment: Post-fix screenshot

Comment 21

[Stephen Donner [:stephend] Not actively reading bugmail]

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.

Comment 22

[Chris Ilias [:cilias]]

This didn't make it to prod. Can anyone else confirm on <https://support.mozilla.com/en-US/kb/Article+Sandbox>?

Comment 23

[Laura Thomson :laura]

Looks like lorchard forgot to merge to prod branch?  Easy fixed.

Comment 24

[David Tenser [:djst]]

Will this be merged for 0.8.1 instead?

Comment 25

[Laura Thomson :laura]

Yes.

Comment 26

[Laura Thomson :laura]

In prod branch r21431.

Comment 27

[Stephen Donner [:stephend] Not actively reading bugmail]

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.

Comment 28

[Marc Laporte]

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?

Comment 29

[Will Kahn-Greene [:willkg] ET needinfo? me]

These bugs are all resolved, so I'm removing the security flag from them.