-
Notifications
You must be signed in to change notification settings - Fork 92
Description
Issue#1: Sigsegv due to writing in unallocated bytes:
In mod_ngrest_nginx.c
mod_ngrest_to_cstring function modifies a const argument. The argument ngx_str_t is not a null terminated string and the code adds '\0' beyond allocated length bytes.
This is the code:
static char* mod_ngrest_to_cstring(const ngx_str_t* str, ngx_pool_t* pool)
{
char* res = ngx_pcalloc(pool, str->len + 1);
strncpy(res, str->data, str->len);
str->data[str->len] = '\0';
return res;
}
The fix is as below. The original intention is to return a null terminated c string
static char* mod_ngrest_to_cstring(const ngx_str_t* str, ngx_pool_t* pool)
{
char* res = ngx_pcalloc(pool, str->len + 1);
strncpy(res, str->data, str->len);
// str->data[str->len] = '\0';
res[str->len] = '\0';
return res;
}
Issue #2: copying pointer to dst->data is incorrect
static void mod_ngrest_to_ngx_string(const char* src, ngx_str_t* dst)
{
dst->data = (u_char*) src;
dst->len = strlen(src);
}
FIx:
static ngx_int_t
mod_ngrest_to_ngx_string(ngx_pool_t* pool, const char* src, ngx_str_t* dst)
{
size_t len = strlen(src);
u_char *p = ngx_palloc(pool, len);
if (p == NULL) {
dst->data = NULL;
dst->len = 0;
return NGX_ERROR;
}
ngx_memcpy(p, src, len);
dst->data = p;
dst->len = len;
return NGX_OK;
}
Since the signature is modified, the callers need to pass the pool
Issue #3: Context allocated in stack. This resulted in incomplete response when the response is large. In my case it was around 230k. mod_ngrest_request_ctx_t has variables that get allocated in stack, but context has to be active throughout the life time of request
typedef struct {
ngx_http_request_t* req;
ngx_chain_t* inChain;
int64_t inOffset;
ngx_chain_t outChain;
ngx_chain_t* outLastChain;
ngx_buf_t outFirstBuf;
} mod_ngrest_request_ctx_t;
outChain (allocted in stack) is used in mod_ngrest_write_client_callback
In function mod_ngrest_handler
mod_ngrest_request_ctx_t ctx;
ngx_memzero(&ctx, sizeof(ctx));
ctx.req = req;
Fix:
mod_ngrest_request_ctx_t* ctx =
ngx_pcalloc(req->pool, sizeof(mod_ngrest_request_ctx_t));
if (ctx == NULL) {
ngx_log_error(NGX_LOG_ERR, req->connection->log, 0,
"mod_ngrest: failed to allocate request ctx");
ngx_http_finalize_request(req, NGX_HTTP_INTERNAL_SERVER_ERROR);
return;
}
In subsequent code, use ctx pointer instead of &ctx
I have attached the fix also.