shimmie2
shimmie2 copied to clipboard
shish
Image access is insecure, Nice URLs even moreso
There are several extensions that allow restricting access to certain images, such as the ratings and trash extensions. These both have in-page code that will redirect a user to the main page if they don't have permission to view the image. The actual image file URLs are not secured in any way, an unauthenticated user can just view anything they want if they have the direct URL. This is further compounded if Nice URLs are enabled, as that involves using redirects that access the files directly, removing any possibility of enforcing security on them.
The solution is simple enough, the url rewrites need to change to point at the php-powered image URLs, and shimmie needs to have code for enforcing these security restrictions. I've already got most of this done, the only issue is that it involves getting people to update their rewrite rules in order to secure things. Would it be desirable to have security checks in place to warn the updating admins, or something else?
Additionally, this could be an opportunity to change the "nice" urls for images and thumbnails to be a little nicer (ie, leaving out the hash), but this has the potential to be a breaking change.
What are your thoughts on all this?
A brain dump of thoughts in no particular order:
In general, there are a lot of places which assume files uploaded to the gallery will either be fully public or fully private - eg. the ratings system was designed to prevent users from seeing NSFW stuff by accident, not designed to protect against an intentional attacker. I'm not against adding some hardening features which make it more suitable for mixed-privacy use, but I worry that any software which isn't designed for mixed-privacy from day one with that use-case in mind for every feature will have a very hard time catching all the corner cases...
What attacks do we want to defend against here? In particular, right now a user needs[1] to have the hash of an image to get the full size version - how do they find the hash, without also finding the full size image the same way[2]? Currently we're using the hash as a not-very-secret secret, in so far as only people with the hash can access the files, and we restrict who has access to the hashes.
[1] Except that even when serving static files by hash, the PHP-powered /images/<image number> endpoint is still there, so an attacker could download all the images just by starting at /images/1.jpg and incrementing... We should probably disable serving-via-php when serving-directly is enabled...
[2] For example if we're worried about "Alice has permission to view an image, but Bob doesn't. Alice shares the URL with Bob. Now Bob can see the image." - Alice could just as easily share the image itself instead of sharing the URL. In general I'm assuming that if we give access to a single untrusted user, then the world has access - trying to contain access to a small number of untrusted users basically means DRM, which seems like a high-cost unwinnable war.
Which makes me think in general:
- High security: Protect the files (Can we do this without full-on DRM?)
- Mid security: Protect the index (If you know a file is there, you can get it; but the system won't provide you with a list of hidden files) <-- this is what the current security model aims for
- Low security: A user with zero access can get a list of hidden files <-- thanks to
/images/<image ID>.jpg, we are currently here, which is embarrassing. But if that gets fixed, then I think we are at the next level?
Using redirects to access files directly, bypassing PHP, is intentional for performance - using PHP to serve static files is a 10-20x load increase on the web server D: Also if we want to run security checks for every request for every user, the static files can't be cached at any point, which is another order of magnitude of performance loss.
If you want to add extra security to the serving-via-PHP path, then I'm ok with pretty much anything since it's already too slow for large sites and a bit more slowness won't hurt. If we can think of some way to secure uploaded files WITHOUT needing to run a bunch of heavyweight code for every request, that'd be great :)
We should probably also move the current security stuff down a layer - eg Image::by_id() should return null if the user doesn't have access - right now the Ratings extension blocks /post/view/123 but it doesn't block /images/123.jpg and it won't block any future extensions that it doesn't know about, because the checks happen at display-time instead of happening at database-time.
I suspect that "move the access controls into the Image class[3]" and "disable serving static files from disk" would be enough to stop the attack you're worrying about? The former seems sensible in any case, and the latter seems like a reasonable option.
[3] Designing off the top of my head - Something like Image::can_be_viewed(), to be called from Image::by_id / Image::by_hash / Image::find_images etc to ensure that we never return an Image object that the user isn't allowed to see. can_be_viewed() then sends out a CheckAccessEvent containing the Image with a can_be_viewed boolean, and any extension which cares about security can set that to False. Eg Ratings would have
function onCheckAccess() {
global $user;
if($event->image->rating not in get_user_class_privs($user->class)) {
$event->deny();
}
}
The problem of "we don't want people sharing URLs with each other" also applies to hotlinking - and I was thinking about that too for other reasons. Considering options like generating signed URLs so that the web server can generate a unique code for every request (eg using a private key to sign a tuple of the image hash, the client's IP address, the username, the current timestamp) and link to eg "/images/$hash/$filename.jpg?key=$BLAH". Then the cache (varnish in my case) would be able to independently verify that URL only using the server's public key and the client's metadata, no need for database lookups.
I don't really have much to say about how to deal with these tradeoffs, but I do think there's some value to not be able to view any link to a file.
A user downloading the image to pass it on is very obviously circumventing security, and the user will know what they're doing leads to someone else being able to view the image.
Sharing a link they could only find by logging in doesn't carry the same context. One might assume that only people with permission to view the image will be able to use the link and then post it in a not-so-private intranet or forum.
Authing the endpoints would protect against such accidental disclosure (but of course not malice)
I hadn't considered potential caching issues. It does throw a bit of a wrench into the works. The issue with using an key in the url would be that then every user's image request is a unique URL. On a large-scale site, you might as well not even cache at that point, since it would only be doing anything when the same user requests the same image. I don't really know how per-user access could be done effectively while using caching.
In my local dev copy I've been tinkering with an ImageDownloadingEvent that triggers the actual file send. Extensions can just interrupt the download if they detect that the image shouldn't be accessed, and is the basis for a get-argument system that allows me to do things like transcode and resize on-the-fly. I'm currently using it to provide wallpapers for my phone in what is quite likely the most needlessly over-complicated rotating wallpaper system ever. I would like to continue developing in that direction but it's apparent here that enforcing security on images is going to require a careful approach.
The changes I made semi-recently to how files are downloaded on page.php do help with memory usage and speeds it up a bit, but the overhead from running it through php (and by implication here, the authentication system) is definitively always going to be more than straight file access. Sites that need the absolute best performance are never going to want to use it, and they likely shouldn't.
The social aspect of people sharing images
Fortunately allowing the choice is easy. Currently direct downloads are enabled by way of the nice url system and URL rewrite redirects. All that has to be done is to present two sets of URL rewrite redirects. If the user needs fast downloads, they use the set that is direct to the file, otherwise the set that redirects to PHP scripts. Then the ImageDownloadingEvent takes care of it blocking it. We could have an additional (or in place of ImageDownloadingEvent) security check event, all depends exactly how we want to tackle this. A security event would be nice and clean, but it's one more event on the stack.
I don't think changing the Image::get functions to enforce security is a good idea, if for nothing else any background-type operations like the cron uploader would then also need to authenticate. That's not impossible, but it seems unnecessary to me. I feel security should be at a higher level than that as well.
At minimum, /images/