Skip to content
This repository has been archived by the owner on Jul 4, 2023. It is now read-only.

openssl: add x509 cert chain handling patches #38897

Closed
wants to merge 1 commit into from
Closed

openssl: add x509 cert chain handling patches #38897

wants to merge 1 commit into from

Conversation

DomT4
Copy link
Member

@DomT4 DomT4 commented Apr 21, 2015

This will land in 1.0.2b, but it's a better solution than us applying an old, outdated, weak Equifax cert till that point.

I've pinged OpenSSL to check I'm not being stupid to cherry-pick these patches, but they should be fine - I pulled all three related patches, so it's not like we're being overly selective. I also asked whether there was a release schedule for the 1.0.2b release with these fixes, but I don't particularly expect to be given an answer given OpenSSL's often (understandably) sensitive release schedule.

Fixes #38495
Fixes #38491

Upstream discussion:
https://www.mail-archive.com/openssl-dev@openssl.org/msg38674.html

@DomT4
Copy link
Member Author

DomT4 commented Apr 21, 2015

CC @Homebrew/owners @chdiza

system bin/"c_rehash"
end
# Remove this once 1.0.2b lands.
rm_f openssldir/"certs/Equifax_CA" if MacOS.version == :yosemite
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

c_rehash generates a copy of the cert, doesn't it? Do we need to remove that as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It generates a symlink rather than a copy. If you kill the actual cert, the symlink becomes ineffective. If the symlink is named 578d5c04.0 on your machine as well, I'll add removing it to the formula.

Copy link
Sponsor Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The file was named 578d5c04.0 on my system as well, although I was using the mozilla ca bundle from the curl website, so I think it's not safe to simply remove the symlink. A better solution could be to run system bin/"c_rehash" after the file is removed, because it will clean up orphaned symlinks.

@tdsmith
Copy link
Contributor

tdsmith commented Apr 21, 2015

Thanks a lot for engaging with the OpenSSL folk about this. I know I was difficult about the Equifax cert but with a little bit of distance the cert feels like a less invasive change to me than patching does, especially if we know this is fixed in the next release so we won't have to carry the cert much longer.

@DomT4
Copy link
Member Author

DomT4 commented Apr 21, 2015

No worries at all. Both OpenSSL and OpenBSD's teams have been helpful on this, which has been great.

I understood (and understand) the position you took on the Equifax cert. It was a crappy fix; It was the only fix we had at the time, but yeah, applying a weak cert someone else deliberately removed is a pretty big no-no in general security. To be forced into a position where we either break every Google domain on the planet for our users, or we add back a weak root certificate... Was not a happy situation.

I don't particularly know what the best fix here is. I favour patching, if only because we have no idea when 1.0.2b will land, and I'm hesitant to keep pushing the weak root till that point. With OpenSSL we could be dealing with a release any time between this weekend and six months time, depending on how the security landscape progresses. It won't be either of those extremes, but I think I managed to semi-communicate the point I'm trying to make 😅.

Ultimately though, I'll let it go to maintainer vote. The solution we have now isn't great, but it works. Patching is invasive, invasive isn't great, but it works. Not sure which is more 👿 in this situation.

Just got a reply from Matt at OpenSSL: https://www.mail-archive.com/openssl-dev@openssl.org/msg38695.html

This *will* land in 1.0.2b, but it's a better solution than us
applying an old, outdated, weak Equifax cert till that point.

I've pinged OpenSSL to check I'm not being stupid to cherry-pick these
patches, but they should be fine - I pulled both related patches,
so it's not like we're being overly selective. I also asked whether
there was a release schedule for the 1.0.2b release with these fixes,
but I don't particularly expect to be given an answer given OpenSSL's
often (understandably) sensitive release schedule.

Fixes #38495
Fixes #38491

Upstream discussion:
https://www.mail-archive.com/openssl-dev@openssl.org/msg38674.html
@chdiza
Copy link
Contributor

chdiza commented Apr 21, 2015

I'm inclined to keep the cert fix.

Thanks again, @DomT4 , for getting on upstream. (I find it somewhat dismaying that they refuse to acknowledge that this is a bug, however.)

@MikeMcQuaid
Copy link
Member

I'd prefer a patch over a weak cert fix. I'm happy to merge this if no other @Homebrew/owners object.

@DomT4
Copy link
Member Author

DomT4 commented Apr 22, 2015

Test bots confirmed the wget test passed fine with the patches to openssl - Have removed the wget commit now, given whatever the decision here, we don't actually want to revision or propose a revision to the wget formulae 😸.

@chdiza
Copy link
Contributor

chdiza commented Apr 23, 2015

Uh oh. I tried the patching way, and it fixed some but not all of the problems.

In particular, mutt is still demanding manual acceptance of the Equifax cert for imap.gmail.com. My little ruby script that checks for new messages, however, works again.

@chdiza
Copy link
Contributor

chdiza commented Apr 23, 2015

Also, mutt is still demanding manual acceptance of the certs associated with hotmail (reported in #38495), despite applying the two patches in this PR.

@chdiza
Copy link
Contributor

chdiza commented Apr 23, 2015

Having applied the two patches, if I do: openssl s_client -connect google.com:443, I get:

CONNECTED(00000003)
depth=2 C = US, O = GeoTrust Inc., CN = GeoTrust Global CA
verify error:num=20:unable to get local issuer certificate
---
Certificate chain
 0 s:/C=US/ST=California/L=Mountain View/O=Google Inc/CN=*.google.com
   i:/C=US/O=Google Inc/CN=Google Internet Authority G2
 1 s:/C=US/O=Google Inc/CN=Google Internet Authority G2
   i:/C=US/O=GeoTrust Inc./CN=GeoTrust Global CA
 2 s:/C=US/O=GeoTrust Inc./CN=GeoTrust Global CA
   i:/C=US/O=Equifax/OU=Equifax Secure Certificate Authority
---
Server certificate
-----BEGIN CERTIFICATE-----
[[[ A whole bunch of base64 ]]]
-----END CERTIFICATE-----

[[[ A whole bunch of stuff I don't know if I should be posting ]]]
    Start Time: 1429763668
    Timeout   : 300 (sec)
    Verify return code: 20 (unable to get local issuer certificate)
---

@DomT4
Copy link
Member Author

DomT4 commented Apr 23, 2015

Did you rebuild mutt after pulling this PR in? Given the rather specific way it hooks into OpenSSL functions it may prove necessary.

Essentially - OpenSSL believe this fixes everything. If it doesn't, either OpenSSL is wrong, or we're going to be carrying the Equifax root for many years to come. I'll be doing some heavy 🙏 for the former.

The hotmail domains don't seem to have an Equifax root in there anywhere, so theoretically the existing fix should be doing nothing on that 😕 - Unless Verisign cross-sign with Equifax, but there's no indication of that in the chain. Unless you're saying neither the previous fix or this fix did anything on that?

@chdiza
Copy link
Contributor

chdiza commented Apr 23, 2015

Yep, I rebuilt mutt straight away.

Sorry, in the hotmail case, mutt didn't demand manual acceptance of the Equifax cert, but it did demand acceptance of some other two certs.

@chdiza
Copy link
Contributor

chdiza commented Apr 23, 2015

I hope the openssl terminal call I posted above is the right way to test this.

@DomT4
Copy link
Member Author

DomT4 commented Apr 23, 2015

Are you only seeing this with mutt and not with cURL, wget, etc? OpenSSL's error messages from their terminal kinda suck sadly, I believe at the moment it'll still throw the local issuer certificate error because it'll still check for all the root certs even if the first one is found and accepted; At least, that's my understanding, the OpenSSL codebase is frequently confusing, heh.

If it's a wider problem for you, wget -O - https://mail.live.com and curl -I https://mail.live.com & similar should fail.

@chdiza
Copy link
Contributor

chdiza commented Apr 23, 2015

I see it with mutt, and not wget or curl from command line.

Possibly mutt is picking up the incorrectly reported (by openssl) error.

@DomT4
Copy link
Member Author

DomT4 commented Apr 23, 2015

Funky. Can you paste a few of the mutt error messages, if there's nothing too sensitive about them? Curious to see what's going on with their end of things.

@chdiza
Copy link
Contributor

chdiza commented Apr 23, 2015

Mutt is not spitting any error messages; it's doing what it's supposed to do if it fails to find the relevant cert, namely ask the user to accept the one the server is offering.

I'll run it in debug mode, however, to see if anything useful pops up. This will have to be on another machine, though.

@chdiza
Copy link
Contributor

chdiza commented Apr 24, 2015

@DomT4: Here is the (AFAICT) relevant snippet of the debug log of mutt.

[2015-04-24 14:01:58] Looking up imap.gmail.com...
[2015-04-24 14:01:58] Connecting to imap.gmail.com...
[2015-04-24 14:01:58] ssl_check_preauth: hostname check passed
[2015-04-24 14:01:58] X509_STORE_load_locations failed
[2015-04-24 14:01:58] X509_verify_cert: unable to get local issuer certificate (20)
[2015-04-24 14:01:58]  [/C=US/ST=California/L=Mountain View/O=Google Inc/CN=imap.gmail.com]
[2015-04-24 14:01:58] X509_STORE_load_locations failed
[2015-04-24 14:01:58] X509_verify_cert: unable to get local issuer certificate (20)
[2015-04-24 14:01:58]  [/C=US/O=GeoTrust Inc./CN=GeoTrust Global CA]

After that, mutt asks for manual acceptance as it's supposed to.

This is with the two patches applied to openssl on 10.10.3

@DomT4
Copy link
Member Author

DomT4 commented Apr 25, 2015

My gut feeling here is that we should merge this, because it solves the wget and curl use cases and those were the primary motivator for including the cert fix, and it reduces the surface area of Homebrew users we're having to shove a weak cert onto.

However - I have no intention to screw you and Mutt users over either. I think we should treat Mutt differently, since the bug fixed upstream has trickled down to everything but Mutt my POV is that we should treat Mutt as a Mutt bug at this point and not part of the OpenSSL bug. Especially since Mutt is having wider cert problems than wget or curl were.

I would essentially keep the OpenSSL cert fix, but insert it into the Mutt formula instead, ensuring Mutt users remain fine but at the same time reducing the surface area of people having the weak cert poked back onto their systems. Even under sandboxing this should work because postinstall has relatively free access.

Your thoughts on the idea, and @MikeMcQuaid's as well, are massively welcome.

@chdiza
Copy link
Contributor

chdiza commented Apr 25, 2015

I'm cool with merging.

I'm not sure it's a mutt bug (it could be, but I bet it's parasitic on openssl being broken all this time), but it's a bug that goes to openssl and mutt jointly. (Hunch: mutt's code for this probably just aped openssl's code, and mutt obviously hasn't adopted the openssl HEAD patches yet.)

However, I don't think we want to stick the cert fix into mutt. Mutt already has a way to deal with this. Users can just manually accept the cert, which gets stored inside their ~/.mutt_certificates (or wherever the user has specified that certs should go) and they'll never see a prompt again.

That's how mutt users have always had to deal with certs, both before Homebrew existed, and also once it did exist (up until the time when the openssl formula got the part where it extracts system certs into its own special place).

In short, if the mutt formula is left alone, its users won't see anything they're not already used to. They might just see something they haven't seen in a while.

@MikeMcQuaid
Copy link
Member

I'd also like to merge unless other @Homebrew/owners object.

@DomT4
Copy link
Member Author

DomT4 commented Apr 25, 2015

Users can just manually accept the cert, which gets stored inside their ~/.mutt_certificates (or wherever the user has specified that certs should go) and they'll never see a prompt again.

Great, as long as you're happy with that, I've no complaints with the status quo on mutt. Thanks for the feedback!

# https://www.mail-archive.com/openssl-dev@openssl.org/msg38674.html
patch do
url "https://github.com/openssl/openssl/commit/6281abc796234.diff"
sha256 "f8b94201ac2cd7dcdee3b07fb3cd77a2de6b81ea67da9ae075cf06fb0ba73cea"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition, this one has become relevant and should be added: https://github.com/openssl/openssl/commit/e5991ec528b1c339062440811e2641f5ea2b328b.patch

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upstream explicitly told me not to use this one. Something change?

You don't need this one. I'm about to revert it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I missed that part. Nevermind!

@DomT4
Copy link
Member Author

DomT4 commented Apr 27, 2015

This PR also fixes #39094.

@danimo
Copy link
Contributor

danimo commented Apr 27, 2015

Would someone please merge this? :)

@MikeMcQuaid
Copy link
Member

@DomT4 Thanks!

@danimo Patience, please.

@MikeMcQuaid MikeMcQuaid deleted the openssl branch April 27, 2015 11:30
@DomT4
Copy link
Member Author

DomT4 commented Apr 27, 2015

Thanks to everyone for the feedback on this. Appreciated 👍

Noctem pushed a commit to Noctem/homebrew that referenced this pull request May 2, 2015
This *will* land in 1.0.2b, but it's a better solution than us
applying an old, outdated, weak Equifax cert till that point.

I've pinged OpenSSL to check I'm not being stupid to cherry-pick these
patches, but they should be fine - I pulled both related patches,
so it's not like we're being overly selective. I also asked whether
there was a release schedule for the 1.0.2b release with these fixes,
but I don't particularly expect to be given an answer given OpenSSL's
often (understandably) sensitive release schedule.

Fixes Homebrew#38495
Fixes Homebrew#38491

Upstream discussion:
https://www.mail-archive.com/openssl-dev@openssl.org/msg38674.html

Closes Homebrew#38897.

Signed-off-by: Mike McQuaid <mike@mikemcquaid.com>
@Homebrew Homebrew locked and limited conversation to collaborators Jul 10, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

problems with cert verification by openssl on 10.10.3
6 participants