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

crash in x509 #820

Closed
ailin-nemui opened this Issue Jan 19, 2018 · 13 comments

Comments

Projects
None yet
5 participants
@ailin-nemui
Contributor

ailin-nemui commented Jan 19, 2018

#0 0x001e8d60 in ?? () 
#1 0x76b2395c in X509_LOOKUP_free () from /usr/lib/arm-linux-gnueabihf/libcrypto.so.1.0.0 
#2 0x76b23bc0 in X509_STORE_free () from /usr/lib/arm-linux-gnueabihf/libcrypto.so.1.0.0 
Backtrace stopped: previous frame identical to this frame (corrupt stack?)

reported by blap

@Zannick

This comment has been minimized.

Zannick commented Jan 20, 2018

I'm reliably reproducing this crash with /reconnect (or /disconnect followed by /connect) using SSL to connect to freenode. (Except when I tried it before joining any channels.)

Example stacktrace built from head (with ./configure --prefix=/usr):

#0  0xb7ab71b8 in sk_num () from /usr/lib/i386-linux-gnu/i686/cmov/libcrypto.so.1.0.0
#1  0xb7b09824 in X509_VERIFY_PARAM_set1_policies ()
   from /usr/lib/i386-linux-gnu/i686/cmov/libcrypto.so.1.0.0
#2  0xb7b09949 in X509_VERIFY_PARAM_inherit ()
   from /usr/lib/i386-linux-gnu/i686/cmov/libcrypto.so.1.0.0
#3  0xb7b01082 in X509_STORE_CTX_init ()
   from /usr/lib/i386-linux-gnu/i686/cmov/libcrypto.so.1.0.0
#4  0xb7bf9727 in ?? () from /usr/lib/i386-linux-gnu/i686/cmov/libssl.so.1.0.0
#5  0xb7bce313 in ?? () from /usr/lib/i386-linux-gnu/i686/cmov/libssl.so.1.0.0
#6  0xb7bd3f82 in ?? () from /usr/lib/i386-linux-gnu/i686/cmov/libssl.so.1.0.0
#7  0xb7bf89ea in SSL_connect () from /usr/lib/i386-linux-gnu/i686/cmov/libssl.so.1.0.0
#8  0x080db874 in irssi_ssl_handshake (handle=0x851bc30) at network-openssl.c:812
#9  0x080d0c8f in server_connect_callback_init_ssl (server=0x825bdb8, handle=0x851bc30)
    at servers.c:176
#10 0x080c91d1 in irssi_io_invoke (source=0x851bc30, condition=G_IO_IN, data=0x84b3af8)
    at misc.c:51
#11 0xb7ca7cee in ?? () from /lib/i386-linux-gnu/libglib-2.0.so.0
#12 0xb7c60cc3 in g_main_context_dispatch () from /lib/i386-linux-gnu/libglib-2.0.so.0
#13 0xb7c610d9 in ?? () from /lib/i386-linux-gnu/libglib-2.0.so.0
#14 0xb7c611a6 in g_main_context_iteration () from /lib/i386-linux-gnu/libglib-2.0.so.0
#15 0x0805c073 in main (argc=1, argv=0xbffff724) at irssi.c:331

Example stacktrace built from head (with ./configure --prefix=/usr CFLAGS="-g -Og"):

#0  0xb7b0492e in ?? () from /usr/lib/i386-linux-gnu/i686/cmov/libcrypto.so.1.0.0
#1  0xb7ab718f in sk_pop_free () from /usr/lib/i386-linux-gnu/i686/cmov/libcrypto.so.1.0.0
#2  0xb7b04f2c in X509_STORE_free ()
   from /usr/lib/i386-linux-gnu/i686/cmov/libcrypto.so.1.0.0
#3  0xb7bf54c7 in SSL_CTX_free () from /usr/lib/i386-linux-gnu/i686/cmov/libssl.so.1.0.0
#4  0x080d2aa5 in irssi_ssl_free (handle=0x84b8828) at network-openssl.c:82
#5  0xb7c5388e in g_io_channel_unref () from /lib/i386-linux-gnu/libglib-2.0.so.0
#6  0xb7ca7a3b in ?? () from /lib/i386-linux-gnu/libglib-2.0.so.0
#7  0xb7c5dc22 in ?? () from /lib/i386-linux-gnu/libglib-2.0.so.0
#8  0xb7c60e60 in g_main_context_dispatch () from /lib/i386-linux-gnu/libglib-2.0.so.0
#9  0xb7c610d9 in ?? () from /lib/i386-linux-gnu/libglib-2.0.so.0
#10 0xb7c611a6 in g_main_context_iteration () from /lib/i386-linux-gnu/libglib-2.0.so.0
#11 0x0806fc85 in main (argc=1, argv=0xbffff724) at irssi.c:331

