SpongePowered / Ore

Repository software for Sponge plugins and Forge mods
https://ore.spongepowered.org/
MIT License
78 stars 25 forks source link

potentially revert download get>post changes due to caching concerns #221

Closed progwml6 closed 7 years ago

progwml6 commented 7 years ago

I have significant concerns about the long term affects if we need to route ore through cloudflare/fastly, etc. 061dd2049520c44e0355f334b284bd0ab7572661 needs to be discussed in depth before this sits around for too long.

Mythra commented 7 years ago

This was brought up in a thread: HERE. Although I understand the need for having a Cache (and not wanting to have to scale up as thousands of people initiate downloads). It has a super nice side effect of being a net-win from a security perspective. Something that no one else has taken, and is overall a good thing ™️ .

I highly plead keep this nice security win for Ore in there. I'd rather see forms of ratelimiting with cloudfare making you wait 5 seconds, or some other sort of throttling (maybe download throttle? like forge?) rather than removing this security win for the community.

dualspiral commented 7 years ago

So, my question here is, why is using POST a good thing? Sure, we can all be HTTP verb purists here and talk about how we are posting acceptance, or GETting should be side-effect free and reproducible, but to be honest, it feels like security by obscurity to me and that's just not security at all. All that will actually happen is that users will go to the Ore site and just download it from there. Then plugin owners will release to Github Releases and get users to download from there, or do what Inscrutable did for himself and just uploaded stuff to Dropbox.

In all honesty, I suspect you're doing it to hide links. This is not a security win at all - users have just gone for the easier options instead. And it's going to be hard to combat that anyway, but using "POST" is just not the way to do it - you've seen how users are struggling on the forums.

While you can do your utmost to protect users, we should be trusting server owners to do their research. I agree, having a different endpoint for unsafe files is not a bad thing in itself, in fact, I encourage this, and there are ways you can try to protect users to try to drive the point home:

In all honesty though, if users are using a link with the name "unsafe" in it, then they should be inherently accepting that there might be something wrong.

Mythra commented 7 years ago

I mean, after reading your post (which I have to say I agree with some of your points). The main thing I get here is how it's trying to hide links? I mean the links are totally reproducible, you can still cURL these links, it's not like their hidden. Sure security through obscurity is really bad. Something we don't want to do, but if the links aren't hidden how is it obscure?

I mean the download links for a plugin will always be: https://ore.spongepowered.org/<author>/<plugin>/versions/<version>/confirm?downloadType=1. If the plugin is unsafe, there's no UUID in the link, there's no hidden secret sauce. The only time a unique uuid is given is for confirmation, and since that's the last line it can easily be wrapped in a cURL command:

curl "$(curl 'https://ore.spongepowered.org/rojo8399/PlaceholderAPI/versions/2.0/confirm?downloadType=1' --silent | tail -n 1 | xargs)" -XPOST --silent >> jarfile.jar

I mean really the only thing we're stopping here is wGet, and are we really sure we want to support something that doesn't even support HTTP1.1? Seems kind of crazy to do all this work to support it.

As for your "users will find other ways around it", I mean that's their choice. Security has always been at a trade off with "Ease of Use". If you want to upload your plugin to a source that doesn't "confirm" the download. That's fine, and if you choose to download from that source that's your choice. However, when you download from Ore currently, you're always prompted to ensure it's unsafe. Someone tries to send you an unsafe plugin, say "Send me the Ore Link". So you know if it's been verified. Ore becomes a place any user can trust. Where you know if a plugin has been verified.

Make the unsafe links require a time based token at the end, one that might last, say, an hour at a > time, and have two active at any one time, effectively a new token every 30 minutes where the latter one is displayed in Ore - meaning users will tend to have to click through a warning on Ore to get to the link. However, I admit that this will still have cache concerns.

This is a decent idea, but having a "cache" on the safety verification means you might as well not have a verification at all. The whole point of the verification from a security perspective is to verify. Yes, I understand this is an unsafe plugin, and have physically been given a warning that this could do bad things :tm: .

Add a text box to the right of Ore that contains a cURL link, which is hidden behind a "Here be dragons" box. You could still point it to the "safe" endpoint if you really like.

Again if you just a solid link (unless you do something like my command up above) that you can hit, there's no point. As I can just give a link to someone saying "Hey kid, download this". And they see "Ore", and trust it. Only to not get the verification popup, and download an unsafe plugin without warning.

On the Ore site, use a cookie to discover whether a user accepted the warning and redirect the "safe" endpoint based on that, so the link that a user will click will always go to a safe endpoint (and that's then what users see), which might then return the "unsafe" download - when using cURL or wget, no cookie set, send back warning!

I mean while this would help stop automated download, this wouldn't stop people running a mine craft server downloading unsafe plugins. This fixes the "Hey kid download straight from this url with cURL", but doesn't fix the "Hey kid go download this plugin from ore". The whole point of the verification is to let people know, they're doing something unsafe. If the verification isn't there always. It's useless. You've nerfed it's ability to truly be effective.

Maybe simply make unsafe links download as zip files that contain a text file "UNSAFE-README.txt" as well as the plugin - that extra step is then on the server owners head to continue with, especially if they ignore the warning.

Could work, but the extra step is already there. And since the links are reproducible :man_shrugging: Feel like players would get more annoyed then this since they have to open a file, rather than just following another download link. Also how long should the unsafe links last for?

In all honesty though, if users are using a link with the name "unsafe" in it, then they should be inherently accepting that there might be something wrong.

Great point! How many people read URLs though? Sure people who are conscious about this sort of thing may head warning to it, but many people do not. Clicking links that aren't correct are one of the biggest ways Spear Phising attacks occur. People do not read URLS Ever. Heck the Social Engineering Toolkit Has TWO modules specifically on creating fake looking sites, on fake looking URLs. Why? It works, really really well. We'd love to say "Just Read the URL", but unfortunately end users consistently don't do that.

dualspiral commented 7 years ago

I don't want to start a flame war over this, and I'll be quiet after this because I don't want this to escalate here further (I just wanted to point out there are other ways to try to do this), but you've said it yourself, all you are really stopping is wget. That honestly makes using POST pointless apart from an initial barrier - because all that is going to happen is that players will find out how to download stuff in one step using cURL and bypassing everything anyway, server owners will pass this method around, and you've ended up with all this work in Ore that isn't effective and also ensures that things like CloudFlare can't be used.

Sure security through obscurity is really bad. Something we don't want to do, but if the links aren't hidden how is it obscure?

Exactly. I didn't suggest making it obscure, I suggested using the available technological resources to make the link work in a way that you can protect users more. I'll say it again, security by obsurity is not security, and so the answer lies in not trying to be obscure. Let them have the link!

And, I should point out, this isn't about security of Ore, it's about trying to protect other servers. We can only go so far. If you are really worried about it, you should just disable non-browser downloads (as far as is possible) of the plugins until they are marked as safe, force them to go through Ore and read the warning that way.

I mean while this would help stop automated download, this wouldn't stop people running a mine craft server downloading unsafe plugins. This fixes the "Hey kid download straight from this url with cURL", but doesn't fix the "Hey kid go download this plugin from ore". The whole point of the verification is to let people know, they're doing something unsafe. If the verification isn't there always. It's useless. You've nerfed it's ability to truly be effective.

I don't think you got my point. I didn't say don't show a warning, I said use a cookie to detect they had. This cookie can be different for each plugin, and the link in question would always be on the warning page. So, the workflow would always still be download page > warning page > plugin, and the cookie would be set by the warning page, or even some JavaScript handler on the link that fires before linking to it, it could even be set up to remove the cookie seconds after the request is made.

We could argue about how far we should protect users, and to be honest, that's a call for the Sponge guys to make. In a perfect world, we could mollycoddle users and make sure they read every warning they get. I would sure love for this to be the case, but ultimately, we just can't do that 100%. Users don't read the same thing over and over. They find ways to skip the text.

There is some burden of trust that the user has to bear, and it is ultimately up to them to decide that. Installing any plugin, heck, any software on a computer comes with risks, reviewed or not. Ultimately, I wanted to provide ideas that don't use POST so that this issue can see some traction - they all have flaws, but the current system feels just as worthless to me as you showed me you could just one-line it.

Mythra commented 7 years ago

I also would not like to start a flame war, and I'm sorry if I cam off that way in my response. I grew up debating, and as such have an unnatural tendency to be gun-ho about things, and it comes out wrong. I'm sorry if I offended anyone, or made anyone feel like they couldn't share their point. That's not my intention in the slightest.

I myself agree, "Yes technically there is a way around it". But I think getting someone to paste in a long command has bit more of security risks elsewhere, "Hey Kid run: sudo rm -rf / --no-preserve-root" (No one actually run that command).

I'm also glad we agree if we really decide to revert this (due to cloudfare) then yes there should be another system in place.

Exactly. I didn't suggest making it obscure, I suggested using the available technological resources to make the link work in a way that you can protect users more. I'll say it again, security by obsurity is not security, and so the answer lies in not trying to be obscure. Let them have the link!

I still don't understand how this POST method is obscure. I agree none of the methods you mentioned are obscure. I just don't understand how this link stuff is obscure. Ore certainly isn't the first one to implement a "POST to this endpoint to accept Download", and nor do I think will it be the last. The link follows restful conventions (even the confirmation by adding a URL Param.) I guess I just don't get this point. Again though yes security by obscurity is not security, but I just don't see how this is security through obscurity. Most of the time security through obscurity is hiding the source code which contains a hardcoded key, or something similar. But ore is all open source, the links follow restful conventions, the post to download isn't a brand new strategy. It's not hiding anything. The download link is there, the verification parameter is named the same thing. Nothing is obscured here.

And, I should point out, this isn't about security of Ore, it's about trying to protect other servers. We can only go so far. If you are really worried about it, you should just disable non-browser downloads (as far as is possible) of the plugins until they are marked as safe, force them to go through Ore and read the warning that way.

I agree it is about protecting servers. I never had a doubt this is what we're trying to do, and I agree we can't be expected to protect everything, and your far fetched (as far as possible) is way too much, but really it's one extra HTTP Request, that actually forces users to paste in a long command, or click an extra button. It can help the non tech savvy, it does some good. Let's not just dismiss it as "Far Fetched".

I don't think you got my point. I didn't say don't show a warning, I said use a cookie to detect they had. This cookie can be different for each plugin, and the link in question would always be on the warning page. So, the workflow would always still be download page > warning page > plugin, and the cookie would be set by the warning page, or even some JavaScript handler on the link that fires before linking to it, it could even be set up to remove the cookie seconds after the request is made.

I'm sorry I misunderstood you here. I also think this would work.

Thanks for providing traction on this issue, and I'm sorry again if I seemed to be entirely dismissive. This is something I'm passionate about, and I would hate to see a subpar implementation in place to have cloudfare. I know we can only go so far, but the current system (while it has flaws, sure!) and users are inclined to skip text. Isn't as bad as I feel many people are making it out to be. It's something that no ones done. Sure not reading the warning message is still going to be a thing that happens. But it makes a user think twice, even if only for a second. And stops potential new server owners from downloading something random off the internet (ones who probably won't even know curl exists, or are on a panel who can't curl).

With that I also won't post anymore as I feel I've made my points. It's up in the hands of sponge, and I trust them to think about security when implementing the solution.

progwml6 commented 7 years ago

@windy1 this needs to stay high priority as if this gets reverted it needs to not get blown off for months breaking scripts again for downloads

lol768 commented 7 years ago

Hello,

I complained to @windy1 to make this a POST request for a while so I'll go over my reasoning:

So, my question here is, why is using POST a good thing? Sure, we can all be HTTP verb purists here and talk about how we are posting acceptance, or GETting should be side-effect free and reproducible, but to be honest, it feels like security by obscurity to me and that's just not security at all.

The reason it's a good thing is because it automatically protects against Cross Site Request Forgery - at least it does with how we've set up Play. All the forms transparently include a high-entropy CSRF token which is locked to the current session, and these are automatically checked by a filter on the server to ensure the request is genuine. I wouldn't call this security through obscurity, unless you'd also call Facebook's session cookies "security through obscurity". This is a known best practice for form submissions.

The aim of this is to ensure that, in a web browsing environment (cURL/wget and other CLI user agents are supposed to be exempt from this, and are out of scope), downloads from Ore are either safe, or unsafe and the actual user has acknowledged the warning. This ensures remote sites cannot forge a POST request and have the user think they're downloading a safe file.

We could potentially add support for a X-Is-Script header or similar, on top of the current User-Agent detection to make scripts exempt. I'd assume they'd normally only be downloading safe files though.

But you're right, the actual request verb isn't of massive importance. We could use a GET, as long as we ensured that the CSRF token was in place in the query string at all times, the URL was never redirected to, and the CSRF token checking was manually implemented on the backend. Ultimately I suspect this would be more brittle - and indeed @windy1 did write a system which made use of tokens and GET requests (see https://github.com/SpongePowered/Ore/commit/91087487a32a884097f4dadec16f7f2781842de2), but I broke this implementation with a one line <img> after it had been deployed. Could it have been patched up and fixed? Perhaps, but the current method is a lot more robust.

I'll go through some of the suggestions made here later today, but you need to keep in mind that: if any user can get a link that works and isn't tied to their session, they can redirect a victim to it and it will look like a download from Ore.


I will note that all of this has been discussed in depth already, Korobi should have some logs.

progwml6 commented 7 years ago

I'm aware of the security concerns, however ... i'd rather this still be a GET endpoint somehow(with security added for web browsers), and work easily for users who are using command line tools to install the plugins on servers.

mbax commented 7 years ago

work easily for users who are using command line tools to install the plugins on servers.

Clicking 'accept' on the 'this is not approved, could damage your computer or light your hair on fire' dialog then provides the user with copypastable single-use curl command?

Aaron1011 commented 7 years ago

This is a known best practice for form submissions.

This is only 'form submission' in the technical sense - i.e. a <form> element is present on the page. Downloading an unsafe file from Ore is simply that - downloading a file. It's irrelevant which user session is used to initiate the download.

This ensures remote sites cannot forge a POST request and have the user think they're downloading a safe file.

That's really not true. CSRF tokens are useful when a request performs some action on the webserver (modifying or deleting a resource). Otherwise, a remote site could trick a user into performing an action as themselves on the target site - i.e. with their cookies.

This concept is completely inapplicable to downloading an unsafe file, because:

a) No change is actually taking place on Ore - downloading an unsafe file 'on behalf of someone' (with their cookies attached to the request) is pointless b) The same file is served up, regardless of the session used to request it. There's absolutely no benefit to causing user A to make the request instead of user B- regardless of who users A and B are, or what privileges they have. c) Because no data is actually being submitted, remote sites can absolutely 'forge' a request by simply accessing and extracting the CSRF token from the page.

