Opened 4 months ago

Last modified 4 months ago

#2673 new defect

ngx_http_limit_req_module race condition that can potentialy result in wrong delay calculation

Reported by: dev-null-undefined@… Owned by:
Priority: minor Milestone:
Component: nginx-module Version: 1.25.x
Keywords: limit-req-module race-condition Cc: dev-null-undefined@…
uname -a: Linux xps 6.10.1 #1-NixOS SMP PREEMPT_DYNAMIC Wed Jul 24 13:54:07 UTC 2024 x86_64 GNU/Linux
nginx -V: nginx version: nginx/1.26.1
built by gcc 13.3.0 (GCC)
built with OpenSSL 3.0.14 4 Jun 2024
TLS SNI support enabled
configure arguments: --prefix=/nix/store/rb9n6mhvp93f9kyir6zijajbvprnvp3c-nginx-1.26.1 --sbin-path=bin/nginx --with-http_ssl_module --with-http_v2_module --with-http_realip_module --with-http_addition_module --with-http_xslt_module --with-http_sub_module --with-http_dav_module --with-http_flv_module --with-http_mp4_module --with-http_gunzip_module --with-http_gzip_static_module --with-http_auth_request_module --with-http_random_index_module --with-http_secure_link_module --with-http_degradation_module --with-http_stub_status_module --with-threads --with-pcre-jit --http-log-path=/var/log/nginx/access.log --error-log-path=/var/log/nginx/error.log --pid-path=/var/log/nginx/nginx.pid --http-client-body-temp-path=/tmp/nginx_client_body --http-proxy-temp-path=/tmp/nginx_proxy --http-fastcgi-temp-path=/tmp/nginx_fastcgi --http-uwsgi-temp-path=/tmp/nginx_uwsgi --http-scgi-temp-path=/tmp/nginx_scgi --with-openssl-opt=enable-ktls --with-stream --with-stream_realip_module --with-stream_ssl_module --with-stream_ssl_preread_module --with-file-aio --add-module=/nix/store/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee-rtmp --add-module=/nix/store/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee-dav --add-module=/nix/store/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee-moreheaders

Description

If I am not mistaking there is potential race condition between the lookup method and delay calculation method, that can result into delay being calculated based on wrong values (from another tree node (with different key)).

The version of the nginx neither the uname -a are important since this I believe is bug in the source code.

I believe the race condition is on the field ngx_http_limit_req_ctx_t.node.

Which is first calculated and set here: https://trac.nginx.org/nginx/browser/nginx/src/http/modules/ngx_http_limit_req_module.c#L246-L251

But the shared memory mutex is unlocked immediately after each "zone"/"limit" is looked at.

And then the value is used here: https://trac.nginx.org/nginx/browser/nginx/src/http/modules/ngx_http_limit_req_module.c#L557

but as the mutex is unlocked before the code gets there the pointer could be overridden by another thread with different key. There for the delay would be calculated based on the other request key node.

Sorry if mistaking and wasting your time.
Thanks for looking into it if you do.

Change History (2)

comment:2 by dev-null-undefined@…, 4 months ago

Actually on second look at the code, I do not think there is a bug there since that structure is not inside of the shared memory and there for there can not be data race.

Sorry for the ticket, my bad.

Note: See TracTickets for help on using tickets.