Attachment 696107 Details for Bug 909959 – Proposed isync upstream patch (against the master branch) to correct this issue

[patch] Proposed isync upstream patch (against the master branch) to correct this issue

isync-master-security-fix-add-SSL-subject-verification.patch (text/plain), 7.89 KB, created by Jan Lieskovsky on 2013-02-11 14:33:03 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jan Lieskovsky

Created: 2013-02-11 14:33:03 UTC

Size: 7.89 KB

patch

obsolete

>From bfe84e0dbbff4dfdcfdd89fbdc135c2189695077 Mon Sep 17 00:00:00 2001
>From: Oswald Buddenhagen <ossi@kde.org>
>Date: Sun, 3 Feb 2013 17:34:15 +0100
>Subject: [PATCH] security fix: add SSL subject verification
>
>we did not check a valid certificate's subject at all so far.
>this is no problem if the certificate file contains only exactly the
>wanted host's certificate - before revision 04fdf7d1 (dec 2000, < v0.4),
>this was even enforced (more or less - if the peer cert had been
>directly signed by a root cert, it would be accepted as well).
>however, when the file contains root certificates (like the system-wide
>certificate file typically does), any host with a valid certificate
>could pretend to be the wanted host.
>---
> src/socket.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 69 insertions(+), 5 deletions(-)
>
>diff --git a/src/socket.c b/src/socket.c
>index 22be813..880e6c5 100644
>--- a/src/socket.c
>+++ b/src/socket.c
>@@ -11,40 +11,41 @@
>  *
>  *  This program is distributed in the hope that it will be useful,
>  *  but WITHOUT ANY WARRANTY; without even the implied warranty of
>  *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>  *  GNU General Public License for more details.
>  *
>  *  You should have received a copy of the GNU General Public License
>  *  along with this program.  If not, see <http://www.gnu.org/licenses/>.
>  *
>  * As a special exception, mbsync may be linked with the OpenSSL library,
>  * despite that library's more restrictive license.
>  */
> 
> /* This must come before isync.h to avoid our #define S messing up
>  * blowfish.h on MacOS X. */
> #include <config.h>
> #ifdef HAVE_LIBSSL
> # include <openssl/ssl.h>
> # include <openssl/err.h>
> # include <openssl/hmac.h>
>+# include <openssl/x509v3.h>
> #endif
> 
> #include "isync.h"
> 
> #include <assert.h>
> #include <unistd.h>
> #include <stdlib.h>
> #include <stddef.h>
> #include <errno.h>
> #include <string.h>
> #include <fcntl.h>
> #include <sys/socket.h>
> #include <sys/ioctl.h>
> #include <netinet/in.h>
> #include <netinet/tcp.h>
> #include <arpa/inet.h>
> #include <netdb.h>
> 
> enum {
> 	SCK_CONNECTING,
>@@ -101,120 +102,183 @@ ssl_return( const char *func, conn_t *conn, int ret )
> static int
> compare_certificates( X509 *cert, X509 *peercert,
>                       unsigned char *peermd, unsigned peermdlen )
> {
> 	unsigned char md[EVP_MAX_MD_SIZE];
> 	unsigned mdlen;
> 
> 	/* Avoid CPU-intensive digest calculation if the certificates are
> 	 * not even remotely equal. */
> 	if (X509_subject_name_cmp( cert, peercert ) ||
> 	    X509_issuer_name_cmp( cert, peercert ))
> 		return -1;
> 
> 	if (!X509_digest( cert, EVP_sha1(), md, &mdlen ) ||
> 	    peermdlen != mdlen || memcmp( peermd, md, mdlen ))
> 		return -1;
> 
> 	return 0;
> }
> 
>+static int
>+host_matches( const char *host, const char *pattern )
>+{
>+	if (pattern[0] == '*' && pattern[1] == '.') {
>+		pattern += 2;
>+		if (!(host = strchr( host, '.' )))
>+			return 0;
>+		host++;
>+	}
>+
>+	return *host && *pattern && !strcasecmp( host, pattern );
>+}
>+
>+static int
>+verify_hostname( X509 *cert, const char *hostname )
>+{
>+	int i, len, found;
>+	X509_NAME *subj;
>+	STACK_OF(GENERAL_NAME) *subj_alt_names;
>+	char cname[1000];
>+
>+	/* try the DNS subjectAltNames */
>+	found = 0;
>+	if ((subj_alt_names = X509_get_ext_d2i( cert, NID_subject_alt_name, NULL, NULL ))) {
>+		int num_subj_alt_names = sk_GENERAL_NAME_num( subj_alt_names );
>+		for (i = 0; i < num_subj_alt_names; i++) {
>+			GENERAL_NAME *subj_alt_name = sk_GENERAL_NAME_value( subj_alt_names, i );
>+			if (subj_alt_name->type == GEN_DNS &&
>+			    strlen( (const char *)subj_alt_name->d.ia5->data ) == (size_t)subj_alt_name->d.ia5->length &&
>+			    host_matches( hostname, (const char *)(subj_alt_name->d.ia5->data) ))
>+			{
>+				found = 1;
>+				break;
>+			}
>+		}
>+		sk_GENERAL_NAME_pop_free( subj_alt_names, GENERAL_NAME_free );
>+	}
>+	if (found)
>+		return 0;
>+
>+	/* try the common name */
>+	if (!(subj = X509_get_subject_name( cert ))) {
>+		error( "Error, cannot get certificate subject\n" );
>+		return -1;
>+	}
>+	if ((len = X509_NAME_get_text_by_NID( subj, NID_commonName, cname, sizeof(cname) )) < 0) {
>+		error( "Error, cannot get certificate common name\n" );
>+		return -1;
>+	}
>+	if (strlen( cname ) == (size_t)len && host_matches( hostname, cname ))
>+		return 0;
>+
>+	error( "Error, certificate owner does not match hostname %s\n", hostname );
>+	return -1;
>+}
>+
> #if OPENSSL_VERSION_NUMBER >= 0x00904000L
> #define READ_X509_KEY(fp, key) PEM_read_X509( fp, key, 0, 0 )
> #else
> #define READ_X509_KEY(fp, key) PEM_read_X509( fp, key, 0 )
> #endif
> 
> /* this gets called when a certificate is to be verified */
> static int
> verify_cert( const server_conf_t *conf, conn_t *sock )
> {
> 	server_conf_t *mconf = (server_conf_t *)conf;
> 	SSL *ssl = sock->ssl;
> 	X509 *cert, *lcert;
> 	BIO *bio;
> 	FILE *fp;
> 	int err;
> 	unsigned n, i;
> 	X509_STORE_CTX xsc;
> 	char buf[256];
> 	unsigned char md[EVP_MAX_MD_SIZE];
> 
> 	cert = SSL_get_peer_certificate( ssl );
> 	if (!cert) {
> 		error( "Error, no server certificate\n" );
> 		return -1;
> 	}
> 
> 	while (conf->cert_file) { /* while() instead of if() so break works */
>+		/* Note: this code intentionally does no hostname verification. */
>+
> 		if (X509_cmp_current_time( X509_get_notBefore( cert )) >= 0) {
> 			error( "Server certificate is not yet valid\n" );
> 			break;
> 		}
> 		if (X509_cmp_current_time( X509_get_notAfter( cert )) <= 0) {
> 			error( "Server certificate has expired\n" );
> 			break;
> 		}
> 		if (!X509_digest( cert, EVP_sha1(), md, &n )) {
> 			error( "*** Unable to calculate digest\n" );
> 			break;
> 		}
> 		if (!(fp = fopen( conf->cert_file, "rt" ))) {
> 			sys_error( "Unable to load CertificateFile '%s'", conf->cert_file );
> 			return -1;
> 		}
> 		err = -1;
> 		for (lcert = 0; READ_X509_KEY( fp, &lcert ); )
>-			if (!(err = compare_certificates( lcert, cert, md, n )))
>+			if (!(err = compare_certificates( lcert, cert, md, n ))) /* TODO: check X509v3 [Extended] Key Usage? */
> 				break;
> 		X509_free( lcert );
> 		fclose( fp );
> 		if (!err)
> 			return 0;
> 		break;
> 	}
> 
> 	if (!mconf->cert_store) {
> 		if (!(mconf->cert_store = X509_STORE_new())) {
> 			error( "Error creating certificate store\n" );
> 			return -1;
> 		}
> 		if (!X509_STORE_set_default_paths( mconf->cert_store ))
> 			warn( "Error while loading default certificate files: %s\n",
> 			      ERR_error_string( ERR_get_error(), 0 ) );
> 		if (!conf->cert_file) {
> 			info( "Note: CertificateFile not defined\n" );
> 		} else if (!X509_STORE_load_locations( mconf->cert_store, conf->cert_file, 0 )) {
> 			error( "Error while loading certificate file '%s': %s\n",
> 			       conf->cert_file, ERR_error_string( ERR_get_error(), 0 ) );
> 			return -1;
> 		}
> 	}
> 
> 	X509_STORE_CTX_init( &xsc, mconf->cert_store, cert, 0 );
> 	err = X509_verify_cert( &xsc ) > 0 ? 0 : X509_STORE_CTX_get_error( &xsc );
> 	X509_STORE_CTX_cleanup( &xsc );
>-	if (!err)
>-		return 0;
>-	error( "Error, cannot verify certificate: %s (%d)\n",
>-	       X509_verify_cert_error_string( err ), err );
>+	if (err) {
>+		error( "Error, cannot verify certificate: %s (%d)\n",
>+		       X509_verify_cert_error_string( err ), err );
>+		goto intcheck;
>+	}
>+	if (conf->host && verify_hostname( cert, conf->host ) < 0)
>+		goto intcheck;
>+	return 0;
> 
>+  intcheck:
> 	X509_NAME_oneline( X509_get_subject_name( cert ), buf, sizeof(buf) );
> 	info( "\nSubject: %s\n", buf );
> 	X509_NAME_oneline( X509_get_issuer_name( cert ), buf, sizeof(buf) );
> 	info( "Issuer:  %s\n", buf );
> 	bio = BIO_new( BIO_s_mem() );
> 	ASN1_TIME_print( bio, X509_get_notBefore( cert ) );
> 	memset( buf, 0, sizeof(buf) );
> 	BIO_read( bio, buf, sizeof(buf) - 1 );
> 	info( "Valid from: %s\n", buf );
> 	ASN1_TIME_print( bio, X509_get_notAfter( cert ) );
> 	memset( buf, 0, sizeof(buf) );
> 	BIO_read( bio, buf, sizeof(buf) - 1 );
> 	BIO_free( bio );
> 	info( "      to:   %s\n", buf );
> 	if (!X509_digest( cert, EVP_md5(), md, &n )) {
> 		error( "*** Unable to calculate fingerprint\n" );
> 	} else {
> 		info( "Fingerprint: " );
> 		for (i = 0; i < n; i += 2)
> 			info( "%02X%02X ", md[i], md[i + 1] );
>-- 
>1.8.0.1.18.g93341d8
>

Actions: View | Diff

Attachments on bug 909959: 696105 | 696107