All of this assumes that a malicious site skips the obvious step - simply host and server whatever malicious 'plugin' they want to, without the need to go through Ore. If some third-party site has managed to convince a user to click on their 'download' link, it really makes no difference where the file that is ultimately downloaded is served from. Any user who isn't alerted by the hostname of the site they're visiting certainly isn't going to notice where the file happens to be served from (e.g. the url displayed in the 'Downloads' view of their browser).

lol768 commented 7 years ago

Downloading an unsafe file from Ore is simply that - downloading a file. It's irrelevant which user session is used to initiate the download.

No it's not. The point is to track which users have acknowledged unsafe file warnings. It's crucial we ensure that the user is the one who has done this. If I'm not mistaken Windy has a table for tracking which users have done this and which haven't.

CSRF tokens are useful when a request performs some action on the webserver (modifying or deleting a resource)

Yes, I'm aware as to what they're used for. In this case, the resource being created is a record of the user's acknowledgement that the file is unsafe. You're absolutely right, without the CSRF token a remote server could make a user inadvertently mark themselves as being aware a file X is unsafe. Then, if they followed a link to a download for that file (or were redirected), they wouldn't be warned. This is exactly why there is a CSRF token.

No change is actually taking place on Ore - downloading an unsafe file 'on behalf of someone' (with their cookies attached to the request) is pointless

