From 1a75704ff07e4ccd426c6c63841bb846a3e273eb Mon Sep 17 00:00:00 2001 From: Thijs Alkemade Date: Fri, 6 Sep 2013 13:52:34 +0200 Subject: [PATCH 1/8] Report the actual TLS version used, not the version the cipher belongs to. --- src/ssl.c | 3 ++- src/ssl.lua | 7 +++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/ssl.c b/src/ssl.c index dd9f6c7..4809add 100644 --- a/src/ssl.c +++ b/src/ssl.c @@ -645,7 +645,8 @@ static int meth_info(lua_State *L) lua_pushstring(L, buf); lua_pushnumber(L, bits); lua_pushnumber(L, algbits); - return 3; + lua_pushstring(L, SSL_get_version(ssl->ssl)); + return 4; } static int meth_copyright(lua_State *L) diff --git a/src/ssl.lua b/src/ssl.lua index 1b062f6..a2e8ad6 100644 --- a/src/ssl.lua +++ b/src/ssl.lua @@ -128,7 +128,7 @@ end -- Extract connection information. -- local function info(ssl, field) - local str, comp, err + local str, comp, err, protocol comp, err = core.compression(ssl) if err then return comp, err @@ -138,7 +138,7 @@ local function info(ssl, field) return comp end local info = {compression = comp} - str, info.bits, info.algbits = core.info(ssl) + str, info.bits, info.algbits, protocol = core.info(ssl) if str then info.cipher, info.protocol, info.key, info.authentication, info.encryption, info.mac = @@ -146,6 +146,9 @@ local function info(ssl, field) "^(%S+)%s+(%S+)%s+Kx=(%S+)%s+Au=(%S+)%s+Enc=(%S+)%s+Mac=(%S+)") info.export = (string.match(str, "%sexport%s*$") ~= nil) end + if protocol then + info.protocol = protocol + end if field then return info[field] end From a344f58b207c696a11d464624631f728497c5147 Mon Sep 17 00:00:00 2001 From: Paul Aurich Date: Sat, 7 Sep 2013 13:31:41 -0700 Subject: [PATCH 2/8] context: Wrap find_ec_key in #ifndef OPENSSL_NO_ECDH "#ifndef OPENSSL_NO_ECDH" is a ridiculous conditional, by the way. --- src/context.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/context.c b/src/context.c index 80b90ee..41bfa15 100644 --- a/src/context.c +++ b/src/context.c @@ -252,6 +252,7 @@ static int verify_cb(int preverify_ok, X509_STORE_CTX *x509_ctx) return (verify & LSEC_VERIFY_CONTINUE ? 1 : preverify_ok); } +#ifndef OPENSSL_NO_ECDH static EC_KEY *find_ec_key(const char *str) { p_ec ptr; @@ -261,6 +262,7 @@ static EC_KEY *find_ec_key(const char *str) } return NULL; } +#endif /*------------------------------ Lua Functions -------------------------------*/ From 3fb33cdc4e97b597930a3c49ffa659767f6b1d2b Mon Sep 17 00:00:00 2001 From: Paul Aurich Date: Sat, 7 Sep 2013 14:44:23 -0700 Subject: [PATCH 3/8] context: Don't leak EC_KEY in set_curve() SSL_CTX_set_tmp_ecdh() takes a reference to the provided key. ==8323== 1,044 (56 direct, 988 indirect) bytes in 1 blocks are definitely lost in loss record 611 of 631 ==8323== at 0x4C2935B: malloc (vg_replace_malloc.c:270) ==8323== by 0x5E05D9F: CRYPTO_malloc (mem.c:308) ==8323== by 0x5E59859: EC_KEY_new (ec_key.c:75) ==8323== by 0x5E59974: EC_KEY_new_by_curve_name (ec_key.c:96) ==8323== by 0x4E395A7: set_curve (context.c:261) ... --- src/context.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/context.c b/src/context.c index 41bfa15..a3f9cde 100644 --- a/src/context.c +++ b/src/context.c @@ -507,15 +507,22 @@ static int set_curve(lua_State *L) #else static int set_curve(lua_State *L) { + long ret; SSL_CTX *ctx = lsec_checkcontext(L, 1); const char *str = luaL_checkstring(L, 2); EC_KEY *key = find_ec_key(str); + if (!key) { lua_pushboolean(L, 0); lua_pushstring(L, "elliptic curve not supported"); return 2; } - if (!SSL_CTX_set_tmp_ecdh(ctx, key)) { + + ret = SSL_CTX_set_tmp_ecdh(ctx, key); + /* SSL_CTX_set_tmp_ecdh takes its own reference */ + EC_KEY_free(key); + + if (!ret) { lua_pushboolean(L, 0); lua_pushstring(L, "error setting elliptic curve"); return 2; From 9262f9e7deec54e0e802e5d6313d3c222f13249a Mon Sep 17 00:00:00 2001 From: Paul Aurich Date: Sat, 7 Sep 2013 14:51:30 -0700 Subject: [PATCH 4/8] ssl.lua: Comment subtle DH/ECDH ordering caveat --- src/ssl.lua | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/ssl.lua b/src/ssl.lua index 1b062f6..6a4b240 100644 --- a/src/ssl.lua +++ b/src/ssl.lua @@ -82,6 +82,11 @@ function newcontext(cfg) succ, msg = context.setdepth(ctx, cfg.depth) if not succ then return nil, msg end end + + -- NOTE: Setting DH parameters and elliptic curves needs to come after + -- setoptions(), in case the user has specified the single_{dh,ecdh}_use + -- options. + -- Set DH parameters if cfg.dhparam then if type(cfg.dhparam) ~= "function" then From 9c7c96f2a04aadfc42a0ee08521526993bde0552 Mon Sep 17 00:00:00 2001 From: Paul Aurich Date: Sat, 7 Sep 2013 15:56:55 -0700 Subject: [PATCH 5/8] Add useful context to various error messages --- src/context.c | 24 ++++++++++++++---------- src/ssl.c | 3 ++- src/x509.c | 5 +++-- 3 files changed, 19 insertions(+), 13 deletions(-) diff --git a/src/context.c b/src/context.c index a3f9cde..5b652fe 100644 --- a/src/context.c +++ b/src/context.c @@ -272,12 +272,14 @@ static EC_KEY *find_ec_key(const char *str) static int create(lua_State *L) { p_context ctx; + const char *str_method; LSEC_SSL_METHOD *method; - method = str2method(luaL_checkstring(L, 1)); + str_method = luaL_checkstring(L, 1); + method = str2method(str_method); if (!method) { lua_pushnil(L); - lua_pushstring(L, "invalid protocol"); + lua_pushfstring(L, "invalid protocol (%s)", str_method); return 2; } ctx = (p_context) lua_newuserdata(L, sizeof(t_context)); @@ -285,11 +287,12 @@ static int create(lua_State *L) lua_pushnil(L); lua_pushstring(L, "error creating context"); return 2; - } + } ctx->context = SSL_CTX_new(method); if (!ctx->context) { lua_pushnil(L); - lua_pushstring(L, "error creating context"); + lua_pushfstring(L, "error creating context (%s)", + ERR_reason_error_string(ERR_get_error())); return 2; } ctx->mode = LSEC_MODE_INVALID; @@ -414,7 +417,7 @@ static int set_verify(lua_State *L) str = luaL_checkstring(L, i); if (!set_verify_flag(str, &flag)) { lua_pushboolean(L, 0); - lua_pushstring(L, "invalid verify option"); + lua_pushfstring(L, "invalid verify option (%s)", str); return 2; } } @@ -445,7 +448,7 @@ static int set_options(lua_State *L) #endif if (!set_option_flag(str, &flag)) { lua_pushboolean(L, 0); - lua_pushstring(L, "invalid option"); + lua_pushfstring(L, "invalid option (%s)", str); return 2; } } @@ -473,7 +476,7 @@ static int set_mode(lua_State *L) return 1; } lua_pushboolean(L, 0); - lua_pushstring(L, "invalid mode"); + lua_pushfstring(L, "invalid mode (%s)", str); return 1; } @@ -514,7 +517,7 @@ static int set_curve(lua_State *L) if (!key) { lua_pushboolean(L, 0); - lua_pushstring(L, "elliptic curve not supported"); + lua_pushfstring(L, "elliptic curve %s not supported", str); return 2; } @@ -524,7 +527,8 @@ static int set_curve(lua_State *L) if (!ret) { lua_pushboolean(L, 0); - lua_pushstring(L, "error setting elliptic curve"); + lua_pushfstring(L, "error setting elliptic curve (%s)", + ERR_reason_error_string(ERR_get_error())); return 2; } lua_pushboolean(L, 1); @@ -608,7 +612,7 @@ static int meth_set_verify_ext(lua_State *L) crl_flag |= X509_V_FLAG_CRL_CHECK_ALL; } else { lua_pushboolean(L, 0); - lua_pushstring(L, "invalid verify option"); + lua_pushfstring(L, "invalid verify option (%s)", str); return 2; } } diff --git a/src/ssl.c b/src/ssl.c index dd9f6c7..43cde99 100644 --- a/src/ssl.c +++ b/src/ssl.c @@ -245,7 +245,8 @@ static int meth_create(lua_State *L) ssl->ssl = SSL_new(ctx); if (!ssl->ssl) { lua_pushnil(L); - lua_pushstring(L, "error creating SSL object"); + lua_pushfstring(L, "error creating SSL object (%s)", + ERR_reason_error_string(ERR_get_error())); return 2; } ssl->state = LSEC_STATE_NEW; diff --git a/src/x509.c b/src/x509.c index 104cfd3..5f71cd0 100644 --- a/src/x509.c +++ b/src/x509.c @@ -306,12 +306,13 @@ static int meth_digest(lua_State* L) } if (!digest) { lua_pushnil(L); - lua_pushstring(L, "digest algorithm not supported"); + lua_pushfstring(L, "digest algorithm not supported (%s)", str); return 2; } if (!X509_digest(cert, digest, buffer, &bytes)) { lua_pushnil(L); - lua_pushstring(L, "error processing the certificate"); + lua_pushfstring(L, "error processing the certificate (%s)", + ERR_reason_error_string(ERR_get_error())); return 2; } to_hex((char*)buffer, bytes, hex_buffer); From 8cf7eb2d785be4381de82c66b4ebbce85ff58ffb Mon Sep 17 00:00:00 2001 From: Paul Aurich Date: Wed, 28 Aug 2013 21:01:55 -0700 Subject: [PATCH 6/8] context: for dhparam_cb, pass is_export as boolean The integer value that's actually returned for this flag is 2, which is fine for C (it is defined as true), but it's sufficiently surprising (because it's not 1), that this is worth fixing -- even if export ciphers aren't common. It should be a boolean anyway. --- src/context.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/context.c b/src/context.c index 5b652fe..9c5e29f 100644 --- a/src/context.c +++ b/src/context.c @@ -167,7 +167,7 @@ static DH *dhparam_cb(SSL *ssl, int is_export, int keylength) lua_gettable(L, -2); /* Invoke the callback */ - lua_pushnumber(L, is_export); + lua_pushboolean(L, is_export); lua_pushnumber(L, keylength); lua_call(L, 2, 1); From 0dab860770647be883af6c2c578b0d5d2861e9cb Mon Sep 17 00:00:00 2001 From: Paul Aurich Date: Mon, 9 Sep 2013 20:29:54 -0700 Subject: [PATCH 7/8] context: Link SSL_CTX to p_context (not lua_State) This is needed because the p_context is going to cache DH (and eventually EC_KEY) objects, to plug a leak in the dhparam callback. --- src/context.c | 16 +++++++++++----- src/context.h | 1 + 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/src/context.c b/src/context.c index 9c5e29f..f02608a 100644 --- a/src/context.c +++ b/src/context.c @@ -159,7 +159,9 @@ static DH *dhparam_cb(SSL *ssl, int is_export, int keylength) lua_State *L; DH *dh_tmp = NULL; SSL_CTX *ctx = SSL_get_SSL_CTX(ssl); - L = (lua_State*)SSL_CTX_get_app_data(ctx); + p_context pctx = (p_context)SSL_CTX_get_app_data(ctx); + + L = pctx->L; /* Get the callback */ luaL_getmetatable(L, "SSL:DH:Registry"); @@ -194,8 +196,9 @@ static int cert_verify_cb(X509_STORE_CTX *x509_ctx, void *ptr) int verify; lua_State *L; SSL_CTX *ctx = (SSL_CTX*)ptr; + p_context pctx = (p_context)SSL_CTX_get_app_data(ctx); - L = (lua_State*)SSL_CTX_get_app_data(ctx); + L = pctx->L; /* Get verify flags */ luaL_getmetatable(L, "SSL:Verify:Registry"); @@ -226,6 +229,7 @@ static int verify_cb(int preverify_ok, X509_STORE_CTX *x509_ctx) int verify; SSL *ssl; SSL_CTX *ctx; + p_context pctx; lua_State *L; /* Short-circuit optimization */ @@ -235,7 +239,8 @@ static int verify_cb(int preverify_ok, X509_STORE_CTX *x509_ctx) ssl = X509_STORE_CTX_get_ex_data(x509_ctx, SSL_get_ex_data_X509_STORE_CTX_idx()); ctx = SSL_get_SSL_CTX(ssl); - L = (lua_State*)SSL_CTX_get_app_data(ctx); + pctx = (p_context)SSL_CTX_get_app_data(ctx); + L = pctx->L; /* Get verify flags */ luaL_getmetatable(L, "SSL:Verify:Registry"); @@ -296,13 +301,14 @@ static int create(lua_State *L) return 2; } ctx->mode = LSEC_MODE_INVALID; + ctx->L = L; luaL_getmetatable(L, "SSL:Context"); lua_setmetatable(L, -2); /* No session support */ SSL_CTX_set_session_cache_mode(ctx->context, SSL_SESS_CACHE_OFF); - /* Link lua_State with the context */ - SSL_CTX_set_app_data(ctx->context, (void*)L); + /* Link LuaSec context with the OpenSSL context */ + SSL_CTX_set_app_data(ctx->context, ctx); return 1; } diff --git a/src/context.h b/src/context.h index 8521852..5f358e3 100644 --- a/src/context.h +++ b/src/context.h @@ -21,6 +21,7 @@ typedef struct t_context_ { SSL_CTX *context; + lua_State *L; int mode; } t_context; typedef t_context* p_context; From 1d920fc13c9a97ac885bfc7e236164cb6ad6c37a Mon Sep 17 00:00:00 2001 From: Paul Aurich Date: Mon, 9 Sep 2013 21:02:41 -0700 Subject: [PATCH 8/8] context: Don't leak DH* in dhparam_cb ==1429== 336 (144 direct, 192 indirect) bytes in 1 blocks are definitely lost in loss record 567 of 611 ... ==1429== by 0x5ECCBC7: PEM_ASN1_read_bio (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0) ==1429== by 0x4E39D8F: dhparam_cb (context.c:184) ==1429== by 0x5B679D3: ??? (in /lib/x86_64-linux-gnu/libssl.so.1.0.0) ==1429== by 0x5B6A6EE: ??? (in /lib/x86_64-linux-gnu/libssl.so.1.0.0) ==1429== by 0x4E3C00D: meth_handshake (ssl.c:103) ... --- src/context.c | 16 ++++++++++++++++ src/context.h | 1 + 2 files changed, 17 insertions(+) diff --git a/src/context.c b/src/context.c index f02608a..1afebaa 100644 --- a/src/context.c +++ b/src/context.c @@ -184,6 +184,16 @@ static DH *dhparam_cb(SSL *ssl, int is_export, int keylength) dh_tmp = PEM_read_bio_DHparams(bio, NULL, NULL, NULL); BIO_free(bio); } + + /* + * OpenSSL exepcts the callback to maintain a reference to the DH*. So, + * cache it here, and clean up the previous set of parameters. Any remaining + * set is cleaned up when destroying the LuaSec context. + */ + if (pctx->dh_param) + DH_free(pctx->dh_param); + pctx->dh_param = dh_tmp; + lua_pop(L, 2); /* Remove values from stack */ return dh_tmp; } @@ -293,6 +303,7 @@ static int create(lua_State *L) lua_pushstring(L, "error creating context"); return 2; } + memset(ctx, 0, sizeof(t_context)); ctx->context = SSL_CTX_new(method); if (!ctx->context) { lua_pushnil(L); @@ -582,6 +593,11 @@ static int meth_destroy(lua_State *L) SSL_CTX_free(ctx->context); ctx->context = NULL; } + if (ctx->dh_param) { + DH_free(ctx->dh_param); + ctx->dh_param = NULL; + } + return 0; } diff --git a/src/context.h b/src/context.h index 5f358e3..2ad322f 100644 --- a/src/context.h +++ b/src/context.h @@ -22,6 +22,7 @@ typedef struct t_context_ { SSL_CTX *context; lua_State *L; + DH *dh_param; int mode; } t_context; typedef t_context* p_context;