Possible buffer overflow in ngx_http_request (1.15.3)

[Ivan Aksamentov]

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

[Ivan Aksamentov]

Bash script to build binutils 2.31.1 with gold plugin enabled

[Ivan Aksamentov]

Version info of various tools used

[Ivan Aksamentov]

Full Nginx LTO build log

[Ivan Aksamentov]

Bash script to build nginx 1.15.3 with LTO

[Maxim Dounin]

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.

[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.

[Valentin V. Bartenev]

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.

[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!