Declare Insecure Redirects

[DomT4]

Re discussion in #38824.

[vszakats]

Wouldn't https://download.gnome.org be better than https://download.gnome here? .com and .net aren't used by the GNOME project, though I haven't checked other TLDs.

[DomT4]

I cut off the country code because I was trying to shorten an already lengthy line. Sourceforge's could also be cut for the same reason.

I don't mind either way though. We can do either, ultimately, if the maintainers like the idea.

[vszakats]

No strong preference from here either. One interesting thing is that sourceforge.jp uses a different setup than the main sourceforge site, so that one is probably best to be handled separately.

[vszakats]

This is good as a general disclaimer to avoid false expectations about security, my minor concern is that it will also appear when these redirectors start to switch using secure URLs, thus giving unnecessary worries for an otherwise secure redirect. Ideally redirectors should switch all their mirrors to SSL/TLS at once, but in practice most probably it will be a slow and gradual process (possibly with some plaintext mirrors remaining indefinitely).

[DomT4]

thus giving unnecessary worries for an otherwise secure redirect.

This is largely why I softened the language from "will" and "won't" to "may" and "may not". I favour obeying the majority in this sort of thing; At the moment the majority (almost all, sadly) of the redirects are done to plaintext, so the warning is merited. Once we see the redirects hitting SSL/TLS more than plaintext, I'm happy to murder the warning, or narrow it down more than it is currently.

[vszakats]

Thanks, noted and agreed.

[jacknagel]

s/link/URL/

[DomT4]

Changed 👍

[MikeMcQuaid]

This is good as a general disclaimer to avoid false expectations about security, my minor concern is that it will also appear when these redirectors start to switch using secure URLs, thus giving unnecessary worries for an otherwise secure redirect.

Might be worth trying something where we use curl or similar to work out the actual redirected URL.

[vszakats]

-I -o /dev/null -w '%{url_effective}' curl options, when appended to the normal download command should return the effective URL on stdout.

[DomT4]

I have some concerns about that being misleading myself. See comment on other thread. On phone atm but will dig out a link when not.

Sent from my iPhone

On 20 Apr 2015, at 09:34, Mike McQuaid notifications@github.com wrote:

This is good as a general disclaimer to avoid false expectations about security, my minor concern is that it will also appear when these redirectors start to switch using secure URLs, thus giving unnecessary worries for an otherwise secure redirect.

Might be worth trying something where we use curl or similar to work out the actual redirected URL.

—
Reply to this email directly or view it on GitHub.

[vszakats]

I think I may have understood your concern: If the redirection detection and the actual download are two separate curl sessions, the detected redirects may (and if they may, they certainly will, as your tests has shown) result in two different redirected URLs. To avoid that, it's possible to add the -w option to the actual download command, f.e.:

curl -sS -f -L 'https://sourceforge.net/projects/upx/files/upx/3.91/upx-3.91-src.tar.bz2' -o 'dl.tar.bz2' -w '%{url_effective}'

Effective URL will be on stdout.

[DomT4]

Changing the curl args themselves is somewhat difficult, well, not difficult but more invasive - We spread the curl args out across multiple points - global, utils and obviously, the download_strategy. Some of those elements are tagged as private API.

Feelings on changing any of that @jacknagel @MikeMcQuaid?

[jacknagel]

Adding the argument wouldn't be too bad, but at first glance, it looks like changing how curl itself is invoked in order the capture the output would be difficult while maintaining compatibility.

One possibility would be something like this:

diff --git a/Library/Homebrew/download_strategy.rb b/Library/Homebrew/download_strategy.rb
index 589147c..d24a06e 100644
--- a/Library/Homebrew/download_strategy.rb
+++ b/Library/Homebrew/download_strategy.rb
@@ -313,6 +313,7 @@ def downloaded_size
   def curl(*args)
     args << '--connect-timeout' << '5' unless mirrors.empty?
     args << "--user" << meta.fetch(:user) if meta.key?(:user)
+    args << "-w" << "Downloaded from: %{url_effective}\n"
     super
   end
 end

but without capturing the output, this would print even in cases where no redirects occur.

I dunno.

[vszakats]

Another alternative is to create an option to not allow protocol changes via redirections. This can be implemented by adding --proto-redir =https or --proto-redir =http curl option depending on the original URL being https:// or http:// respectively. The warning message in this patch could advertise such option.

[ In case the download would fail this way, Apache now has an implicit https mirror and similar can be implemented for sourceforge/gnome now or in the future. ]

[MikeMcQuaid]

Another alternative is to create an option to not allow protocol changes via redirections. This can be implemented by adding --proto-redir =https or --proto-redir =http curl option depending on the original URL being https:// or http:// respectively. The warning message in this patch could advertise such option.

