-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Description
Greetings! I've been investigating lua-nginx-module with Svace static analyzer and it found a curious method to look at.
lua-nginx-module/src/ngx_http_lua_socket_tcp.c
Lines 1629 to 1881 in 004922e
ngx_http_lua_ffi_socket_tcp_sslhandshake(ngx_http_request_t *r, | |
ngx_http_lua_socket_tcp_upstream_t *u, ngx_ssl_session_t *sess, | |
int enable_session_reuse, ngx_str_t *server_name, int verify, | |
int ocsp_status_req, STACK_OF(X509) *chain, EVP_PKEY *pkey, | |
const char **errmsg) | |
{ | |
ngx_int_t rc, i; | |
ngx_connection_t *c; | |
ngx_http_lua_ctx_t *ctx; | |
ngx_http_lua_co_ctx_t *coctx; | |
const char *busy_msg; | |
ngx_ssl_conn_t *ssl_conn; | |
X509 *x509; | |
ngx_log_debug0(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, | |
"lua tcp socket ssl handshake"); | |
if (u == NULL | |
|| u->peer.connection == NULL | |
|| u->read_closed | |
|| u->write_closed) | |
{ | |
*errmsg = "closed"; | |
return NGX_ERROR; | |
} | |
if (u->request != r) { | |
*errmsg = "bad request"; | |
return NGX_ERROR; | |
} | |
busy_msg = ngx_http_lua_socket_tcp_check_busy(r, u, SOCKET_OP_CONNECT | |
| SOCKET_OP_READ | |
| SOCKET_OP_WRITE); | |
if (busy_msg != NULL) { | |
*errmsg = busy_msg; | |
return NGX_ERROR; | |
} | |
if (u->raw_downstream || u->body_downstream) { | |
*errmsg = "not supported for downstream sockets"; | |
return NGX_ERROR; | |
} | |
c = u->peer.connection; | |
u->ssl_session_reuse = 1; | |
if (c->ssl && c->ssl->handshaked) { | |
if (sess != NULL) { | |
return NGX_DONE; | |
} | |
u->ssl_session_reuse = enable_session_reuse; | |
(void) ngx_http_lua_ssl_handshake_retval_handler(r, u, NULL); | |
return NGX_OK; | |
} | |
if (ngx_ssl_create_connection(u->conf->ssl, c, | |
NGX_SSL_BUFFER|NGX_SSL_CLIENT) | |
!= NGX_OK) | |
{ | |
*errmsg = "failed to create ssl connection"; | |
return NGX_ERROR; | |
} | |
ssl_conn = c->ssl->connection; | |
ctx = ngx_http_get_module_ctx(r, ngx_http_lua_module); | |
if (ctx == NULL) { | |
return NGX_HTTP_LUA_FFI_NO_REQ_CTX; | |
} | |
coctx = ctx->cur_co_ctx; | |
c->sendfile = 0; | |
if (sess != NULL) { | |
if (ngx_ssl_set_session(c, sess) != NGX_OK) { | |
*errmsg = "ssl set session failed"; | |
return NGX_ERROR; | |
} | |
ngx_log_debug1(NGX_LOG_DEBUG_HTTP, c->log, 0, | |
"lua ssl set session: %p", sess); | |
} else { | |
u->ssl_session_reuse = enable_session_reuse; | |
} | |
if (chain != NULL) { | |
ngx_http_lua_assert(pkey != NULL); /* ensured by resty.core */ | |
if (sk_X509_num(chain) < 1) { | |
ERR_clear_error(); | |
*errmsg = "invalid client certificate chain"; | |
return NGX_ERROR; | |
} | |
x509 = sk_X509_value(chain, 0); | |
if (x509 == NULL) { | |
ERR_clear_error(); | |
*errmsg = "ssl fetch client certificate from chain failed"; | |
return NGX_ERROR; | |
} | |
if (SSL_use_certificate(ssl_conn, x509) == 0) { | |
ERR_clear_error(); | |
*errmsg = "ssl set client certificate failed"; | |
return NGX_ERROR; | |
} | |
/* read rest of the chain */ | |
for (i = 1; i < (ngx_int_t) sk_X509_num(chain); i++) { | |
x509 = sk_X509_value(chain, i); | |
if (x509 == NULL) { | |
ERR_clear_error(); | |
*errmsg = "ssl fetch client intermediate certificate from " | |
"chain failed"; | |
return NGX_ERROR; | |
} | |
if (SSL_add1_chain_cert(ssl_conn, x509) == 0) { | |
ERR_clear_error(); | |
*errmsg = "ssl set client intermediate certificate failed"; | |
return NGX_ERROR; | |
} | |
} | |
if (SSL_use_PrivateKey(ssl_conn, pkey) == 0) { | |
ERR_clear_error(); | |
*errmsg = "ssl set client private key failed"; | |
return NGX_ERROR; | |
} | |
} | |
if (server_name != NULL && server_name->data != NULL) { | |
ngx_log_debug1(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, | |
"lua ssl server name: \"%V\"", server_name); | |
#ifdef SSL_CTRL_SET_TLSEXT_HOSTNAME | |
if (SSL_set_tlsext_host_name(c->ssl->connection, | |
(char *) server_name->data) | |
== 0) | |
{ | |
*errmsg = "SSL_set_tlsext_host_name failed"; | |
return NGX_ERROR; | |
} | |
#else | |
*errmsg = "no TLS extension support"; | |
return NGX_ERROR; | |
#endif | |
} | |
u->ssl_verify = verify; | |
if (ocsp_status_req) { | |
#ifdef NGX_HTTP_LUA_USE_OCSP | |
SSL_set_tlsext_status_type(c->ssl->connection, | |
TLSEXT_STATUSTYPE_ocsp); | |
#else | |
*errmsg = "no OCSP support"; | |
return NGX_ERROR; | |
#endif | |
} | |
if (server_name == NULL || server_name->len == 0) { | |
u->ssl_name.len = 0; | |
} else { | |
if (u->ssl_name.data) { | |
/* buffer already allocated */ | |
if (u->ssl_name.len >= server_name->len) { | |
/* reuse it */ | |
ngx_memcpy(u->ssl_name.data, server_name->data, | |
server_name->len); | |
u->ssl_name.len = server_name->len; | |
} else { | |
ngx_free(u->ssl_name.data); | |
goto new_ssl_name; | |
} | |
} else { | |
new_ssl_name: | |
u->ssl_name.data = ngx_alloc(server_name->len, ngx_cycle->log); | |
if (u->ssl_name.data == NULL) { | |
u->ssl_name.len = 0; | |
*errmsg = "no memory"; | |
return NGX_ERROR; | |
} | |
ngx_memcpy(u->ssl_name.data, server_name->data, server_name->len); | |
u->ssl_name.len = server_name->len; | |
} | |
} | |
u->write_co_ctx = coctx; | |
#if 0 | |
#ifdef NGX_HTTP_LUA_USE_OCSP | |
SSL_set_tlsext_status_type(c->ssl->connection, TLSEXT_STATUSTYPE_ocsp); | |
#endif | |
#endif | |
rc = ngx_ssl_handshake(c); | |
ngx_log_debug1(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, | |
"ngx_ssl_handshake returned: %d", rc); | |
if (rc == NGX_AGAIN) { | |
if (c->write->timer_set) { | |
ngx_del_timer(c->write); | |
} | |
ngx_add_timer(c->read, u->connect_timeout); | |
u->conn_waiting = 1; | |
u->write_prepare_retvals = ngx_http_lua_ssl_handshake_retval_handler; | |
ngx_http_lua_cleanup_pending_operation(coctx); | |
coctx->cleanup = ngx_http_lua_coctx_cleanup; | |
coctx->data = u; | |
c->ssl->handler = ngx_http_lua_ssl_handshake_handler; | |
if (ctx->entered_content_phase) { | |
r->write_event_handler = ngx_http_lua_content_wev_handler; | |
} else { | |
r->write_event_handler = ngx_http_core_run_phases; | |
} | |
return NGX_AGAIN; | |
} | |
ngx_http_lua_ssl_handshake_handler(c); | |
if (rc == NGX_ERROR) { | |
*errmsg = u->error_ret; | |
return NGX_ERROR; | |
} | |
return NGX_OK; | |
} |
Here the return value of method incovation SSL_set_tlsext_status_type()
(which calls SSL_ctrl()
under the hood) is not checked at the following cases:
lua-nginx-module/src/ngx_http_lua_socket_tcp.c
Lines 1791 to 1792 in 004922e
SSL_set_tlsext_status_type(c->ssl->connection, | |
TLSEXT_STATUSTYPE_ocsp); |
and
lua-nginx-module/src/ngx_http_lua_socket_tcp.c
Line 1838 in 004922e
SSL_set_tlsext_status_type(c->ssl->connection, TLSEXT_STATUSTYPE_ocsp); |
but usually it is checked for the function SSL_ctrl()
The Question:
After a long research and official OpenSSL docs read I'm still not sure if it's correct not to check the returning value in the cases above.
What do you think about this?
Found by Linux Verification Center (linuxtesting.org) with SVACE.