Opened 8 days ago

Last modified 7 days ago

#2416 new defect

Memory overrun due to alignment issues when cross-compiling mingw on linux

Reported by: Orgad Shaneh Owned by:
Priority: minor Milestone: nginx-1.23
Component: nginx-core Version: 1.22.x
Keywords: memory security vulnerability Cc: Orgad Shaneh
uname -a: MINGW64_NT-10.0-19043
nginx -V: nginx version: nginx/1.22.1
built with OpenSSL 1.1.1s 1 Nov 2022
TLS SNI support enabled
configure arguments: --crossbuild=win32 --with-http_stub_status_module --with-http_ssl_module --with-http_realip_module --without-http_scgi_module --without-http_uwsgi_module --with-openssl=../openssl --with-stream --with-stream_ssl_module --with-http_auth_request_module --with-http_sub_module --without-http_auth_basic_module --with-http_gzip_static_module --conf-path=nginx.conf --with-pcre=pcre --with-zlib=zlib-1.2.13

Description (last modified by Orgad Shaneh)

Hi,

When I cross-compile nginx for Windows on linux host using mxe, NGX_ALIGNMENT is set to 16. This is set in auto/os/conf:

case "$NGX_MACHINE" in
...
    *)
        have=NGX_ALIGNMENT value=16 . auto/define
        NGX_MACH_CACHE_LINE=32
    ;;
esac

But the pool is allocated using plain malloc (on Windows target), which is *not* aligned to 16 bytes (it's aligned to 4), so pool->d.end is 4-byte aligned.

Now, in ngx_palloc_small, the following code causes integer underflow, and the comparison doesn't work as expected:

        m = p->d.last;

        if (align) {
            m = ngx_align_ptr(m, NGX_ALIGNMENT);
        }

        if ((size_t) (p->d.end - m) >= size) { /* <------- This condition */
            p->d.last = m + size;

            return m;
        }

Since m is aligned, and end is not, end - m can be a negative number. Then it is cast to size_t, which is unsigned, and so it just keeps writing past the memory pool, and overruns the next pool.

A minimal patch that solves this issue is:

  • src/core/ngx_palloc.c

    a b ngx_palloc_small(ngx_pool_t *pool, size_t size, ngx_uint_t align)  
    160160            m = ngx_align_ptr(m, NGX_ALIGNMENT);
    161161        }
    162162
    163         if ((size_t) (p->d.end - m) >= size) {
     163        if (m + size <= p->d.end) {
    164164            p->d.last = m + size;
    165165
    166166            return m;

But it would be better to assign the right alignment for Windows, even when cross-building. And also ensure that end is aligned correctly.

I also noticed that the default alignment when not set is sizeof(long), which is wrong for Win64. It should be sizeof(intptr_t).

Build command:

CC=i686-w64-mingw32.static-gcc ./auto/configure --crossbuild=win32 --without-http_rewrite_module --without-http_gzip_module

$ grep -1 ALIGN objs/ngx_auto_config.h

#ifndef NGX_ALIGNMENT
#define NGX_ALIGNMENT  16
#endif

Change History (2)

comment:1 by Orgad Shaneh, 8 days ago

Description: modified (diff)

comment:2 by Maxim Dounin, 7 days ago

Priority: criticalminor

Thanks for reporting this.

Crossbuilds are not really supported. The --crossbuild configure option was used a while ago for compilation of Windows binaries on Unix systems using Wine, mostly for compile-testing, though it is not really supported, and intentionally not documented neither in ./configure --help nor in the documentation. Nevertheless, this issue probably worth fixing.

Note though that the condition in ngx_palloc_small() is perfectly correct, but assumes pointers properly aligned at least to NGX_ALIGNMENT (see #686). Further, the suggested patch looks wrong, as m + size is not guaranteed to exist, and might overflow, leading to incorrect results even with correctly aligned pointers.

As I see it, there are two problems here:

  1. Alignment requirements are not correctly set, since ./configure does not know the machine architecture and assumes strict alignment requirements.
  1. Windows provides no standard interfaces for aligned allocations, such as posix_memalign() (and not even aligned_alloc() introduced by C11), so nginx uses plain malloc() for supposedly aligned allocations via ngx_memalign(), and too aggressive alignment requirements become important.

The most trivial fix would be to explicitly define the expected alignment for the target platform during compilation, such as ./configure --crossbuild=win32 --with-cc-opt="-DNGX_ALIGNMENT=8" for 32-bit builds (and NGX_ALIGNMENT=16 for 64-bit builds).

It might make sense to hardcode these into src/os/win32/ngx_win32_config.h to simplify things. Alternatively, assuming i386 or amd64 machine in case of --crossbuild=win32 might make sense, so the default alignment will be used.

Not sure if trying to support aligned allocations on win32 make sense. The only interface available seems to be _aligned_malloc(). It requires _aligned_free() for deallocation, which is not compatible with free(), so we'll have to use aligned allocations everywhere.

I also noticed that the default alignment when not set is sizeof(long), which is wrong for Win64. It should be sizeof(intptr_t).

The default alignment is the default, and does not pretend to be correct for all possible platforms. On Windows, malloc() is expected to provide 8 bytes alignment on 32-bit platforms, and 16 bytes on 64-bit platforms.

On the other hand, given that nginx on Windows does not support platforms with strict alignment requirements, any arbitrary small number will be good enough, including the default sizeof(unsigned long) as used now with native compilation.

Note: See TracTickets for help on using tickets.