Skip to content
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

Uncheked result of method SSL_set_tlsext_status_type() #2395

Open
Anchels opened this issue Feb 13, 2025 · 0 comments
Open

Uncheked result of method SSL_set_tlsext_status_type() #2395

Anchels opened this issue Feb 13, 2025 · 0 comments

Comments

@Anchels
Copy link

Anchels commented Feb 13, 2025

Greetings! I've been investigating lua-nginx-module with Svace static analyzer and it found a curious method to look at.

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:

SSL_set_tlsext_status_type(c->ssl->connection,
TLSEXT_STATUSTYPE_ocsp);

and

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant