Privoxy / Patches / #143 OpenSSL support

Fabian Keil - 2020-08-17

Thanks a lot for the patch.

Unfortunately it does not compile for me on a system based on FreeBSD 11.4-STABLE using OpenSSL 1.0.2t from base:

fk@t520 ~/git/privoxy.git $gmake
[...]
cc -c -pipe -fstack-protector-all -ggdb -Wshadow -Wconversion -I/usr/local/include/ -pthread -Wall ssl_common.c -o ssl_common.o
cc -c -pipe -fstack-protector-all -ggdb -Wshadow -Wconversion -I/usr/local/include/ -pthread -Wall openssl.c -o openssl.o
openssl.c:1656:7: warning: implicit declaration of function 'SSL_COMP_free_compression_methods' is invalid in C99 [-Wimplicit-function-declaration]
SSL_COMP_free_compression_methods();
^
openssl.c:1661:7: warning: implicit declaration of function 'COMP_zlib_cleanup' is invalid in C99 [-Wimplicit-function-declaration]
COMP_zlib_cleanup();
^
2 warnings generated.
cc -L/usr/local/lib -pthread -o privoxy actions.o cgi.o cgiedit.o cgisimple.o deanimate.o encode.o errlog.o filters.o gateway.o jbsockets.o jcc.o list.o loadcfg.o loaders.o miscutil.o parsers.o ssplit.o urlmatch.o client-tags.o pcrs.o ssl_common.o openssl.o -lbrotlidec -lcrypto -lz -lpcre -lpcreposix -lssl -lcrypto -lbrotlidec
openssl.o: In function ssl_release': /home/fk/git/privoxy.git/openssl.c:1656: undefined reference toSSL_COMP_free_compression_methods'
cc: error: linker command failed with exit code 1 (use -v to see invocation)
gmake: *** [GNUmakefile:778: privoxy] Error 1

The complaint about COMP_zlib_cleanup() can be silenced by including openssl/comp.h.

SSL_COMP_free_compression_methods() seems to be defined in openssl/ssl.h but including that header doesn't help.

Any ideas?

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Fabian Keil - 2020-08-17

My mistake. My build environment included libressl which was picked up instead of the base openssl.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Fabian Keil - 2020-08-21

status: open --> pending

assigned_to: Fabian Keil

Group: -->
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Fabian Keil - 2020-08-21

After removing libressl from the build environment and with minor changes this built on a system based on FreeBSD 11.4-STABLE.

It seems that invalid certificates aren't handled properly as can be tested at:
https://badssl.com/

1) If I open https://expired.badssl.com/ with default settings I get the "Connection failure" page and the log shows:
Error: BIO_do_handshake failed: error:14090086:SSL routines:ssl3_get_server_certificate:certificate verify failed

The expected behavior is that ssl_send_certificate_error() is used to explain the certificate error.

2) If I use the action section:

{+ignore-certificate-errors}
expired.badssl.com/

opening https://expired.badssl.com/ results in ssl_send_certificate_error() getting called but the error reason is "Reason: error:FFFFFFFF:lib(255):func(4095):reason(4095)" which obviously isn't very helpful.

The expected behaviour is that the request is let through despite the certificate error.

Can you please propose fixes?

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Fabian Keil - 2020-08-21

Another issue is that generated certificates aren't renewed when they are expired.

The following seems to fix this:

diff --git a/openssl.c b/openssl.c
index 1904317c..447cb44c 100644
--- a/openssl.c
+++ b/openssl.c
@@ -1179,7 +1179,7 @@ static int ssl_certificate_is_invalid(const char *cert_file)
return 1;
}

ret = X509_cmp_current_time(X509_get_notBefore(cert));

ret = X509_cmp_current_time(X509_get_notAfter(cert));
if (ret == 0)
{
log_ssl_errors(LOG_LEVEL_ERROR,
@@ -1188,7 +1188,7 @@ static int ssl_certificate_is_invalid(const char *cert_file)

X509_free(cert);

return ret == -1 ? 0 : 1;

return ret == -1 ? 1 : 0;
}

Do you agree?
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Fabian Keil - 2020-08-21

Also the code allows the use of SSLv3 which has the POODLE vulnerability.

It should probably be disabled with something like:

--- a/openssl.c
+++ b/openssl.c
@@ -649,6 +649,8 @@ extern int create_server_ssl_connection(struct client_state *csp)
goto exit;
}