This makes sense to me.

[DomT4]

option depending on the original URL being https:// or http:// respectively.

I put in a version of this. I don't mind at all if links want to silently upgrade themselves from plaintext to SSL/TLS - Breaking that side of things would send the wrong message. We're only interested in offering a way to block insecure redirects here, IMO.

I've added an environmental variable to the mix here, along the lines of your suggestion. I've also changed the download message to mention that variable. The whole run through currently looks like this in practice:

Thoughts welcome from everyone.

[DomT4]

If maintainers are happy with the above, I'll probably disable the this URL may redirect to http etc message if the environmental variable is set, since it won't be true in that case and it'll just noise up downloads without serving a purpose.

[vszakats]

I like these. Only one part I'd argue: protocol upgrades. While convenient and slightly better than full plaintext, these can't IMO be considered secure, because in the initial (or any in-between) plaintext phase the redirection is very easy to spoof and the URL is also visible to any listeners. So, if we're to call the option 'secure downloads only', it'd seem better to exclude them. In such case the right solution is to swap the initial URL in the formula to use SSL/TLS instead.

[DomT4]

In such case the right solution is to swap the initial URL in the formula to use SSL/TLS instead.

To be put it somewhat bluntly, I don't particularly think most users, even those running with the environmental variable, can be consistently relied upon to submit package updates to bump URLs to the correct protocol.

My worry there is that we'll end up blocking a lot of things, and instead of users submitting the changes most of the time, they'll just unset the variable for that download. I want to improve security, but at the same time, I don't want to make using any Homebrew environmental variable an inconvenience to users more than absolutely necessary.

I don't think we can argue this two ways. We say we use https://downloads.sourceforge and similar because some SSL/TLS is better than complete plaintext - But if we ban plaintext to SSL/TLS redirects under the INSECURE env we're essentially reversing that and saying links should be explicitly plaintext all the way or explicitly SSL/TLS all the way, surely?

[vszakats]

I agree not to bug users with this. This could rather be something verified in the brew audit phase to help it getting fixed by contributors.

[MikeMcQuaid]

This could rather be something verified in the brew audit phase to help it getting fixed by contributors.

This sounds interesting and we'd review a patch to do this.

[DomT4]

This could rather be something verified in the brew audit phase to help it getting fixed by contributors.

Aye. Agreed. We should do it, but it should be nudged onto formulae authors rather than folks who just want to use Homebrew to deal with package management and not be fussed about the inner workings and such.

[vszakats]

I've updated my prototype shell script with capability to catch this case and show a message accordingly:
https://gist.github.com/vszakats/663dc8ccf1d49b903f8e

[sorry for no direct patch - it's my Ruby skills]

UPDATE: There is one caveat: If there is a redirector accessible via http-only, which sometimes redirects to https, in other times to http, the origin URL cannot be updated. There is the hope such unsafe redirector doesn't exist.

[MikeMcQuaid]

I've updated my prototype shell script with capability to catch this case and show a message accordingly:
https://gist.github.com/vszakats/663dc8ccf1d49b903f8e

[sorry for no direct patch - it's my Ruby skills]

@vszakats It'd be good if you feel passionate about these things if you can try and work on some Ruby. I'd never written any Ruby before Homebrew and started with formulae and then moved to core code, being a maintainer and now work full-time writing (mostly) Ruby. There are lots of resources and people who will help you.

[vszakats]

Can't argue with that. I'll need to finish some running projects to allocate time for it. What would normally be a 5-60 minutes patch, ATM requires days, including to find out how to create/run a spare working copy of Homebrew where debugging is done. Ruby has lots of ways to achieve the same result, which requires an extra leap to create something presentable. I'm hoping to get there eventually.

[MikeMcQuaid]

Thanks @vszakats, much appreciated.

[DomT4]

Pushed a change so the message doesn't print if the environmental variable is set.

[vszakats]

👍

[DomT4]

I'm happy enough with this if everyone else is at this point, but will adjust if there's any further feedback.

[MikeMcQuaid]

Any other @Homebrew/owners have thoughts?

[jacknagel]

Requesting a day to look it over, haven't kept up with the current proposal (sorry).

[MikeMcQuaid]

In fact, I'm not that confident on my implementation. There are some edge cases which are easy to be ignored. For example, I failed to use URI.join in the first time. I do have a question on whether we should print the effective url for normal users(who don't set HOMEBREW_NO_INSECURE_REDIRECT)? If not, proto-redir may be a more robust solution.

I'd like us to print this URL as I think it'll help with debugging and it's better to keep users informed if there's a protocol downgrade.

[MikeMcQuaid]

Might want to call this resolved_url or actual_url or something to make the below code a bit easier to follow

[MikeMcQuaid]

To me the diff looks pretty much what I had in mind (with a few tweaks).

[DomT4]

Changes pushed in response to comments. More comment welcome 😉

[xu-cheng]

This won't work. We cannot add -L. If you test it, you will see why.

[DomT4]

Yes that's what I thought to but I did test it, it worked. Have a reproducible example?

Sent from my iPhone

On 16 May 2015, at 17:28, Xu Cheng notifications@github.com wrote:

In Library/Homebrew/download_strategy.rb:

@@ -312,6 +323,11 @@ def _fetch
curl @url, "-C", downloaded_size, "-o", temporary_path
end

• def actual_url(url)
• m = Utils.popen_read("curl", "-I", "-L", url).match /^Location: (.+)$/
  This won't work. We cannot add -L. If you test it, you will see why.

—
Reply to this email directly or view it on GitHub.

[DomT4]

It slows the GNU formulae fetch process down very very slightly. That's the only problem I've seen though. I fetched several SF formulae, several known non-redirected formulae, bottles and source packages. What problem are you expecting to run into here?

[xu-cheng]

I didn't test it on a resource but on my own website.

$ brew pry
==> Interactive Homebrew Shell
Example commands available with: brew pry --examples
[1] brew(main)> def actual_url(url)
[1] brew(main)*   m = Utils.popen_read("curl", "-I", "-L", url).match /^Location: (.+)$/
[1] brew(main)*   m.nil? ? url : URI.join(url, actual_url(m.captures.first)).to_s
[1] brew(main)* end
=> :actual_url
[2] brew(main)> actual_url "http://www.xuc.me"
URI::InvalidURIError: bad URI(is not URI?): /
from /usr/local/Cellar/ruby/2.2.2/lib/ruby/2.2.0/uri/generic.rb:1100:in `rescue in merge'
[3] brew(main)>

$ curl -I -L http://www.xuc.me
HTTP/1.1 302 Found
Date: Sat, 16 May 2015 17:09:17 GMT
Connection: keep-alive
Set-Cookie: __cfduid=def8c22540ebda317655e556fef3699ed1431796157; expires=Sun, 15-May-16 17:09:17 GMT; path=/; domain=.xuc.me; HttpOnly
Pragma: no-cache
cache-control: no-cache
Location: /
Server: cloudflare-nginx
CF-RAY: 1e78b07e9429198c-HKG

HTTP/1.1 301 Moved Permanently
Date: Sat, 16 May 2015 17:09:18 GMT
Content-Type: text/html
Connection: keep-alive
Set-Cookie: __cfduid=def8c22540ebda317655e556fef3699ed1431796157; expires=Sun, 15-May-16 17:09:17 GMT; path=/; domain=.xuc.me; HttpOnly
Location: http://xuc.me/
Server: cloudflare-nginx
CF-RAY: 1e78b08194a1198c-HKG

HTTP/1.1 301 Moved Permanently
Date: Sat, 16 May 2015 17:09:18 GMT
Connection: keep-alive
Set-Cookie: __cfduid=ddf971c5bcd00711d97cebfc66f78c9221431796158; expires=Sun, 15-May-16 17:09:18 GMT; path=/; domain=.xuc.me; HttpOnly
Location: https://xuc.me/
Server: cloudflare-nginx
CF-RAY: 1e78b084d9a200c4-HKG

HTTP/1.1 200 OK
Server: cloudflare-nginx
Date: Sat, 16 May 2015 17:09:18 GMT
Content-Type: text/html; charset=utf-8
Connection: keep-alive
Set-Cookie: __cfduid=dc3f4faf4b1bb38c89b7f6c00ec55dff51431796158; expires=Sun, 15-May-16 17:09:18 GMT; path=/; domain=.xuc.me; HttpOnly
Last-Modified: Mon, 16 Mar 2015 01:33:56 GMT
Access-Control-Allow-Origin: *
Expires: Sat, 16 May 2015 17:19:18 GMT
Cache-Control: max-age=600
CF-RAY: 1e78b0863c6b1944-HKG

[DomT4]

For me:

==> Interactive Homebrew Shell
Example commands available with: brew irb --examples
irb(main):001:0> def actual_url(url)
irb(main):002:1>   m = Utils.popen_read("curl", "-I", "-L", url).match /^Location: (.+)$/
irb(main):003:1>   m.nil? ? url : URI.join(url, actual_url(m.captures.first)).to_s
irb(main):004:1> end
=> nil
irb(main):005:0> actual_url "http://www.xuc.me"
=> "http://www.xuc.me/"
irb(main):006:0>

Tested using both Ruby 2.2.2 via rbenv and using Ruby 2.2.2 from Homebrew.

@MikeMcQuaid @jacknagel @vszakats Can you run the above as well and see what reproduces for you?

[MikeMcQuaid]

@xu-cheng Can you explain what's incorrect about mine? I tested it and it produced the expected result so I'm confused.

[MikeMcQuaid]

Ah yes, I see the recursion issue.

[xu-cheng]

Yes, the test may create false positive as there aren't many resources with multiple redirects.

[DomT4]

Which version am I using here? Your's or Xu's?

[MikeMcQuaid]

Xu's.

[smarek]

I've addressed this issue to GNOME sysadmins through this ticket, https://bugzilla.gnome.org/show_bug.cgi?id=749481

Which would be far more elegant solution, to get redirect only to valid HTTPS mirrors if the original file is accessed through HTTPS (which means, client is already able to verify SSL certificate chain or deliberately ignores it)

[MikeMcQuaid]

I'd like to help test this so we can get it merged in. Can you suggest some formulae I can try to reproduce the insecure redirect (ideally one that has a single redirect and one that has more than one)? Thanks!

[xu-cheng]

Sorry for the late comment. But current implementation is still incorrect. In short, you cannot use -L (which will print all the redirects) and recursion at the same time.

My version of actual_urls is correct. Except it will output all redirect urls as an array.

[DomT4]

I need to deal with the multi-hop situation still - I'll rebase this and push that change ASAP.

Can you suggest some formulae I can try to reproduce the insecure redirect

brew fetch bash -fs -v
brew fetch diction -fs -v
brew fetch cogl -fs -v

Those are all pretty small formulae that can (always in GNU's case) insecure redirect off the primary url. Any sourceforge formula will have the same issue, but SF downloads seem to be having issues at the moment (Both in browser and via wget/curl).

[MikeMcQuaid]

@DomT4 the current output looks great! None of them seem to be https -> http, though.

[DomT4]

Thanks! GNOME's rejiggle of redirection priority helped a lot on that - Since they bumped the priority of SSL/TLS mirrors I've barely hit a plaintext one.

SF is the main redirector:

==> Downloading https://downloads.sourceforge.net/doublecpp/doublecpp-0.6.3.tar.gz
==> Downloading from: http://downloads.sourceforge.net/project/doublecpp/doublecpp/0.6.3/doublecpp-0.6.3.tar.gz

==> Downloading https://downloads.sourceforge.net/project/mussh/mussh/1.0/mussh-1.0.tgz
==> Downloading from: http://freefr.dl.sourceforge.net/project/mussh/mussh/1.0/mussh-1.0.tgz

==> Downloading https://downloads.sourceforge.net/project/arss/arss/0.2.3/arss-0.2.3-src.tar.gz
==> Downloading from: http://heanet.dl.sourceforge.net/project/arss/arss/0.2.3/arss-0.2.3-src.tar.gz

[vszakats]

Another https → http redirector is Apache, f.e. formula ant. (though it's most likely single-step)

[MikeMcQuaid]

Thanks folks. Will test again when you've pushed the code changes (and I guess it should be good to merge then?). Shout when that's happened!

[DomT4]

Alright, code updated, manpage updated.

@Homebrew/owners @bfontaine - Let's give this a (hopefully) final review and 👍 / 👎 this.

[DomT4]

As an aside, and something I would prefer not to get super hung up on in this PR, Apache formulae are currently excluded from being impacted by the variable. This is explicit in the manpage.

I'd like to automatically shunt Apache formulae onto the secure/private https mirror if the variable is set in future, but before doing so I think it'd be polite to get in touch with Apache and ask them if they mind the potential extra traffic, or like GNOME if they could bump the priority of secure mirrors so we hit them far less often.

Given the vast numbers of Sourceforge (In particular) formulae in the core, and the general benefits of redirection being announced to both us and users, I think this is worth getting in beforehand.

[MikeMcQuaid]

Why is Apache excluded, just the different download strategy? If it's not a lot of work it'd be good to enable the same for them for consistency but no worries if that's a later PR. I'll review this more in the next few days and merge if it looks good

[DomT4]

just the different download strategy?

This is a part of it, yeah. It doesn't share the same code as the standard curl strategy.

Beyond that though there's always a safe assumption that Apache formulae will have a secure mirror available, which we can use automatically if the ENV is set, once we talk to Apache upstream - Sadly we can't make the same assumption about any other group of formula, aside GNU, and GNU redirect from HTTP to HTTP anyway.

[MikeMcQuaid]

This is great work here @DomT4, thanks for all your patience here and for your help @vszakats. Merged!

[vszakats]

Thanks to all of you folks, great to see this finalized.

[DomT4]

Thanks for the feedback, assistance and patience in getting this done all 🎉.