Fixed: $naxsi_request_id (issue #142)

[lubomudr]

Hi

I propose a solution to issue #142

The first commit is simply a fix for $naxsi_request_id not being available in an internal redirect

The second one has been fixed: now $naxsi_request_id is inherited by NGINX $request_id and is calculated only when necessary, for example, logging an error or directly accessing a variable.

Since the size of $request_id is fixed in NGINX and is always 16 random bytes, in hexadecimal, the size of the string is explicitly specified in the code

[wargio]

Please, address the changes.

[lubomudr]

Ok

I specifically removed the "request_id" size constants because I believed that since this is inherited from NGINX, then these are internal structures and should not be changed in any way in the NAXSI code

But if you maintain compatibility with NGINX, when it does not yet contain the “request_id” variable, you will have to implement it yourself and these constants are needed. So I got excited here 😄

But in any case, calculating naxsi_request_id for EVERY request is expensive. The algorithm has been slightly modified - it is calculated only by demand and only if the "request_id" variable is not defined. Otherwise naxsi_request_id is equal to request_id.

[wargio]

LGTM, but now i wonder if it would just make more sense to replace rid with just request_id and implement a check for the NGINX version and add the extra code just for the older versions.

[lubomudr]

Information about naxsi _request_id has already been leaked 😄

[wargio]

Information about naxsi _request_id has already been leaked 😄

could be just an alias.

[wargio]

To be clear, i'm very ok with this optimization, but i just checked the code that nginx uses for generating the value and is essentially the same as ours.

ngx_http_variable_request_id(ngx_http_request_t *r,
    ngx_http_variable_value_t *v, uintptr_t data)
{
    u_char  *id;

#if (NGX_OPENSSL)
    u_char   random_bytes[16];
#endif

    id = ngx_pnalloc(r->pool, 32);
    if (id == NULL) {
        return NGX_ERROR;
    }

    v->valid = 1;
    v->no_cacheable = 0;
    v->not_found = 0;

    v->len = 32;
    v->data = id;

#if (NGX_OPENSSL)

    if (RAND_bytes(random_bytes, 16) == 1) {
        ngx_hex_dump(id, random_bytes, 16);
        return NGX_OK;
    }

    ngx_ssl_error(NGX_LOG_ERR, r->connection->log, 0, "RAND_bytes() failed");

#endif

    ngx_sprintf(id, "%08xD%08xD%08xD%08xD",
                (uint32_t) ngx_random(), (uint32_t) ngx_random(),
                (uint32_t) ngx_random(), (uint32_t) ngx_random());

    return NGX_OK;
}

[lubomudr]

To be clear, i'm very ok with this optimization, but i just checked the code that nginx uses for generating the value and is essentially the same as ours.

Yes
The optimization is that this is not calculated for EVERY request, regardless of whether the value is needed or not

[wargio]

Last two small changes and we can merge