I'd really appreciate review/feedback. I know this might seem like a theoretical issue, but we're seeing real world problems arising from how the stack memory is being allocated. We use a custom allocator to wrap talloc. Talloc calls the system malloc and requests an allocation of the talloc chunk header size + the memory requested. Talloc returns a pointer to the memory directly after its header. The fact we're using talloc is not the issue, but it is useful in detecting buffer overflows because on free talloc will check the integrity of its header via a magic number. When talloc chunks are used for stack memory by the ASYNC_* API, and are freed, talloc asserts as the magic number in the header has been altered. This is a strong indication of a stack overrun, likely resulting from the fact that either makecontext assumes the memory will be page aligned (and it's not), or the stack size being allocated is simply too small. If the current PR is not acceptable please indicate an alternative solution. I thought it'd be easiest to have OpenSSL punt on the problem, and leave it up to the application to allocate the stack correctly for a given platform, but if you wanted something less open ended, some alternatives could be add a configuration option to change the default stack size for the ASYNC_* API and allow the user to provide an aligned memory allocation function in addition to malloc and realloc. I also see that OpenSSL is moving towards library contexts so global allocators may no longer be appropriate. Let me know if that's an issue here and I'll move the code to using allocators bound to library contexts. -Arran > On Feb 23, 2022, at 4:29 PM, Arran Cudbard-Bell <a.cudbardb@xxxxxxxxxxxxxx> wrote: > > PR is now available here: https://github.com/openssl/openssl/pull/17762 > > -Arran > >> On Feb 22, 2022, at 11:10 AM, Arran Cudbard-Bell <a.cudbardb@xxxxxxxxxxxxxx> wrote: >> >> In our application we use the OpenSSL ASYNC_* API to jump out of verification and session load/store callbacks. >> >> On the POSIX side, when creating a new context OpenSSL calls the standard OPENSSL_malloc and >> OPENSSL_free functions to allocate memory for the stack passed into makecontext. >> >> https://github.com/openssl/openssl/blob/1c0eede9827b0962f1d752fa4ab5d436fa039da4/crypto/async/arch/async_posix.c >> >> There are several issues with this approach: >> >> 1. On some systems stack memory must be page aligned. The user provided allocation functions >> don't know the context in which the memory is being requested so can't ensure this. >> 2. glibc recommends the stack be allocated with mmap and set to executable to allow trampoline >> functions to work correctly (unsure if this is relevant). >> https://www.gnu.org/software/libc/manual/html_node/System-V-contexts.html >> 3. Using heap memory for the stack means there's no guard page. Overflowing the stack appears >> to cause silent memory corruption on both macOS and Linux. >> >> The stack size is also currently fixed and requires OpenSSL recompilation to change. >> >> I'd like to make the stack allocation and free functions configurable. >> >> Ideally the malloc function would have a signature similar to this: >> >> void *(*stack_alloc)(size_t *len); >> >> and free would be >> >> void (*stack_free)(void *stack); >> >> The idea being that the allocation function returns the memory it allocated for the stack, and the size of the stack via *len. >> >> If user stack alloc/free functions are not configured then we'd fall back to the standard OPENSSL_malloc() and OPENSSL_free() >> functions with a static stack size. >> >> For our particular use case we're planning to have stack_alloc create a new thread. We'll then return the stack allocated >> for that thread, which we believe in most cases fixes the issues described above. >> >> The free function will then signal/join the thread. >> >> -Arran >> >> >> Arran Cudbard-Bell <a.cudbardb@xxxxxxxxxxxxxx> >> FreeRADIUS Development Team >> >> FD31 3077 42EC 7FCD 32FE 5EE2 56CF 27F9 30A8 CAA2 >> >
Attachment:
signature.asc
Description: Message signed with OpenPGP