I have a few other stacktraces from the former build that are basically the same (network-openssl.c:82 or :812), or empty (0x084f88f8 in ?? ()).

@Zannick

This comment has been minimized.

Zannick commented Jan 20, 2018

Reproduced in valgrind:
log.txt

@dequis

This comment has been minimized.

Member

dequis commented Jan 20, 2018

Thank you!

Copying first block from there for convenience:

==25325== Invalid read of size 4
==25325==    at 0x4498F94: CRYPTO_add_lock (in /usr/lib/i386-linux-gnu/i686/cmov/libcrypto.so.1.0.0)
==25325==    by 0x80D2D6B: X509_STORE_up_ref (network-openssl.c:54)
==25325==    by 0x80D3B2D: irssi_ssl_get_iochannel (network-openssl.c:529)
==25325==    by 0x80D3C73: net_connect_ip_ssl (network-openssl.c:772)
==25325==    by 0x80C98D0: server_real_connect (servers.c:223)
==25325==    by 0x80C9C98: server_connect_callback_readpipe (servers.c:311)
==25325==    by 0x80C22CE: irssi_io_invoke (misc.c:51)
==25325==    by 0x435BCED: ??? (in /lib/i386-linux-gnu/libglib-2.0.so.0.4200.1)
==25325==    by 0x4314CC2: g_main_context_dispatch (in /lib/i386-linux-gnu/libglib-2.0.so.0.4200.1)
==25325==    by 0x43150D8: ??? (in /lib/i386-linux-gnu/libglib-2.0.so.0.4200.1)
==25325==    by 0x43151A5: g_main_context_iteration (in /lib/i386-linux-gnu/libglib-2.0.so.0.4200.1)
==25325==    by 0x806FC84: main (irssi.c:331)
==25325==  Address 0x63adb6c is 68 bytes inside a block of size 72 free'd
==25325==    at 0x402B3A8: free (vg_replace_malloc.c:473)
==25325==    by 0x4499B07: CRYPTO_free (in /usr/lib/i386-linux-gnu/i686/cmov/libcrypto.so.1.0.0)
==25325==    by 0x456BF69: X509_STORE_free (in /usr/lib/i386-linux-gnu/i686/cmov/libcrypto.so.1.0.0)
==25325==    by 0x44304C6: SSL_CTX_free (in /usr/lib/i386-linux-gnu/i686/cmov/libssl.so.1.0.0)
==25325==    by 0x80D2AA4: irssi_ssl_free (network-openssl.c:82)
==25325==    by 0x430788D: g_io_channel_unref (in /lib/i386-linux-gnu/libglib-2.0.so.0.4200.1)
==25325==    by 0x435BA3A: ??? (in /lib/i386-linux-gnu/libglib-2.0.so.0.4200.1)
==25325==    by 0x4311C21: ??? (in /lib/i386-linux-gnu/libglib-2.0.so.0.4200.1)
==25325==    by 0x4314E5F: g_main_context_dispatch (in /lib/i386-linux-gnu/libglib-2.0.so.0.4200.1)
==25325==    by 0x43150D8: ??? (in /lib/i386-linux-gnu/libglib-2.0.so.0.4200.1)
==25325==    by 0x43151A5: g_main_context_iteration (in /lib/i386-linux-gnu/libglib-2.0.so.0.4200.1)
==25325==    by 0x806FC84: main (irssi.c:331)

This is #751, specifically 36d8b97 which tries to implement X509_STORE_up_ref for openssl older than 1.1.

So this needs 1.0 to reproduce it.

@dequis

This comment has been minimized.

Member

dequis commented Jan 21, 2018

I can reproduce it with:

docker run -it ubuntu:14.04
sudo sh -c "echo 'deb http://download.opensuse.org/repositories/home:/ailin_nemui:/irssi-git/xUbuntu_14.04/ /' > /etc/apt/sources.list.d/irssi-git.list"
sudo apt-get update
sudo apt-get install irssi-git
irssi
/connect -ssl Freenode
/reconnect
@klopsi

This comment has been minimized.

klopsi commented Jan 22, 2018

a different segfault this time
raspberry pi 3, raspbian, openssl_1.0.1t-1+deb8u7_armhf.deb built from source
jan 19 git clone, ./configure CFLAGS=-g - O0, running in gdb

Program received signal SIGSEGV, Segmentation fault.
X509_LOOKUP_by_subject (ctx=0x0, type=type@entry=1, name=name@entry=0x262f28, 
    ret=ret@entry=0x7effe8dc) at x509_lu.c:128
128         if ((ctx->method == NULL) || (ctx->method->get_by_subject == NULL))

(gdb) bt
#0  X509_LOOKUP_by_subject (ctx=0x0, type=type@entry=1, name=name@entry=0x262f28, 
    ret=ret@entry=0x7effe8dc) at x509_lu.c:128
