New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CVE-2014-3230 - don't disable verification if only hostnames should not ... #14
base: master
Are you sure you want to change the base?
Conversation
|
Thanks for the patch, not sure I understand the fix, though :). The bug says that setting HTTPS_CA_DIR or HTTPS_CA_FILE causes the IO::Socket::SSL to skip checking the server cert. The IO::Socket::SSL docs say that "The default for SSL_verify_mode on the client is currently SSL_VERIFY_NONE, which is a very bad idea, thus the default will change in the near future. The fix uses instead of the previous setting for SSL_verify_mode, but the docs say that "If you are really sure, that you don't want to verify the identity using the hostname you can use 'none' as a scheme." -- isn't this the same as disabling the cert check? Would be great if you could clarify, just want to make sure we're doing the right thing here -- maybe it's just the docs that need to be updated? |
|
Long story: In LWP version<6 Net::SSL/Crypt::SSLeay was used as default to provide https support. This module got configured with environment variables like HTTPS_CA_FILE etc and while it could check the certificate against the given CA file, it would not verify the host name, e.g. not check if the name in the certificate it got when accessing example.com really matched example.com or was instead attacker.com. When changing the default SSL package to IO::Socket::SSL with LWP 6 it tried to keep this behavior for compatibility reasons, when it detected that HTTPS_CA_* variables where used. It did this by setting the verify_hostname option to 0. This option is described as influencing the checking of the host name, not the checking of the certificate itself (there is no extra option in LWP to completely disable all verifications, one would have to use ssl_opts with SSL_verify_mode to do this). At about the same time LWP 6 was released I decided to change the default verification for the certificate (not host name, but certificate) in IO::Socket::SSL from the default no verification to verify by default - because users usually don't read enough documentation and enable important options as long as it works. In order to not break existing users too much I've started with a check only, e.g. if SSL_verify_mode was not set by the user it complained loudly, that it is using insecure defaults and how to change the code to fix this message. But obviously several people did get this message wrong and instead of making sure, that it has the necessary information to check the certificate (like CA path) they've decided to switch certification checking off completely, thus keeping everything explicitly insecure. RT#81948 is one example of how such a fix got wrong. Instead of explicitly enabling verification and setting SSL_ca_file, as recommended by the error message, the patch completely disabled verification if verify_hostname was set to 0. This in effect changed the meaning of verify_hostname from disabling verification of host name to disabling all verification, even for the certificate. This patch restores the intended behavior of the option, e.g. that it should only disable verification of the host name. If the user still uses an old version of IO::Socket::SSL this will result in getting the warning again. But the real fix here would not be to disable verification, but to enable verification and provide CA settings - or better yet to upgrade to a newer version of IO::Socket::SSL which has secure defaults enabled and thus does not complain anymore. |
Why special case users who set HTTPS_CA_*? "For compatibility reasons" doesn't make sense without further justification, given that you broke compatiblity for everyone else. Please keep in mind that:
If would make sense if verify_hostname influenced only hostname verification (after all, this is what the name says), but:
|
|
Hi jwilk,
If you google for HTTPS_CA_FILE you will probably only find references to LWP/Crypt::SSLeay. So, in a way, it makes sense to special case this because these are mostly users of the older LWP versions. Everybody likes a clean cut and nobody likes to maintain all the old code, but if you maintain a heavily used package like LWP you cannot just make changes which break the code for lots of users.
That would be nice, but if you look at recent discussions at stackoverflow or perlmonks they regularly try to force LWP to use Crypt::SSLeay when they run into problems and I often try to point them into the right direction, e.g. how to configure IO::Socket::SSL correctly (or in most cases just use a recent enough version).
The wording in LWP::UserAgent about verify_hostname is a bit unclear, but I read "no checks" as "not the checks for hostname verification which were described in the previous sentence". I would not read it as "no certificate verification at all". But a clearer message would be good.
Yes and no. In most cases I would prefer to just use the options from IO::Socket::SSL directly. But for hostname verification it makes more sense, that LWP sets SSL_verifycn_scheme to www by default. This makes it easier for the user to get it right, because most users don't even know that there are different schemes how hostnames can be verified. And LWP::Protocol::https already does the right thing, but only if verify_hostname is true (which it is by default). So it should (and does) make it easy to do the right thing, and hard to do the wrong thing. |
|
Here I wrote a test case [https://bugzilla.redhat.com/attachment.cgi?id=893881]. It would be nice if LWP-Protocol-https got more tests. |
|
Following patch is needed for IO::Socket::SSL < 1.950 to pass LWP::Protocol::https tests. |
This effectively disables certificate verification if not specified different and thus overrides the default of newer IO::Socket::SSL to have verification enabled unless specified otherwise. This in effect recreates the original problem and thus it is not a good idea. |
|
On Tue, May 13, 2014 at 07:58:33AM -0700, Steffen Ullrich wrote:
I agree - we should never turn off security features unless the user |
|
On Tue, May 13, 2014 at 07:58:34AM -0700, Steffen Ullrich wrote:
Not exactly. Have you tried that? The branch is executed only if verify_hostname is 0. And that happens only if I addition I wrote that this is suitable for IO::Socket::SSL < 1.950. And with With newer IO::Socket:SSL, more suitable code would be: $ssl_opts{SSL_verify_mode} ||= 1; However that's not needed because 1 is already the default value there. I proposed "||= 0" because of backward compatibility. If you want break it, However without this code, LWP-Protocol-https test suite will not pass with -- Petr |
|
The default value of IO::Socket::SSL is only used, if there is no value set by the user, e.g. SSL_verify_mode is undef. But with your proposal you define SSL_verify_mode whenever verify_hostname is set to 0, so that the default setting of IO::Socket::SSL gets not used. In essence this means, that if the user has set verify_hostname to 0 either through an LWP option or though the environment variable PERL_LWP_SSL_VERIFY_HOSTNAME, it will not only disable the checking of the hostname against the certificate, but also the verification of the certificate against the trusted CAs - unless the user has also explicitly set SSL_verify_mode to 1. Or in other words: it makes verify_hostname disable the complete certificate verification unless the user explicitly said otherwise. And this is not the expected behavior. In my opinion the test suite should just be fixed to explicitly add |
|
What would help here (for both future correctness, and to be sure that this @ppisar, can you include such tests with your patch? |
|
Am I the only one bothered to see that each of the modules involved (LWP::UserAgent, LWP::Protocol::https, CA::Mozilla, IO::Socket::SSL, Net::SSLeay, Crypt::SSLeay) has their own verification policy and variable names (HTTPS_CA_FILE , SSL_verify_mode, PERL_LWP_SSL_VERIFY_HOSTNAME, SSL_verifycn_scheme, etc.) which makes it really hard to understand what policy is indeed in effect? |
|
You are definitely not the only one. This is a maintenance, usability and security nightmare. But it all has historical reasons :{. As far as I know Crypt::SSLeay is/was only used by LWP and is only kind of maintained with no new features and will only be used if for whatever reason IO::Socket::SSL/Net::SSLeay is not possible. So it will never have support for hostname verification or do secure defaults. But, IO::Socket::SSL is used in lots of modules, and because you cannot expect everybody to understand the complexity of SSL, it evolved into providing secure as possible defaults. If I would be sure that IO::Socket::SSL/Net::SSLeay supports every platform Crypt::SSLeay does, I would suggest to cleanup LWP and remove any support and compatibility hacks for Crypt::SSLeay. |
|
On Wed, May 14, 2014 at 09:39:50AM -0700, Karen Etheridge wrote:
If you mean test for the IO::Socket::SSL warning, then it's already included If you mean general tests covering all possible scenarios, then it's much more At the end, instead of adding the controversal "$ssl_opts{SSL_verify_mode} ||= -- Petr |
|
I've contacted the maintainer of Crypt::SSLeay to see if he sees any problems with removing Crypt::SSLeay support from LWP - https://rt.cpan.org/Ticket/Display.html?id=95663 |
Yes, if we just merge #15 we would remove all of these problems, because this requires a recent IO::Socket::SSL too. |
|
@noxxi , could you please fix the Debian bug number in the pull request, |
Thanks for pointing this out. It is fixed now. |
|
Okay, why don't we take a step back and simplify this to the point where everybody can understand what the cert verification policy actually is? Look at curl, for example: To me, the cert check is either on or off. Secure or insecure. No one cares whether it's insecure because the host name isn't checked or the cert isn't checked or some other variable has a special value. So why don't we change IO::Socket::SSL to have an option to switch the cert check on or off (preferably something human-readable like "ssl_insecure => 1/0", defaults to 0) and a path to the CA file (something like ssl_cacert), and get rid of all those other confusing options? |
|
Since 1.970 the hostname will not be automatically checked, if SSL_verify_mode is 0, so this effectively works like the -k option of curl. As for other options: I think that the average user does not need to deal with any options at all, it should just work like in the browser. Only in special cases the user has to deal with the following options:
I agree that the user should not need to distinguish between verification of the hostname and verification of the certificate: like this bug and this discussion shows it is already hard for developers to distinguish between these things. And it also does not make sense, because if hostname verification is disabled a man-in-the-middle could present just a correctly signed certificate for another host. But this effectively means, that we cannot use Crypt::SSLeay any longer, because it does not support verification of the hostname at all. Or one has to copy the relevant code over from IO::Socket::SSL or reimplement the www parts without getting them wrong like others (Ruby and wget have the relevant checks wrong, Python fixed it in the last version). And I'm not sure if one should make it too easy for the user to disable verification of the certificate (and definitely not with a simple environment variable). Most users don't understand SSL and don't know that it is not possible to get secure end-to-end encryption without verifying the certificate. Even software like OTRS or FosWiki just disabled certificate verification permanently when sending mail with SSL, without giving the user a way to enable it. It would be nice of more documentation would help, but I doubt it :( Maybe one should name the option "SSL_i_dont_care_about_security_at_all" :) |
|
Is there a clear conclusion on what setting of As noted in @jwilk's comment, Few considerations:
|
|
I think the name of the key "verify_hostname" clearly states, that it is the verification of the hostname only, and nothing else. So any documentation should reflect the naming of the key and not re-interprete "hostname verification" as "certificate verification". And for security it would be fatal, if the user just wants to disable hostname verification, as the name of the key suggests, but it would disable the complete certificate verification. Apart from that, this key is just a hack. It controls only a small (and often misunderstood) aspect of the certificate verification and there is no configuration in LWP itself available for the rest. And I don't think it is even a good idea to let users disable certificate validation partly or completely in an easy way. Look at http://www2.dcsec.uni-hannover.de/files/android/p50-fahl.pdf what happens, if developers just disable SSL validation because it is more convenient. I propose to:
Sure, this might break code which just disabled certificate validation. But, if the user just disabled verification because it somehow did not work at this time, it might magically continue to work now with verification enabled (because of fixes in the code, usable CAs etc). And if the user disabled validation because this were non-public certificates, it might even be a good idea that it fails now and that the user learns how to do a proper validation by reading from documentation (e.g. importing CA or checking against a fingerprint). Sure, it might break the accuracy of internet resources. But there are enough resources out there which at the slightest problem recommend users to disable validation. Lots of these problems just have to do with missing certificate store or failure to use SNI, which are all solved with current versions of IO::Socket::SSL (but not Crypt::SSLeay). So I'm very glad if these bad recommendations don't work any longer and we can show the users the correct way. Remember, this is security. So it should be easy to do the right thing and it should be hard to do the wrong thing. |
I agree that the name is plain bad for something that actually disables all checks. I wanted to note that LWP already recommends using that (via the matching environment variable) to disable both checks, at that it seems that it always worked as an option to disable both checks. So harm was done already, and you seemed to prefer to avoid breaking existing behaviour even if it's bad and not documented (name checks disabled for I had a look at the history, and it's quite messy. Initially, LWP documentation mentioned
It makes sense to provide environment variable that disables all checks, for use cases where user don't want to or can't change a script to pass correct |
From the documentation the verify_hostname option in LWP::UserAgent version 6.06: So here it states clearly, that
If the part "If FALSE no checks are made" is taken out of the context of this key, it would claim that no (unspecified) checks are done. In this case one could assume that these unspecified checks refer to all certification checks. But, in the context of this key it describes only a single check: the verification of the hostname against the certificate. That is the only documentation of this variable in the documentation I can find and I cannot agree that it recommends to disable both checks. And from the git history this documentation never claimed this behavior. So there might be articles out there claiming this meaning, but not the documentation of LWP itself. There is a comment inside LWP::Protocol::https which interprets the key differently, but this is no user documentation (e.g. not available with perldoc).
I disagree. If you look into the efforts of the browsers to teach people that disabling verification is bad and to make it hard to disable it, I see no reason to make it easy for script users to disable verification. It should be possible for a developer (but not to easy), but much harder for a user. But, maybe one can find a compromise by adding a variable, which should not be set simply to a true value, but to a specific string like "I really want to disable any certificate checks and know that this is completely insecure". This is a strategy some tools use to make sure, that the user really read the documentation and is really sure, what (s)he s doing. And it has the added bonus of showing up always when somebody documents how to disable verification for LWP, thus pointing out the problems with disabling verification. |
I as referring to this message, which was already quoted in one of the comments above: https://github.com/libwww-perl/lwp-protocol-https/blob/c5f6cc5/lib/LWP/Protocol/https.pm#L39 |
Yeah, valid certificate. When I first noticed that Incidentally, LWP::protocol::https POD says: which seems to confirm the hypothesis that in the LWP world “hostname verification” might mean something different than in the rest of the universe.
There's also LWP POD: And the changelog:
I don't think offending users' intelligence is a valid strategy for writing security-sensitive software (or any software, really). |
|
@thoger : @jwilk : As for the environment variable: I don't want to offend the user, but I don't like the idea of making it too easy for the user to just disable verification without thinking too much about it. There is a reason, that the browsers made it harder to disable verification over the years and tried to add more documentation in their dialogs to make the users aware of the implications. If you read stackoverflow or similar, it is often the first attempt on any SSL problems to disable certificate verification. And lots of users assume, that they still have encryption without certificate validation, but don't realize that they are open to man-in-the-middle attacks and that all the encryption does not help against it. Thus my proposal might be a way to get your wish of an easy setting with an environment variable, but make the user at least think a bit longer when using it. |
|
The discussion is moving away from the original bug. This bug has a CVE now (e.g. it is considered a security problem) and I would suggest that we make a release out of the fixes, instead of letting everybody just pull in these fixes and apply them to their version (at least Debian and RedHat/Fedora did already). This patch does not interprets verify_hostname in a new way, but instead restores the behavior this option had from version 6.02 (03/2011) until version 6.04 (03/2013), e.g. before the bug was introduced by trying to fix RT#81948. The documentation for this option did not change all the time. I would suggest that we continue the rest of the discussion, e.g. adding an environment variable to disable all certification and how to fix the documentation, in new issues. |
|
Any news on this? |
|
Trying to tidy up the outstanding tickets for this. Can this now be closed as fixed? |
From looking at the code the issue is still the same. It is disabling any kind of validation ( hostname and trust chain and expiration and ...) if
This also means that the CVE from 3 years ago are still unresolved and that the issue is not fixed and that various distributions like Debian maintain there own patches to fix the CVE. |
This should fix https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=746579, https://bugzilla.redhat.com/show_bug.cgi?id=1094440