Page MenuHome

Fix some buffer overflow vulnerabilities in mesh code.

Authored by Brecht Van Lommel (brecht) on Jan 15 2018, 12:20 AM.



Hopefully solves these security issues from T52924:

I only checked code superficially though, and did not verify because the
repro files do not appear to be publicly available.

It works by adding MEM_malloc_arrayN() and MEM_calloc_arrayN() functions that
return NULL on integer overflow. These functions were only used in some
files around the reported vulnerabilities, not all over Blender, not sure
if we should try to use them everywhere. There is also no realloc, mapalloc
or aligned array function yet.

Diff Detail

rB Blender

Event Timeline

  • Add fixes for the remaining vulnerabilities, far from elegant though.
  • Abort in case of integer overflow in MEM_.alloc_arrayN, since we lack proper tests for NULL return in a lot of places. The idea being that the overflow means we have invalid data anyway that would most likely crash later, so better do it immediately than when it could lead to vulnerabilities.

You must be using calloc() to allocate arrays. There is a really good reason why malloc() and calloc() are having different semantics, and why they are boiling down to different kernel functions (kmalloc() and kzalloc() on Linux).

Consider such example code:

1// gcc -Wall -D USE_CALLOC main.c -o allocate_array
2// gcc -Wall -D USE_MALLOC main.c -o allocate_array
4#include <stdio.h>
5#include <stdlib.h>
7// #define USE_CALLOC
8// #define USE_MALLOC
10#define BLOCK_SIZE ((size_t)32 * 1024 * 1024 * 1024)
12#if defined(USE_CALLOC) && defined(USE_MALLOC)
13# error "Only one of USE_CALLOC and USE_MALLOC is to be defined!"
16#if !defined(USE_CALLOC) && !defined(USE_MALLOC)
17# error "One of USE_CALLOC and USE_MALLOC is to be defined!"
20void* allocate_array(size_t len, size_t count) {
21 // NOTE: We do not do any numeric overflow checks here, this example is NOT
22 // supposed to overflow. It demonstrates totally different effect of memory
23 // allocation.
24#ifdef USE_CALLOC
25 return calloc(len, count);
27#ifdef USE_MALLOC
28 return malloc(len * count);
32int main(int argc, char** argv) {
33 (void) argc;
34 (void) argv;
35 size_t total_allocation = 0;
36 for (int i = 0; i < 32; ++i) {
37 void* mem = allocate_array(sizeof(char), BLOCK_SIZE);
38 if (mem != NULL) {
39 total_allocation += BLOCK_SIZE;
40 }
41 printf("iter:%d mem:%p total_allocation:%lu\n", i, mem, total_allocation);
42 }
43 return EXIT_SUCCESS;

Running it on a system with 64GB physical memory and no swap, will give non-NULL pointer for every allocation using malloc(), even though obviously you can't use all the requested memory. Version with calloc() will properly return non-NULL for first iteration, but after that it will return NULL.

So, if we call this change buffer overflow vulnerabilities we have to use calloc for both MEM_calloc_arrayN AND MEM_malloc_arrayN, otherwise we are still exposed to the buffer overflow.

From this point of view:

  • Always use calloc() for array allocations.
  • Remove own safety check function which does integer overflow check (which isn't even covered with unit tests! How would one trust it?).
  • Check result to be non-NULL, otherwise abort() in the same way as it is done for integer overflow.
Sergey Sharybin (sergey) requested changes to this revision.Jan 15 2018, 10:49 AM
This revision now requires changes to proceed.Jan 15 2018, 10:49 AM

Followup based on discussion with Bastien in IRC.

The approach i've noted above will slow things down (extra memset etc), which is not necessarily what we want. But there are ways around this. I would state: this part of our allocator is really easy to switch on runtime. What this means is:

  • We can have current Brecht's implementation as mem_{c,m}alloc_array_fast.
  • But we also should have proper calloc() based allocations as stated above, something like mem_{c,m}alloc_array_safe.
  • Actual MEM_{c,m}alloc_arrayN pointer can be assigned to either of those. Similar to what we do with fully guarded allocator, we can have MEM_use_safe_array_allocator.
  • Decision on whether we want fully safe allocator or not we make based on trusted source, similar to our python scripts.
  • Current code with integer overflow we can keep, it doesn't add measurable slowdown, and will still be handy to catch obvious issues. But cover it with tests!

I'm not sure why using calloc() would be safer for the kind of vulnerability we are trying to protect against? malloc() lets you allocate more memory than you can actually use, but still the allocations should not overlap and you should not be able to overwrite other memory that is in use, as long as you don't write outside the bounds for each memory allocation? The program may crash when out of memory, but not in a way that can be exploited?

If it can be exploited, what makes an array allocation different than another allocation? Wouldn't it be possible to encounter the same issue for a fixed size allocation, or is the assumption that such allocations typically have a smaller size and so are handled differently in some way?

And yes we need unit tests for this, but I wanted some feedback on the overall approach first.

I am not expert in what's exactly is happening under the hood, but scary thing is: you can READ all the memory allocated by malloc(). So who's memory is being addressed then? Is it some "fake" memory page filled in with zeroes? What is the exact time when pages are getting created?

Things i know for sure: calloc() is designed to deal with this cases. And any crash is a potentially open door for exploits.

My understanding of other types of allocs, is that they are fixed-size allocations, and hence are rather really tricky to get use of them for exploits. Maybe you can still put some random data in them, but making them to reliably execute your code could be really tricky since of the really tight space available for this.

My guts are saying: unless there's an expert saying "memory returned by optimistic allocator in Linux is not exploitable" i'd go the safest possible way.

That being said, looked into libpng and ffmpeg. They don't use real calloc(). But libpng is leaving pixels allocations to the software, and ffmpeg is limiting array size quite dramatically. Not sure what to make of this.

Ok, so after deeper investigation and discussing things with Brecht in IRC it appears to be fine to use malloc(). Writes can't write to someone's else memory. So only harm here would be reads of memory for which pages aren't created yet. But that's usual read-of-uninitialized memory which is to be fixed anyway.

Additionally, in Linux kernel calloc() boils down to regular malloc() + memset().

Brecht also looked into STL, where new() is boiling down to posix_memalign() which has single argument for memory size.

That makes me convinced it's ok to ignore SIGSEGV generated when writing to optimistically-allocated malloc()-ed memory.

That makes me feel fine about changes in guarded allocator, and in the sources elsewhere. But would still love to see unit test for integer overflow test.

I discussed this with @Sergey Sharybin (sergey) in IRC, and conclusion was that malloc() should be ok to use here. The only real difference with calloc() appears to be the zeroing, and we are not trying to protect against reading uninitialized memory here.

When allocating a big chunk of memory, it may not be immediately assigned to a physical page, but rather get paged in by the kernel after a page fault. If the system runs out of memory this may cause Blender to crash, but this doesn't appear to be exploitable as far as we can tell.

Brecht Van Lommel (brecht) updated this revision to Diff 9826.

Add unit tests.

The major missing things are MEM_reallocN() versions for arrays, and the fact
that only a somewhat random subset of the code was changed related to the found

@Ton Roosendaal (ton) or @Talos Security Advisory (regiwils), do you have the repro .blends or python scripts so we can
verify our solution works?

Generally LGTM.

Not sure it matters but it will be easy to swap sizeof/length arguments. GCC/Clang can ensure the second argument is a constant - but there will be a handful of cases where the size isn't known at compile time so no simple solution for that afaics.

548 ↗(On Diff #9818)

Should use UNLIKELY(...), also use elsewhere.

Or swap the checks so the common case happens first.

179 ↗(On Diff #9818)

We could have mallocn_inline.h since this is only needed for internal use.

184 ↗(On Diff #9818)

(size_t)defbase_tot needed.

MOD_mask.c:184:41: error: conversion to ‘size_t {aka long unsigned int}’ from ‘int’ may change the sign of the result [-Werror=sign-conversion]
268 ↗(On Diff #9818)

cast needed here too.

Address comments.

The argument order does not matter, swapped arguments will work fine. I just
followed the naming of calloc().

This revision was not accepted when it landed; it landed in state Needs Review.Jan 17 2018, 8:30 PM
This revision was automatically updated to reflect the committed changes.

Update to fix more crashes found after testing with repro cases.

The test files provided are all manually created with Python scripts for some reason, and they are generally corrupt and incomplete, not actual usable .blend files. As a result, they can cause crashes in other places than the reported vulnerabilities.

This revision was not accepted when it landed; it landed in state Needs Review.Jan 18 2018, 12:55 AM
This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.