The point is to disallow a remote site causing the user to be marked as having seen a warning. Marking an unsafe file as acknowledged as unsafe is certainly not pointless, it stops the user from seeing a warning.

The same file is served up, regardless of the session used to request it. There's absolutely no benefit to causing user A to make the request instead of user B- regardless of who users A and B are, or what privileges they have.

Users who have not acknowledged a warning are not supposed to be able to download an unsafe file. If user B hasn't clicked through a warning, the server should not serve them a file.

Because no data is actually being submitted, remote sites can absolutely 'forge' a request by simply accessing and extracting the CSRF token from the page.

This isn't possible due to the same origin policy. The server could only receive its own CSRF token which wouldn't match the one tied to the end user's session.

All of this assumes that a malicious site skips the obvious step - simply host and server whatever malicious 'plugin' they want to, without the need to go through Ore.

An attempt to do this would be clear from the download dialog. If the user were to ignore this, then yes - there's not a lot we can do.

The POST approach also ensures that it's not possible to create a direct link from within an Ore project description because of the restrictions on the markdown permitted. Combined with this, and an interstitial page for external links, users are in a much better position in terms of security.

lol768 commented 7 years ago

I'm aware of the security concerns, however ... i'd rather this still be a GET endpoint somehow(with security added for web browsers), and work easily for users who are using command line tools to install the plugins on servers.

When I originally proposed this, I suggested that the system check the User-Agent and work fine with a GET request for CLI clients (i.e. exempt them from all of this). If this isn't the case, there's a bug that needs to be addressed.

Aaron1011 commented 7 years ago

It's crucial we ensure that the user is the one who has done this. If I'm not mistaken Windy has a table for tracking which users have done this and which haven't.

Fair point - but any malicious site has numerous ways to work around this.

Marking an unsafe file as acknowledged as unsafe is certainly not pointless, it stops the user from seeing a warning.

This isn't possible due to the same origin policy. The server could only receive its own CSRF token which wouldn't match the one tied to the end user's session.

OK - but I'm still unconvinced that this kind of direct linking to Ore is really an issue.

An attempt to do this would be clear from the download dialog. If the user were to ignore this, then yes - there's not a lot we can do.

The entire premise of this issue is that the user has already clicked a download link from a remote site. If the user is aware that they're not on Ore, the download dialog won't present them with any new information. If the user isn't aware of what site their on, it seems incredibly unlikely that the download dialog would cause them to take notice. Someone who's decided to download a file has already shown that they trust the site they're on, whether that trust is misplaced or not.

The POST approach also ensures that it's not possible to create a direct link from within an Ore project description because of the restrictions on the markdown permitted.

We can easily prevent this by sanitizing/replacing all links present in the user's markdown, just like for the interstitial page. It's not necessary to have a POST request to accomplish this.

Aaron1011 commented 7 years ago

Thinking about this some more, couldn't we still perform the necessary checks for the users's 'acknowledged unsafe files' status without using a POST request or CSRF token, o? As @dualspiral said earlier, we can simply store this in the user's session, and display a warning page if necessary.

Actually confirming unsafe downloads can be done via a separate POST request, ensuring that a user always needs to explicitly confirm unsafe downloads before being presented with the file.

windy1 commented 7 years ago

couldn't we still perform the necessary checks for the users's 'acknowledged unsafe files' status without using a POST request or CSRF token

Yes we can, but not under the current design.

The problem is that images, among other things I'm sure, will follow redirects in some browsers, since just landing on the warning page is constitutes "acknowledgement" under the current system, this makes it possible to redirect a client to the warning page through an image, which in turn, opens up the download to the user without ever actually seeing the warning. My initial response was to change the routes to POST: problem solved, right? Not exactly...

I think this design needs to be reevaluated to change the acknowledgement to be stored after the warning page and not right when it's landed on as it's handled now.

tl;dr I know the system isn't perfect as of right now but it's safe as it as and I'm not about to revert it until I have a better solution

Aaron1011 commented 7 years ago

I think this design needs to be reevaluated to change the acknowledgement to be stored after the warning page and not right when it's landed on as it's handled now.

I think requiring the user to explicitly click a button on the warning page (which would submit a form) would be best. That we, we ensure that the user has given explicit confirmation, while avoiding making links overly verbose or complicated.

lol768 commented 7 years ago

I think requiring the user to explicitly click a button on the warning page (which would submit a form) would be best. That we, we ensure that the user has given explicit confirmation, while avoiding making links overly verbose or complicated.

How would you see this working, a POST (w/ CSRF protection) request to some /warning/acknowledge endpoint and then a redirect to a GET for the download endpoint (which checks to ensure the warning has been ack'd)?

I can't see an issue with this approach.

windy1 commented 7 years ago

824b5e93805c15164019568b91b9ca1535440732