Certificate handling problem #54

Closed
Papafox opened this Issue May 5, 2013 · 7 comments

Projects

None yet

2 participants

@Papafox
Papafox commented May 5, 2013

Google is evil - they started using two different certificates which have the same subject and the same issuer, but different validity dates and, obviously, different signatures. This broke imapfilter and caused me enough irritation that I dug into the code developed a patch.

The problem is in cert.c It compares the subject and issuer and skips to the next iteration if they don't match. If they do match, it compares the two signatures and if they don't match throws an error.

My fix changes this logic to simply read every certificate in the file and compare the signature with the signature of the server certificate. If no match is found it defaults to an error "certificate mismatch" (I haven't changed the error text, you may wish to consider it).

I've added some header text for each certificate in the file to make things a little clearer.

diff --git src/cert.c src/cert.c
index 51acf3f..a26342f 100644
--- src/cert.c
+++ src/cert.c
@@ -85,7 +85,7 @@ check_cert(X509 *pcert, unsigned char *pmd, unsigned int *pmdlen)
    unsigned char md[EVP_MAX_MD_SIZE];
    unsigned int mdlen;

-   r = 0;
+   r = -1;
    cert = NULL;

    certf = get_filepath("certificates");
@@ -99,20 +99,19 @@ check_cert(X509 *pcert, unsigned char *pmd, unsigned int *pmdlen)
        return -1;

    while ((cert = PEM_read_X509(fd, &cert, NULL, NULL)) != NULL) {
-       if (X509_subject_name_cmp(cert, pcert) != 0 ||
-           X509_issuer_name_cmp(cert, pcert) != 0)
-           continue;
-
+       // Compute MD5 signature for the certificate just read.
+       // Make sure it's valid 
        if (!X509_digest(cert, EVP_md5(), md, &mdlen) ||
            *pmdlen != mdlen)
            continue;

-       if (memcmp(pmd, md, mdlen) != 0) {
-           r = -1;
+       // Compare the signature for the certificate from the server
+       // against the signature of the certificate just read.
+       // If they match exit the loop
+       if (memcmp(pmd, md, mdlen) != 0) {
            continue;
        }
-       r = 1;
-       break;
    }

    fclose(fd);
@@ -175,6 +174,10 @@ write_cert(X509 *cert)
    if (fd == NULL)
        return -1;

+   fprintf(fd, "##----------------------------------------------------------\n");
+   fprintf(fd, "## Subject: %s\n", X509_NAME_oneline(X509_get_subject_name(cert), NULL, 0));
+   fprintf(fd, "## Issuer : %s\n", X509_NAME_oneline(X509_get_issuer_name(cert), NULL, 0));
+   fprintf(fd, "##----------------------------------------------------------\n");
    PEM_write_X509(fd, cert);

    fclose(fd);
@lefcha
Owner
lefcha commented May 6, 2013

Am I wrong thinking that with this code there will never be a chance for the user to accept a new certificate? Because with the first change made on the above diff where r = 0; becomes r = -1;, the r variable can never take the value 0, which should happen when the certificate was not found in the file.

Also, how are we going to know that a certificate that seems to be from the same Issuer has the same Subject, is a new valid certificate, and not a fake one from someone with malicious intentions? Every time a server sends a new certificate very similar to the old certificate, will the user just accept it? Shouldn't this make the user suspicious, instead of just accepting the new certificate?

@Papafox
Papafox commented May 10, 2013

Yes, you are correct the patch broke more than it fixed. I've reworked it as follows:

In check_cert(), the call to X509_issuer_name_cmp() has been replaced by a call to X509_issuer_and_serial_cmp(). This one line is the only necessary change to fix the problem.

However, I feel that comments are required to make the certificates file readable. This is simple enough - just the fprintf(0)'s. However, documenting the serial no required a new routine to convert an ASN1_INTEGER to a string. ASN1_INTEGER's are theoretically decimal numbers, but can be of an arbitrary length. So get_serialNumber() treats ASN1_INTEGERS less than or equal to 64-bits as a decimal, but simply dumps longer numbers in hex.

The existing code made no attempt to validate the certificate - it doesn't even check the validity dates. Adding validation would be a major change, partly for structural issues - openssl uses the SSL_CTX object rather than the SSL object, so the context would need to be worked into the imapfilter ssn object. Since the ssn is used in a lot of places, this seems like a lot of work.

The other reason is that it would be necessary to build a trust store and populate it with CA/Root certificates. While most Linux distro's have the Mozilla certificates in /usr/share/ca-certificates/mozilla, not all do. So first there is the issue of make imapfilter dependent on the mozilla ca-certs or distribute our own. The problem with the dependency is obvious. the problem with distributing our own list is that it would have to be maintain, and that is a lot of work - perhaps a day per month. Anyway, I'm leaving it in the too-hard basket at the moment.

diff --git src/cert.c src/cert.c
index 51acf3f..f1520d0 100644
--- src/cert.c
+++ src/cert.c
@@ -21,7 +21,7 @@ int check_cert(X509 *pcert, unsigned char *pmd, unsigned int *pmdlen);
 void print_cert(X509 *cert, unsigned char *md, unsigned int *mdlen);
 int write_cert(X509 *cert);
 int mismatch_cert(void);
-
+char* get_serialNumber(X509 *cert);

 /*
  * Get SSL/TLS certificate check it, maybe ask user about it and act
@@ -100,7 +100,7 @@ check_cert(X509 *pcert, unsigned char *pmd, unsigned int *pmdlen)

    while ((cert = PEM_read_X509(fd, &cert, NULL, NULL)) != NULL) {
        if (X509_subject_name_cmp(cert, pcert) != 0 ||
-           X509_issuer_name_cmp(cert, pcert) != 0)
+           X509_issuer_and_serial_cmp(cert, pcert) != 0)
            continue;

        if (!X509_digest(cert, EVP_md5(), md, &mdlen) ||
@@ -144,6 +144,45 @@ print_cert(X509 *cert, unsigned char *md, unsigned int *mdlen)
        printf(i != *mdlen - 1 ? "%02X:" : "%02X\n", md[i]);
 }

+/*
+ * Extract a certificates serial number as a string
+ * Remember the free the string using xfree() after.
+ */
+char*
+get_serialNumber(X509 *cert)
+{
+    ASN1_INTEGER* serial;
+    long l;
+    char *neg;
+    char *buff;
+    int  i;
+    
+    buff = xmalloc(128);
+    serial = X509_get_serialNumber(cert);
+    if (serial->length <= (int)sizeof(long)) {
+   l = ASN1_INTEGER_get(serial);
+   if (serial->type == V_ASN1_NEG_INTEGER) {
+       l = -l;
+       neg = "-";
+   }
+   else
+       neg = "";
+   sprintf(buff,"%s%lu (%s0x%lx)",neg,l,neg,l);
+    }
+    else {
+   neg = (serial->type == V_ASN1_NEG_INTEGER)?" (Negative)":"";
+   sprintf(buff,"%s",neg);
+   char *p = buff + strlen(buff);
+
+   for (i = 0; i < serial->length; i++) {
+       sprintf(p+i*3,"%02x%c",serial->data[i], ((i+1 == serial->length)?' ':':'));
+   }
+    }
+    
+    return buff;
+}
+
+

 /*
  * Write the SSL/TLS certificate after asking the user to accept/reject it.
@@ -154,6 +193,7 @@ write_cert(X509 *cert)
    FILE *fd;
    char c, buf[64];
    char *certf;
+   char *serial;

    do {
        printf("(R)eject, accept (t)emporarily or "
@@ -174,9 +214,17 @@ write_cert(X509 *cert)
    xfree(certf);
    if (fd == NULL)
        return -1;
+   serial = get_serialNumber(cert);

+   fprintf(fd,"##--------------------------------------------------------------\n");
+   fprintf(fd,"## Subject: %s\n", X509_NAME_oneline(X509_get_subject_name(cert), NULL, 0));
+   fprintf(fd,"## Issuer : %s\n", X509_NAME_oneline(X509_get_issuer_name(cert), NULL, 0));
+   fprintf(fd,"## Serial : %s\n", serial);
+   fprintf(fd,"##--------------------------------------------------------------\n");
    PEM_write_X509(fd, cert);
-
+   fprintf(fd,"\n");
+   
+   xfree(serial);
    fclose(fd);

    return 0;
@Papafox
Papafox commented May 11, 2013

I've forked the code, so that you can run the test case called 'testcert'. All the changes for the actual fix are contained in cert.c, so that it the only file you need to copy.

@lefcha
Owner
lefcha commented May 11, 2013

I agree with your change to the cert.c file, checking issuer and serial number is better and fixes this problem, this is great and with no side effects as far as I can see, thanks a lot!

Regarding adding information inside the certs files, I also think it's a good idea to have the details of each certificate in there, since you can't know which is which easily (eg. in case you want to remove some certificate). I think it can be done command line with openssl x509 and some bash scripting, but it is inconvenient.

But I have one question: is it valid to have anything you want in a certificates file, between the certificates like that? Is there any way to add a comment in the specification, or is anything that is not between the -----BEGIN CERTIFICATE----- and -----END CERTIFICATE----- lines ignored? Do you have any idea about this or any pointers to a specification documenting this? I couldn't verify this, and it worries me a bit. Maybe it's the way the OpenSSL library works and we are covered for now...

@Papafox
Papafox commented May 11, 2013

lefcha said:

But I have one question: is it valid to have anything you want in a certificates file,
between the certificates like that? Is there any way to add a comment in the specification,
or is anything that is not between the -----BEGIN CERTIFICATE----- and
-----END CERTIFICATE----- lines ignored?

I can say with certainty that PEM_read_X509() will ignore any lines preceding -----BEGIN CERTIFICATE-----. I know this because I wrote that part of the code in the original SSLeay - although I note that OpenSSL has rewritten all PEM I/O code. I know Eric Young and Tim Hudson and I was involved in writing small parts SSLeay (all the PKCS#7 and S/Mime code is mine) I only wrote maybe 1% of SSLeay, but I gained a good knowledge of how it works.

Do you have any idea about this or any pointers to a specification documenting this?
I couldn't verify this, and it worries me a bit. Maybe it's the way the OpenSSL library
works and we are covered for now..

Documentation? OpenSSL doesn't need no stinking documentation :-)

Actually, SSLeay had sparse but usable docs, but somehow/somewhere it was lost in the change to OpenSSL.

As with all OpenSSL features, you need to check the source. When the OpenSSL folks rewrote the PEM routines, they used a lot of preprocessor macros which makes understanding the code even more difficult. Basically PEM_read_X509() is implemented in PEM_bytes_read_bio(). The first of the NULL parameters in PEM_read_X509() is replaced by the string -----BEGIN CERTIFICATE-----. You can see in the code (crypto/pem/pem_lib.c). The code has an infinte loop reading lines looking for a specific string. If it's not found, it issues error PEM_R_NO_START_LINE.

The best demonstration is to run my code. It adds new certs to the .imapfilter/certificates file with the heading lines. check_cert() will then read each the certs (including the headings) to match the server cert.

The test driver code is in testcert.c. It reads a certificate from the test director and calls check_cert(). My code is a bit sloppy as I have the test cert file names hard coded, with one active and the others commented out. If you look at around line 73 in testcert.c, you'll see:

    char *filename = "google_1.pem";
//    char *filename = "iinet.pem";
//    char *filename = "google_2.pem";

I extracted these certs from imap servers using OpenSSL: openssl s_client -connect imap.google.com:993 -showcerts.

So, the steps are:

  1. Clean out the .imapfilter/certificates file, removing all certificates
  2. Build the project, compiling all the modules and creating testcert among other code
  3. Run testcert with the first certificate.
  4. Modify testcert.c, rebuild and rerun testcert using the second certificate
  5. Modify testcert.c, rebuild and rerun testcert using the third certificate

After all that, the .imapfilter/certificates file should look like:

##--------------------------------------------------------------
## Subject: /C=US/ST=California/L=Mountain View/O=Google Inc/CN=imap.gmail.com
## Issuer : /C=US/O=Google Inc/CN=Google Internet Authority
## Serial : 3b:73:26:8b:00:00:00:00:68:a5 
##--------------------------------------------------------------
-----BEGIN CERTIFICATE-----
MIIDgDCCAumgAwIBAgIKO3MmiwAAAABopTANBgkqhkiG9w0BAQUFADBGMQswCQYD
VQQGEwJVUzETMBEGA1UEChMKR29vZ2xlIEluYzEiMCAGA1UEAxMZR29vZ2xlIElu
dGVybmV0IEF1dGhvcml0eTAeFw0xMjA5MTIxMTU1NDlaFw0xMzA2MDcxOTQzMjda
MGgxCzAJBgNVBAYTAlVTMRMwEQYDVQQIEwpDYWxpZm9ybmlhMRYwFAYDVQQHEw1N
b3VudGFpbiBWaWV3MRMwEQYDVQQKEwpHb29nbGUgSW5jMRcwFQYDVQQDEw5pbWFw
LmdtYWlsLmNvbTCBnzANBgkqhkiG9w0BAQEFAAOBjQAwgYkCgYEA2OmU9DjI+DFQ
ThqIN4vL6EqZbzH0ejLKcc+zhxsq9BU5hXohSJ1sS5FUU2vReDKk8fd+ZR3cWtpf
CTYAUSvdnz1ZFjESSzyUBmGRqByhoc0yqdfb61NosA4CDaO+z7DtAgKyecqnAJad
TPYYf9aLk/UgJuc6GseitjzFYonXi6ECAwEAAaOCAVEwggFNMB0GA1UdJQQWMBQG
CCsGAQUFBwMBBggrBgEFBQcDAjAdBgNVHQ4EFgQUFuLyTg2NcsyaEESytZbLbQan
YIowHwYDVR0jBBgwFoAUv8Aw6/VDET5nup6R+/xq2uNrEiQwWwYDVR0fBFQwUjBQ
oE6gTIZKaHR0cDovL3d3dy5nc3RhdGljLmNvbS9Hb29nbGVJbnRlcm5ldEF1dGhv
cml0eS9Hb29nbGVJbnRlcm5ldEF1dGhvcml0eS5jcmwwZgYIKwYBBQUHAQEEWjBY
MFYGCCsGAQUFBzAChkpodHRwOi8vd3d3LmdzdGF0aWMuY29tL0dvb2dsZUludGVy
bmV0QXV0aG9yaXR5L0dvb2dsZUludGVybmV0QXV0aG9yaXR5LmNydDAMBgNVHRMB
Af8EAjAAMBkGA1UdEQQSMBCCDmltYXAuZ21haWwuY29tMA0GCSqGSIb3DQEBBQUA
A4GBAC1LV7tM6pcyVJLcwdPml4DomtowsjTrqvy5ZFa3SMKANK0iZBgFu74O0THX
8SxP/vn4eAs0yRQxcT1ZuoishLGQl5NoimLaQ4BGQnzFQHDJendfaVKDl21GenJp
is72sIrAeprsVU8PbNsllUamWsIjKr3DH5xQdH54hDtzQojY
-----END CERTIFICATE-----

##--------------------------------------------------------------
## Subject: /C=US/ST=California/L=Mountain View/O=Google Inc/CN=imap.gmail.com
## Issuer : /C=US/O=Google Inc/CN=Google Internet Authority
## Serial : 54:4b:1b:b5:00:01:00:00:84:2e 
##--------------------------------------------------------------
-----BEGIN CERTIFICATE-----
MIIDgDCCAumgAwIBAgIKVEsbtQABAACELjANBgkqhkiG9w0BAQUFADBGMQswCQYD
VQQGEwJVUzETMBEGA1UEChMKR29vZ2xlIEluYzEiMCAGA1UEAxMZR29vZ2xlIElu
dGVybmV0IEF1dGhvcml0eTAeFw0xMzA0MTUwODQ0MDBaFw0xMzEyMzExNTU4NTBa
MGgxCzAJBgNVBAYTAlVTMRMwEQYDVQQIEwpDYWxpZm9ybmlhMRYwFAYDVQQHEw1N
b3VudGFpbiBWaWV3MRMwEQYDVQQKEwpHb29nbGUgSW5jMRcwFQYDVQQDEw5pbWFw
LmdtYWlsLmNvbTCBnzANBgkqhkiG9w0BAQEFAAOBjQAwgYkCgYEA3a/wUjZBSOgZ
EeyRqaSaKEwS8+1y/8AK9HdplSR72PU+iBc7HyA4aXgD6XYEJVoyGsO97nMj+oeN
2iNvKfkPvTrn2YnQfJLuxpEw9gwIHvwVqy3TNpHwt4DHnxOg5CxV8e7PaCAhAXD+
uj0H09aVFJmfYDnU0VSSukNJX2MZSJUCAwEAAaOCAVEwggFNMB0GA1UdJQQWMBQG
CCsGAQUFBwMBBggrBgEFBQcDAjAdBgNVHQ4EFgQUY9A6EExy3NNFBc2R0vrY8lpf
OB8wHwYDVR0jBBgwFoAUv8Aw6/VDET5nup6R+/xq2uNrEiQwWwYDVR0fBFQwUjBQ
oE6gTIZKaHR0cDovL3d3dy5nc3RhdGljLmNvbS9Hb29nbGVJbnRlcm5ldEF1dGhv
cml0eS9Hb29nbGVJbnRlcm5ldEF1dGhvcml0eS5jcmwwZgYIKwYBBQUHAQEEWjBY
MFYGCCsGAQUFBzAChkpodHRwOi8vd3d3LmdzdGF0aWMuY29tL0dvb2dsZUludGVy
bmV0QXV0aG9yaXR5L0dvb2dsZUludGVybmV0QXV0aG9yaXR5LmNydDAMBgNVHRMB
Af8EAjAAMBkGA1UdEQQSMBCCDmltYXAuZ21haWwuY29tMA0GCSqGSIb3DQEBBQUA
A4GBAAcrDCcXCKZ2VNcJv31SSXTKs1AH0sU1lvAB0kzy3mIB/H8UHvMz1+T3Lfmy
68bqBSM97W6MO6UiqmVvbMhwPBrktUVT/Q4cWskVf2MONrW3g0UtX47L1ocs/WZe
XdUTkjQ3EFCzxpw4joHefndfZHsEn0VrjZR49kzR9+1Me7Rz
-----END CERTIFICATE-----

##--------------------------------------------------------------
## Subject: /C=AU/postalCode=6000/ST=WA/L=Perth/street=263 Adelaide Tce/street=Durack House/street=Level 6/O=iiNet Ltd/OU=Network Services/OU=PlatinumSSL Wildcard/CN=*.iinet.net.au
## Issuer : /C=GB/ST=Greater Manchester/L=Salford/O=COMODO CA Limited/CN=COMODO High-Assurance Secure Server CA
## Serial : 8c:7f:5b:ca:64:29:c4:2c:5e:4a:ae:f3:2d:b3:18:36 
##--------------------------------------------------------------
-----BEGIN CERTIFICATE-----
MIIFwjCCBKqgAwIBAgIRAIx/W8pkKcQsXkqu8y2zGDYwDQYJKoZIhvcNAQEFBQAw
gYkxCzAJBgNVBAYTAkdCMRswGQYDVQQIExJHcmVhdGVyIE1hbmNoZXN0ZXIxEDAO
BgNVBAcTB1NhbGZvcmQxGjAYBgNVBAoTEUNPTU9ETyBDQSBMaW1pdGVkMS8wLQYD
VQQDEyZDT01PRE8gSGlnaC1Bc3N1cmFuY2UgU2VjdXJlIFNlcnZlciBDQTAeFw0x
MTExMTcwMDAwMDBaFw0xNjEyMDEyMzU5NTlaMIHkMQswCQYDVQQGEwJBVTENMAsG
A1UEERMENjAwMDELMAkGA1UECBMCV0ExDjAMBgNVBAcTBVBlcnRoMRkwFwYDVQQJ
ExAyNjMgQWRlbGFpZGUgVGNlMRUwEwYDVQQJEwxEdXJhY2sgSG91c2UxEDAOBgNV
BAkTB0xldmVsIDYxEjAQBgNVBAoTCWlpTmV0IEx0ZDEZMBcGA1UECxMQTmV0d29y
ayBTZXJ2aWNlczEdMBsGA1UECxMUUGxhdGludW1TU0wgV2lsZGNhcmQxFzAVBgNV
BAMUDiouaWluZXQubmV0LmF1MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKC
AQEAs4vMKYcCS0d005C0hSGuP/oT8HrLfGbS+RGRkYHqUfWTlfiyInqkI2gaXzZw
B2pbOR2qaC518auAf2i8UgtZIES18CPEtVOroSUDdmrZaKmPjPS4WHAMnmUqf+Qq
KV32LWeG4vtCHpp4F96gtTHCFyE9Iz1xU+lm8GlFR7IUmMXrjgWaMiZIXhzcmjYV
k7wJorPrzQePsYFrewzuwGJleEsVGcG74DvA781le0feQI7Bbk0jNekqP/ldLQVS
D55OOfkZ83OIdMR2ZOtbPFy2a6CUm68ft4IqWw9cjhkcUcz8q4GwAKRDO9B7Eo41
5C0w5vRkgnjI5xOeusqYzylWAwIDAQABo4IBxjCCAcIwHwYDVR0jBBgwFoAUP9W1
0NZEeVBKF6ObjErcuLAiZGswHQYDVR0OBBYEFFa3uiBvunwlvr3FGisBmeRI7+OD
MA4GA1UdDwEB/wQEAwIFoDAMBgNVHRMBAf8EAjAAMB0GA1UdJQQWMBQGCCsGAQUF
BwMBBggrBgEFBQcDAjBGBgNVHSAEPzA9MDsGDCsGAQQBsjEBAgEDBDArMCkGCCsG
AQUFBwIBFh1odHRwczovL3NlY3VyZS5jb21vZG8uY29tL0NQUzBPBgNVHR8ESDBG
MESgQqBAhj5odHRwOi8vY3JsLmNvbW9kb2NhLmNvbS9DT01PRE9IaWdoLUFzc3Vy
YW5jZVNlY3VyZVNlcnZlckNBLmNybDCBgAYIKwYBBQUHAQEEdDByMEoGCCsGAQUF
BzAChj5odHRwOi8vY3J0LmNvbW9kb2NhLmNvbS9DT01PRE9IaWdoLUFzc3VyYW5j
ZVNlY3VyZVNlcnZlckNBLmNydDAkBggrBgEFBQcwAYYYaHR0cDovL29jc3AuY29t
b2RvY2EuY29tMCcGA1UdEQQgMB6CDiouaWluZXQubmV0LmF1ggxpaW5ldC5uZXQu
YXUwDQYJKoZIhvcNAQEFBQADggEBAAVsTkb+s9AZz5EO5MJGESxcIQzGTJo5g0lo
ekYIyIKhE0XSzXAjwjCIkte0RAXf7VOO96I8a9tXTVpAFAx4IGP1ugWp3SkZJWeC
7A+8uTrAChRrT2x0+j9ZJFDVsfdWZTySTcXBtqNH1qnnH5zyrV+EU8WB11iI6Zkq
SoNDtutAH+zZJNMXdkvzk5BtT4Q8lO/p6Fck/6pYqLH+RRNoOcv086Bq8MrDDn1l
xD5DzJ5vqGL8fGabSXQMAZYRByi1sjJZ/VTf6voQHr+JxNMoYkxnKBatnAC606XS
7es+xFCvS6qRr8AFmWrihUuE0KJ+fckI0+gUN8VeX6c5MLGYlfI=
-----END CERTIFICATE-----

This file is being read successfully by check_cert(), which demonstrates that PEM_read_X509() will skip text preceding a -----BEGIN CERTIFICATE----- header.

So, try running my code and see if it breaks anything else.

@lefcha
Owner
lefcha commented May 12, 2013

You have addressed all my concerns, and it all looks excellent, so I will merge your changes. This is clearly a very nice improvement in the certificates part of the code.

Thanks for all the great work you put on this (implementation, testing, etc.).

@lefcha
Owner
lefcha commented May 12, 2013

Closed by 7e8560f.

@lefcha lefcha closed this May 12, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment