Fix potential NULL dereference in ngx_http_auth_spnego_store_delegated_creds()

[Fahnenfluchtige]

The Svace static analysis tool identified two potential issues in the function ngx_http_auth_spnego_store_delegated_creds(), where the return values of ngx_pcalloc() and ngx_pool_cleanup_add() were used without NULL-checks. These functions internally call ngx_palloc(), which may return NULL in case of memory allocation failure.

In current situation, If ngx_pcalloc() fails to allocate memory, ccname will be NULL (815-848):

ccname = (char *)ngx_pcalloc(r->pool, ccname_size);
...
var_value.len = ngx_strlen(ccname);

And similar case we can see in lines 852-853, ngx_pool_cleanup_add() internally calls ngx_palloc(), which can return NULL:

 ngx_pool_cleanup_t *cln = ngx_pool_cleanup_add(r->pool, 0);
 cln->handler = ngx_http_auth_spnego_krb5_destroy_ccache; 

ngx_palloc() is guaranteed not to return NULL only if there is enough space available in at least one of the existing memory pools in the linked list. However, this guarantee is not absolute. If there is no available space, memory allocation will proceed by calling ngx_palloc_block(), which then invokes ngx_memalign() or posix_memalign()/memalign() to allocate memory. These system calls can return NULL if allocation fails. Therefore, it is always recommended to check the return value of ngx_palloc() and similar functions before using the allocated memory.

So the solution is:

diff --git a/ngx_http_auth_spnego_module.c b/ngx_http_auth_spnego_module.c
index dfdead6..a60f652 100644
--- a/ngx_http_auth_spnego_module.c
+++ b/ngx_http_auth_spnego_module.c
@@ -813,6 +813,9 @@ ngx_http_auth_spnego_store_delegated_creds(ngx_http_request_t *r,
                           ngx_strlen("/") + ngx_strlen(escaped)) +
                          1;
     ccname = (char *)ngx_pcalloc(r->pool, ccname_size);
+    if (ccname == NULL) {
+        return NGX_ERROR;
+    }
 
     ngx_snprintf((u_char *)ccname, ccname_size, "FILE:%s/%*s", P_tmpdir,
                  ngx_strlen(escaped), escaped);
@@ -850,6 +853,10 @@ ngx_http_auth_spnego_store_delegated_creds(ngx_http_request_t *r,
     ngx_http_auth_spnego_set_variable(r, &var_name, &var_value);
 
     ngx_pool_cleanup_t *cln = ngx_pool_cleanup_add(r->pool, 0);
+    if (cln == NULL) {
+        return NGX_ERROR;
+    }
+
     cln->handler = ngx_http_auth_spnego_krb5_destroy_ccache;
     cln->data = ccname;
 done:

[stnoonan]

LGTM with a minor nit.

Could you also rebase since your other change is already merged?

[stnoonan]

This codebase prefers constants on the LHS of a comparator. Could you reverse these for consistency?

if (NULL == cln).

[Fahnenfluchtige]

Hope everything is alright now