Page MenuHomePhabricator

Full version of "old" image not purged from varnish cache when file deleted
Closed, ResolvedPublic

Description

Filing as a security bug to be paranoid, but I don't really think its actually that much of a security issue. Worst case is someone could get a version of a deleted file, if they planned ahead before it was deleted.

Example: https://upload.wikimedia.org/wikipedia/test/archive/b/b3/20130709191600!Bawolff-test-del.jpg returns the file, despite https://test.wikipedia.org/w/index.php?title=File:Bawolff-test-del.jpg

It appears that we send HTCP purges for old version thumbnails, but not the actual file asset itself.

Steps to reproduce:
*Upload some file
*Overwrite it with something
*In the file description page, click on the old version of the image, thus loading the full resolution version of the old file (and getting it in varnish cache)
*Delete the file
*The old full resolution link still works as its still in varnish cache.


I'm working on a patch for this, and I'll post it to the bug when done.


Version: 1.22.0
Severity: normal

Details

Reference
bz51064

Event Timeline

bzimport raised the priority of this task from to High.
bzimport set Reference to bz51064.
Bawolff created this task.Jul 9 2013, 7:26 PM

Hi Brian, I'm assuming you're making progress on this, but let me know if you need help. Thanks!

bawolff: How are things going?
Any news, or do you need help from somebody (who)?

Bryan is going to try having a go at it, since this is relatively self contained.

bd808 added a comment.Aug 14 2013, 9:40 PM

Reproduced on test2 with:

Created attachment 13103
Call SquidUpdate::purge() when deleteing

I think this patch will send the appropriate squid/varnish purge messages.

I tested it locally by running a sniffer to watch for the HTCP packets being sent. You can get the sniffer I used from https://github.com/bd808/htcpsnoop. I have not tested with an actual squid/varnish cache layer.

attachment bug51064.patch ignored as obsolete

I almost posted a review to gerrit and then I remembered this was marked as "security".

Aaron, can you look at the patch too, since this is part of filerepo?

I've tested and read over this patch. It looks good to me.

Some notes:

*body of the foreach loop should be indented
*There's still a code path with moving files where the old name of the file isn't purged, but that could be a separate issue, since we're not trying to hide the old filename, and long term we want the old filename to give an http redirect anyhow, so really this code path probably doesn't matter.
*The revision delete code path (RevDel_FileList::doPostCommitUpdates) still has this issue. This is a much more important code path then the move file (This includes things like oversight of files), but still could be considered a mildly separate bug. Nonetheless, I would recommend fixing this code path at the same time.

bd808 added a comment.Aug 16 2013, 8:36 PM

*body of the foreach loop should be indented

Easy fix. I think my current listchars setting in vim is making it hard for me to see some of these indent issues. I'll work on that.

*There's still a code path with moving files where the old name of the file
isn't purged, but that could be a separate issue, since we're not trying to
hide the old filename, and long term we want the old filename to give an http
redirect anyhow, so really this code path probably doesn't matter.

I think that the call to purgeEverything() in move() _should_ purge the squid. File::purgeEverything() calls LocalFile::purgeCache() which in turn calls SquidUpdate::purge(array($this->getURL())).

*The revision delete code path (RevDel_FileList::doPostCommitUpdates) still
has this issue. This is a much more important code path then the move file
(This includes things like oversight of files), but still could be considered
a mildly separate bug. Nonetheless, I would recommend fixing this code path
at the same time.

I'm looking at this now. Unfortunately the level of abstraction here and my very limited understanding of the codebase may make this take longer than it could/should. The original patch took me 2 days to create. :)

bd808 added a comment.Aug 19 2013, 6:16 PM

Created attachment 13129
Call SquidUpdate::purge() when deleteing

Updated patch that fixes indent issue and adds a call to SquidUpdate::purge() from RevDel_FileList::doPostCommitUpdates().

Attached:

(In reply to comment #10)

Created attachment 13129 [details]
Call SquidUpdate::purge() when deleteing

Updated patch that fixes indent issue and adds a call to SquidUpdate::purge()
from RevDel_FileList::doPostCommitUpdates().

This looks good to me

Attached:

Change 79842 had a related patch set uploaded by BryanDavis:
Purge upstream caches when deleting file assets.

https://gerrit.wikimedia.org/r/79842

Change 79842 merged by jenkins-bot:
Purge upstream caches when deleting file assets.

https://gerrit.wikimedia.org/r/79842

Change 79843 had a related patch set uploaded by BryanDavis:
Purge upstream caches when deleting file assets.

https://gerrit.wikimedia.org/r/79843

Change 79844 had a related patch set uploaded by BryanDavis:
Purge upstream caches when deleting file assets.

https://gerrit.wikimedia.org/r/79844

The patch for master has been merged, so marking fix. The patches for stable still need to be merged.

Change 79915 had a related patch set uploaded by BryanDavis:
Release note update for bug 51064.

https://gerrit.wikimedia.org/r/79915

Change 79914 had a related patch set uploaded by BryanDavis:
Release note update for bug 51064.

https://gerrit.wikimedia.org/r/79914

Change 79918 had a related patch set uploaded by BryanDavis:
Release note update for bug 51064.

https://gerrit.wikimedia.org/r/79918

Change 79914 merged by jenkins-bot:
Release note update for bug 51064.

https://gerrit.wikimedia.org/r/79914

Chris: did you want me to merge the backports of Bryan's patch?

Yes please!

Change 79844 merged by jenkins-bot:
Purge upstream caches when deleting file assets.

https://gerrit.wikimedia.org/r/79844

Change 79915 merged by jenkins-bot:
Release note update for bug 51064.

https://gerrit.wikimedia.org/r/79915

Change 79843 merged by Brian Wolff:
Purge upstream caches when deleting file assets.

https://gerrit.wikimedia.org/r/79843

Change 79918 merged by jenkins-bot:
Release note update for bug 51064.

https://gerrit.wikimedia.org/r/79918

Change 80515 had a related patch set uploaded by Brian Wolff:
Backport purge upstream caches when deleting file assets.

https://gerrit.wikimedia.org/r/80515

Change 80515 merged by jenkins-bot:
Backport purge upstream caches when deleting file assets.

https://gerrit.wikimedia.org/r/80515

Gilles moved this task from Untriaged to Done on the Multimedia board.Dec 4 2014, 10:11 AM
Gilles raised the priority of this task from High to Unbreak Now!.
Gilles lowered the priority of this task from Unbreak Now! to High.Dec 4 2014, 11:23 AM