Bug 2718 - Failure to check peer hostname when checking certificate
Summary: Failure to check peer hostname when checking certificate
Status: RESOLVED FIXED
Alias: None
Product: Claws Mail
Classification: Unclassified
Component: Other (show other bugs)
Version: 3.8.1
Hardware: PC Linux
: P3 normal
Assignee: users
URL:
Depends on:
Blocks:
 
Reported: 2012-08-05 17:59 CEST by Dominique Leuenberger
Modified: 2012-08-29 15:40 CEST (History)
0 users

See Also:


Attachments
Attempt to fix point 3 (6.01 KB, patch)
2012-08-09 13:05 CEST, Ricardo Mones
no flags Details | Diff
Pseudo-patch to use directory-based trust CA certs (845 bytes, patch)
2012-08-09 13:55 CEST, Colin Leroy
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dominique Leuenberger 2012-08-05 17:59:45 CEST
A security review came up with this result:

1. claws_ssl_get_cert_file() doesn't try any existing bundle file so
   the included bundle isn't used either
2. the return value of gnutls_certificate_verify_peers2() isn't
   used. Instead claws always runs into the code path for
   self-signed certificates (ie prompts for confirm)
3. claws does not call gnutls_x509_crt_check_hostname() which would
   make it prone to MITM. Due to 2) that's not a problem though.
Comment 1 Dominique Leuenberger 2012-08-05 18:01:13 CEST
Part 1 was addressed in the distribution with a patch, adding the path to the 'own' bundle to the list of known ones:
+		"/etc/ssl/ca-bundle.pem",
Comment 2 Ricardo Mones 2012-08-09 09:29:41 CEST
1. I don't think there's a problem to add the new path to the current existing collection.

2. Doesn't seem clear to me, from reading https://bugzilla.novell.com/show_bug.cgi?id=761503#c12 if this is still an issue or not... but seems it's not, is it?

3. Also don't think there's a problem with adding this, but a patch would be appreciated, of course. Anyway from RFC 2818 section 3.1:
"
If the client has external information as to the expected identity of
the server, the hostname check MAY be omitted [...]
"
So I think hostname check should not be done after prompting certificate prompt, as the user has already validated it (otherwise could faild leading to a new prompt and user annoyance ;-)
Comment 3 Dominique Leuenberger 2012-08-09 09:33:50 CEST
1. Agree => Nothing more to be done from your end

2. I think a confirmation from your end that I read that code all right is in order.

3. The 'issue' arises if the certificate is certified to a trusted root. In this case, the user is not prompted, won't see the cert details but there is no verification if the presented certificate belongs to the hostname we tried to connect to. As such, MITM seems easily possible. This seems to be the main issue at the moment.
Comment 4 Colin Leroy 2012-08-09 12:09:37 CEST
Hi,

FYI, I am working on that.
Comment 5 Ricardo Mones 2012-08-09 13:05:26 CEST
Created attachment 1142 [details]
Attempt to fix point 3

I was working also on a patch, but still not completely finished, it just compiles, but I'm glad you're working on it.

Just attaching the current patch in case it could save you some time, but probably just garbage.
Comment 6 Colin Leroy 2012-08-09 13:26:01 CEST
Hi Ricardo,

Thanks ! I've taken a different way, where I don't plan on checking hostname matching for known saved certificates, there's no need to in my opinion (I don't want to bother users telling them everytime for something they may not be able to change. We already do this for expired certificates, but there the admin fix for this is easier).

So I plan on checking hostname in each of the other cases.
Comment 7 users 2012-08-09 13:33:34 CEST
Changes related to this bug have been committed.
Please check latest CVS and update the bug accordingly.
You can also get the patch from:
http://www.claws-mail.org/tracker/

2012-08-09 [colin]	3.8.1cvs26

	* src/common/ssl.c
	* src/common/ssl_certificate.c
	* src/common/ssl_certificate.h
	* src/gtk/sslcertwindow.c
		Fix bug 2718, "Failure to check peer hostname
		when checking certificate"
Comment 8 Ricardo Mones 2012-08-09 13:35:33 CEST
IMHO if the hostname doesn't match certificate name then the admin should also fix the certificate, otherwise I think we're leaving these known ones vulnerable to MITM attacks.

But it may happen I had not understood it well, so more insight welcome.
Comment 9 Colin Leroy 2012-08-09 13:38:59 CEST
Hi,

to sum it up, Dominique:

1) Only the Win32 version uses an included bundle, the *nix version tries to find various distro-dependent file bundles. Your distro's path was left out, sorry about that.

2) Yes, we always prompt users when hitting a new, previously unknown certificate, be it "correctly" signed or not. (I don't think that root-CA-signed certificates are anymore trustworthy than self-signed certificates). This alleviates point 3, indeed. We also ask the user when a known certificate changes (after renewal for example) and when it's expired.

3) Even if the user was asked for acceptation when reaching an unknown certificate, the hostname check was "to be done by the user", which isn't very good indeed. The patch I just commited fixes that by adding a warning about the hostname in the SSL certificates alertpanels.
Comment 10 Colin Leroy 2012-08-09 13:44:53 CEST
> IMHO if the hostname doesn't match certificate name then the admin should also
> fix the certificate,

I'm thinking it would bother numerous users using servers serving for multiple domains with only one certificate matching only the real FQDN of the server. For example, my server serves my domain, Paul's domain and three others, and the certificate only matches my domain.

> otherwise I think we're leaving these known ones vulnerable to MITM attacks.

If the known ones are already subject of MITM, yes, but in any case that would be too late for those.

If a MITM is attempted on clients for which the server certificate is already known, the certificate would have to be changed, thus triggering the alert (with or without my just-commited patch).
Comment 11 Dominique Leuenberger 2012-08-09 13:46:49 CEST
> I'm thinking it would bother numerous users using servers serving for multiple
> domains with only one certificate matching only the real FQDN of the server.
> For example, my server serves my domain, Paul's domain and three others, and
> the certificate only matches my domain.

That's up to the admin to create proper and valid certificates... alternativ subjects have been long supported in X.509
Comment 12 Colin Leroy 2012-08-09 13:54:34 CEST
Dominique, one more thing 'cause I don't have an account on Novell's bugzilla:

https://bugzilla.novell.com/show_bug.cgi?id=761503#c5

We have a list of certs directories too, which we used back in the days where our SSL implementation was OpenSSL-based. We added the files when switching to GnuTLS which didn't at the time provide a way to provide a directory of trusted CA certs.

If GnuTLS now provides that, I'll include a patch to use directories instead of files when possible, when my distro will ship a recent enough GnuTLS so that I can test -- or when someone writes it, it should be rather trivial like in the upcoming attachment.
Comment 13 Colin Leroy 2012-08-09 13:55:41 CEST
Created attachment 1143 [details]
Pseudo-patch to use directory-based trust CA certs
Comment 14 Ricardo Mones 2012-08-09 14:04:42 CEST
We already have the hidden property to skip checking certificates, but if a user choose to check them, I think the check should be as complete and confident as possible, no matter the certificate is new or already known or how lazy the server admin was when generating the certificate.

But that's my opinion only.
Comment 15 Colin Leroy 2012-08-09 14:59:38 CEST
Ricardo: I have no hard feelings about this. I'm just not sure how useful that would be, and trying to weigh that usefulness against dozens of annoyed users after the next release.

What do other people think ? Paul, wwp, the whole rest of the team, any opinion ?
Comment 16 Andrej Kacian 2012-08-09 15:26:21 CEST
I agree with Ricardo, if we are doing (skip_ssl_cert_check=0) a certificate check, we should really do a certificate check, with all bells and whistles. 

Users annoyed by C-M pointing out hostname mismatch for their server should be silenced by this being clearly mentioned in release notes, and the ability to click "Accept" and never see such warning for a given server again (unless the certificate changes, of course).
Comment 17 Holger Berndt 2012-08-09 23:46:21 CEST
I think Ricardo has a point. However, the downside of bothering users with that kind of decision (where most of them don't know what this weird popup is talking about) is that we're basically conditioning users to either just accept the stuff that pops up, or to turn that hidden pref on. Both may result in a loss of security.

So, no hard feelings from me either. The traditional Claws Mail way is probably to do it accurately, and ask the (supposedly expert) user.

(Silencing via Release Notes, that's a good one :D)