Opened 15 months ago

Closed 15 months ago

Last modified 15 months ago

#1641 closed defect (invalid)

Possible buffer overflow in ngx_http_request (1.15.3)

Reported by: ivan.aksamentov@… Owned by:
Priority: critical Milestone:
Component: nginx-core Version: 1.15.x
Keywords: http request buffer overflow lto Cc:
uname -a: Linux omega 4.15.0-34-generic #37~16.04.1-Ubuntu SMP Tue Aug 28 10:44:06 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
nginx -V: nginx version: nginx/1.15.3 (Ubuntu) built with OpenSSL 1.1.1 11 Sep 2018 TLS SNI support enabled configure arguments: --prefix=/usr/share/nginx --sbin-path=/usr/sbin/nginx --pid-path=/run/nginx.pid --conf-path=/etc/nginx/nginx.conf --lock-path=/var/lock/nginx.lock --modules-path=/usr/lib/nginx/modules --http-log-path=/var/log/nginx/access.log --error-log-path=/var/log/nginx/error.log --build=Ubuntu --http-client-body-temp-path=/var/lib/nginx/body --http-fastcgi-temp-path=/var/lib/nginx/fastcgi --http-proxy-temp-path=/var/lib/nginx/proxy --http-scgi-temp-path=/var/lib/nginx/scgi --http-uwsgi-temp-path=/var/lib/nginx/uwsgi --group=www-data --user=www-data --add-module=/home/ia/src/nginx-1.15.3/ngx_brotli --add-module=/home/ia/src/nginx-1.15.3/ngx_echo --add-module=/home/ia/src/nginx-1.15.3/ngx_cache_purge --add-module=/home/ia/src/nginx-1.15.3/ngx_headers_more --with-compat --with-file-aio --with-threads --with-stream --with-openssl=/home/ia/src/nginx-1.15.3/openssl-1.1.1 --with-openssl-opt=enable-tls1_3 --with-http_auth_request_module --with-http_degradation_module --with-http_gunzip_module --with-http_gzip_static_module --with-http_realip_module --with-http_secure_link_module --with-http_slice_module --with-http_ssl_module --with-http_stub_status_module --with-http_sub_module --with-http_v2_module --with-mail_ssl_module --with-stream --with-stream_realip_module --with-stream_ssl_module --with-stream_ssl_preread_module --without-http_auth_basic_module --without-http_autoindex_module --without-http_browser_module --without-http_empty_gif_module --without-http_geo_module --without-http_memcached_module --without-http_scgi_module --without-http_split_clients_module --without-http_ssi_module --without-http_userid_module --without-http_uwsgi_module --with-cc-opt='-O2 -march=native -fomit-frame-pointer -fPIE -pie -fstack-protector-strong -D_FORTIFY_SOURCE=2' --with-ld-opt='-fPIE -pie -Wl,-flto -Wl,-Bsymbolic-functions -Wl,-z,relro -Wl,-z,now'

Description

When building Nginx 1.15.3 with Link-time optimization (LTO) and binutis build with gold plugin enabled, this warning is produced during linking of Nginx executable:

In function ‘__recv_alias’,
    inlined from ‘ngx_http_ssl_handshake’ at src/http/ngx_http_request.c:656:7:
/usr/include/x86_64-linux-gnu/bits/socket2.h:39:9: warning: call to ‘__recv_chk_warn’ declared with attribute warning: recv called with bigger length than size of destination buffer
  return __recv_chk (__fd, __buf, __n, __bos0 (__buf), __flags);

LTO implies some specific optimizations, which may expose issues that are hidden in normal builds (this warning do not appear in non-LTO builds). LTO also allows for additional static checks that was able to detect the overrun in this case. This may be an important finding, but I am not familiar with Nginx codebase and I would be glad if someone could investigate this.

I provide some of the details about my configuration below:

Versions:

Ubuntu 16.04.4 x64
GCC 5.4.1 (default)
Binutils 2.31.1 (custom build, with `--enable-gold`)
Nginx 1.15.3
OpenSSL 1.1.1 (built along with nginx)

Flags:

export CFLAGS='-O2 -flto -march=native -fomit-frame-pointer -fPIE -pie -fstack-protector-strong -D_FORTIFY_SOURCE=2'
export LDFLAGS='-fPIE -pie -flto -Wl,-flto -Wl,-Bsymbolic-functions -Wl,-z,relro -Wl,-z,now'
export AR=gcc-ar
export RANLIB=gcc-ranlib
export NM=gcc-nm

Plugins:
https://github.com/google/ngx_brotli
https://github.com/openresty/echo-nginx-module
https://github.com/FRiCKLE/ngx_cache_purge
https://github.com/openresty/headers-more-nginx-module

Attachments (4)

build_binutils.sh (816 bytes) - added by ivan.aksamentov@… 15 months ago.
Bash script to build binutils 2.31.1 with gold plugin enabled
versions.txt (3.8 KB) - added by ivan.aksamentov@… 15 months ago.
Version info of various tools used
nginx-lto-build-log.txt.tar.gz (57.2 KB) - added by ivan.aksamentov@… 15 months ago.
Full Nginx LTO build log
build_nginx.sh (4.4 KB) - added by ivan.aksamentov@… 15 months ago.
Bash script to build nginx 1.15.3 with LTO

Download all attachments as: .zip

Change History (8)

Changed 15 months ago by ivan.aksamentov@…

Bash script to build binutils 2.31.1 with gold plugin enabled

Changed 15 months ago by ivan.aksamentov@…

Version info of various tools used

Changed 15 months ago by ivan.aksamentov@…

Full Nginx LTO build log

Changed 15 months ago by ivan.aksamentov@…

Bash script to build nginx 1.15.3 with LTO

comment:1 follow-up: Changed 15 months ago by mdounin

  • Resolution set to invalid
  • Status changed from new to closed

The recv() call in question uses the buffer defined at the start of the function:

    u_char                    *p, buf[NGX_PROXY_PROTOCOL_MAX_HEADER + 1];
    size_t                     size;

and uses either sizeof(buf) or 1 as the size:

    size = hc->proxy_protocol ? sizeof(buf) : 1;
    
    n = recv(c->fd, (char *) buf, size, MSG_PEEK);

Clearly the destination buffer is always bigger than the length passed to the recv() call in all possible cases. If recv() is indeed called with length bigger than the size of the destination buffer, it must be a problem with optimizations you are using.

Also, thank you for all your respect and support for nginx project as seen in the following lines of your build scripts:

            # Remove nginx name and version info from headers and pages
            sed -i -e 's/static u_char ngx_http_server_string\[\] = "Server: nginx" CRLF;/static u_char ngx_http_server_string[] = "" CRLF;/g' src/http/ngx_http_header_filter_module.c
            sed -i -e 's/static u_char ngx_http_server_full_string\[\] = "Server: " NGINX_VER CRLF;/static u_char ngx_http_server_full_string[] = "" CRLF;/g' src/http/ngx_http_header_filter_module.c
            sed -i -e 's/static u_char ngx_http_server_build_string\[\] = "Server: " NGINX_VER_BUILD CRLF;/static u_char ngx_http_server_build_string[] = "" CRLF;/g' src/http/ngx_http_header_filter_module.c

Your support is appreciated.

comment:2 in reply to: ↑ 1 ; follow-up: Changed 15 months ago by ivan.aksamentov@…

Replying to mdounin:

Okay, sorry for the false alarm.

Is it considered disrespectful to remove the server info? Brief internet search gave no explanation. Please clarify.

Last edited 15 months ago by ivan.aksamentov@… (previous) (diff)

comment:3 in reply to: ↑ 2 ; follow-up: Changed 15 months ago by vbart

Replying to ivan.aksamentov@…:

Replying to mdounin:

Okay, sorry for the false alarm.

Is it considered disrespectful to remove the server info? Brief internet search gave no explanation. Please clarify.

See ticket:936#comment:4.

There are various analytical agencies, that publish reports regarding web-server software popularity, like W3Techs, Netcraft, and etc. They generally use information from the "Server" header. Those publications help us to rise more money for developing free open-source version of nginx.

comment:4 in reply to: ↑ 3 Changed 15 months ago by ivan.aksamentov@…

Replying to vbart:

Ah, I see. I was mislead by all these articles on the web that tell you to "harden" nginx.
Will totally bring the header back on!

Note: See TracTickets for help on using tickets.