SSL_CTX_set_options(ssl_attrs->ctx, SSL_OP_NO_SSLv3);
+
/*

Loading file with trusted CAs
*/
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Fabian Keil - 2020-08-25

A few more comments after more testing and reviewing:

1) It looks like SSLv3 should also be disabled in create_client_ssl_connection().

2) In openssl.c's generate_webpage_certificate() an error message references the wrong key:

--- a/openssl.c
+++ b/openssl.c
@@ -1548,7 +1548,7 @@ static int generate_webpage_certificate(struct client_state *csp)
if (!(pk_bio = BIO_new_file(cert_opt.issuer_key, "r")))
{
log_ssl_errors(LOG_LEVEL_ERROR, "Failure opening issuer key %s BIO",
- cert_opt.subject_key);
+ cert_opt.issuer_key);
ret = -1;
goto exit;
}

3) The Parameters comment for both ssl_crt_verify_info() implementations references buf multiple times.

4) In both ssl_release() descriptions "resources" is misspelled.

5) The openssl version of ssl_crt_verify_info() should probably be something like:
--- a/openssl.c
+++ b/openssl.c
@@ -1775,7 +1775,7 @@ exit:
**********/
extern void ssl_crt_verify_info(char buf, size_t size, uint32_t verification_result)
{
- ERR_error_string_n(verification_result, buf, size);
+ strlcpy(buf, X509_verify_cert_error_string(verification_result), size);
}

6) The OpenSSL version of create_server_ssl_connection() currently doesn't properly set csp->server_cert_verification_result which explains why the invalid certifcates aren't handled properly as mentioned in the previous comment.

7) The SSL_get_peer_certificate() call in SSL_get_peer_certificate() seems to always fail. Probably the certificates has to be gathered from a callback set with SSL_CTX_set_verify() similar to the way it's done when using mbedTLS.

8) It seems that the OpenSSL version currently does not validate that the certificate matches the host. Example: https://wrong.host.badssl.com/

The comment "Set the hostname to check against the received server certificate" above SSL_set_tlsext_host_name() appears to be incorrect and the function merely sets the SNI extension but does not affect the certificate validation.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Maxim Khon - 2020-08-26

@fabiankeil please check new commit in the PR which should fix all the issues found during the review. Thanks!

Last edit: Maxim Khon 2020-08-26

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Fabian Keil - 2020-08-27

Thanks a lot for the update.

I spotted a few more issues but they are all minor and I already have fixes for them in my local repository:

1) ssl_send_data() can loop endlessly if BIO_write() consistently returns 0.
I've fixed this with:
--- a/openssl.c
+++ b/openssl.c
@@ -171,7 +171,7 @@ extern int ssl_send_data(struct ssl_attr ssl_attr, const unsigned char buf, si
/
while ((ret = BIO_write(bio,
(const unsigned char )(buf + pos),
- send_len)) < 0)
+ send_len)) <= 0)
{
if (!BIO_should_retry(bio))

2) ssl_store_cert() does not check html_encode()'s return code for NULL.
This is probalby a copy and paste issue as ssl_verify_callback() lacks the check as well.
I fixed both.

3) generate_key() does not check the return code of BN_set_word()

4) Several error messages in generate_webpage_certificate() are incorrect.
This looks like a copy and paste issue as well.

So far the code seems to be working great and I could reproduce the "2x improvement".

If no new issues are discovered I intend to squash and push your commits at the end of the month.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Fabian Keil - 2020-09-01

status: pending --> closed-accepted
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Fabian Keil - 2020-09-01

Squashed and pushed with a couple of follow-up fixes.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

OpenSSL support

HTTP proxy to block ads and customize webpages

Group

Searches

Help

#143 OpenSSL support

Discussion