Opened 5 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: | 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:1 by , 5 months ago
comment:2 by , 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.
Probably introduced by https://trac.nginx.org/nginx/changeset/9ce48f9eb85b8e301366916af78c0d6aaa4d2e01/nginx