#1  0x76b21e38 in X509_STORE_get_by_subject (vs=vs@entry=0x7effe9ac, type=type@entry=1, 
    name=name@entry=0x262f28, ret=0x76b21e38 <X509_STORE_get_by_subject+184>, 
    ret@entry=0x7effe91c) at x509_lu.c:300
#2  0x76b22680 in X509_STORE_CTX_get1_issuer (issuer=0x7effe970, ctx=0x7effe9ac, 
    x=0x3cab50) at x509_lu.c:604
#3  0x76b1e378 in X509_verify_cert (Cannot access memory at address 0xaba
ctx=0x7effe9ac) at x509_vfy.c:296
#4  0x76bdc2b4 in ?? () from /usr/lib/arm-linux-gnueabihf/libssl.so.1.0.0
Cannot access memory at address 0xaba
@LemonBoy

This comment has been minimized.

Member

LemonBoy commented Jan 24, 2018

Oh, I think we're hitting this bug. Fuck this shit, amirite?

Here's a ugly solution/workaround, it does seem to fix the crash at least in the test case @dequis reported.

diff --git a/src/core/network-openssl.c b/src/core/network-openssl.c
index c7ce4b4..5f59483 100644
--- a/src/core/network-openssl.c
+++ b/src/core/network-openssl.c
@@ -79,6 +79,7 @@ static void irssi_ssl_free(GIOChannel *handle)
        GIOSSLChannel *chan = (GIOSSLChannel *)handle;
        g_io_channel_unref(chan->giochan);
        SSL_free(chan->ssl);
+       chan->ctx->cert_store = NULL;
        SSL_CTX_free(chan->ctx);
        g_free(chan);
 }

If this works I'll make a proper PR.

@ailin-nemui

This comment has been minimized.

Contributor

ailin-nemui commented Jan 24, 2018

yea. no. what I would propose is raise the minimum openssl version required to 1.0.2.

but. good work in finding at least dequis' bug

@dequis

This comment has been minimized.

Member

dequis commented Jan 24, 2018

Why do we need this change in the first place? #750 looks like it was something caused in the 1.1 branch, possibly due to capsicum, can't we do something else about that instead? 96f4fe1 i guess. Capsicum support isn't worth dropping support for older distros.

@ailin-nemui

This comment has been minimized.

Contributor

ailin-nemui commented Jan 24, 2018

yea, you also have a valid point...

@ailin-nemui

This comment has been minimized.

Contributor

ailin-nemui commented Jan 24, 2018

let's try this:

  • #error if openssl is too old and capsicum is on,
  • #ifdef the code so that it matches the old version when capsicum is not compiled and the current (openssl >= 1.0.2 or crash) version when capsicum is used.
  • maybe add a check to configure.ac (see patch below which would fix whole project to >=1.0.2).

the bad thing is that this will expose the (generic style, but written with capsicum in mind) code paths to less testing. @trasz ?

diff --git a/configure.ac b/configure.ac
index dcae6863..47639f6f 100644
--- a/configure.ac
+++ b/configure.ac
@@ -300,12 +300,23 @@ GLIB_TESTS
 dnl **
 dnl ** OpenSSL checks
 dnl **
-PKG_CHECK_MODULES([OPENSSL], [openssl], [
+PKG_CHECK_MODULES([OPENSSL], [openssl >= 1.0.2], [
 	CFLAGS="$CFLAGS $OPENSSL_CFLAGS"
 	LIBS="$LIBS $OPENSSL_LIBS"
 ], [
 	AC_CHECK_LIB([ssl], [SSL_library_init], [
-		LIBS="$LIBS -lssl -lcrypto"
+		AC_MSG_CHECKING([for OpenSSL version >= 1.0.2])
+		AC_EGREP_CPP([^good$], [
+#include <openssl/crypto.h>
+#if (OPENSSL_VERSION_NUMBER >= 0x10002000L)
+good
+#endif
+		], [
+			AC_MSG_RESULT([ok])
+			LIBS="$LIBS -lssl -lcrypto"
+		], [
+			AC_MSG_ERROR([OpenSSL version too old])
+		])
 	], [
 		AC_MSG_ERROR([The OpenSSL library was not found])
 	])
@ailin-nemui

This comment has been minimized.

Contributor

ailin-nemui commented Jan 24, 2018

(I'm not in favour of trying to work around openssl bugs like wahern/luaossl@8dbe7e4 ...

@ailin-nemui

This comment has been minimized.

Contributor

ailin-nemui commented Jan 25, 2018

I prepared #831, can you verify this @Zannick ?

@Zannick

This comment has been minimized.

Zannick commented Jan 25, 2018

I applied #831 and /reconnect did not crash (in 1 of 1 tests). Can test more later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment