Is this malloc wrapper safe?

Question

I am trying to stop integer overflow vulnerabilities by creating a simple wrapper around malloc(3) and related functions. The idea is that it returns a NULL pointer if the amount of required memory is too large for the size_t argument (or zero). However my implementation does not seem to be satisfying our static analyzer. Can anyone identify what is wrong with this implementation?

template <typename T>
T* safe_malloc(const size_t num_elements)
{
  if (num_elements <= 0 || num_elements > SIZE_MAX / sizeof(T))
  {
    return NULL;
  }
  return static_cast<T*>(malloc(sizeof(T) * num_elements));
}

Usage:

// Previously:
int *a = (int*)malloc(sizeof(int)); // single value
int *b = (int*)malloc(sizeof(int) * ATTACKER_CONTROLLED_VALUE); // array
if (b == NULL) {
  // out of memory
}


// Now:
int *a = safe_malloc<int>(1); // single value
int *b = safe_malloc<int>(ATTACKER_CONTROLLED_VALUE); // array
if (b == NULL) {
  // out of memory OR user provided value that would overflow, or zero
}

What static analyzer is it, and what does it say is wrong with the code? — Gilles 'SO- stop being evil', Feb 23 '21 at 20:06
@Gilles'SO-stopbeingevil' Fortify, and it says that there is a possible integer overflow at the line where `malloc` is called. The only other information it gives is the trace showing `ATTACKER_CONTROLLED_VALUE` is indeed attacker-controlled, which I already knew. — matoro, Feb 23 '21 at 20:11
I think you are mixing C and C++, something that you need to reconsider, mixing the languages is not a good idea, not only for security reasons. Have a look to https://stackoverflow.com/questions/184537/in-what-cases-do-i-use-malloc-and-or-new — camp0, Feb 23 '21 at 21:18
@camp0 Trust me, if it were up to me all the C-isms would be ditched, but this is for a very large and old codebase that depends on them, but since the whole thing is compiled as C++, I figured I might as well take advantage of that where I could (hence the templates). — matoro, Feb 23 '21 at 21:24
@multithr3at3d You can't. Where do you see that? I don't quite follow. — matoro, Feb 23 '21 at 22:53
@multithr3at3d I am just trying to be extremely overcautious to get Fortify to recognize the code as safe. It can't hurt to do `<= 0`, since the equality check against 0 is included in that. — matoro, Feb 25 '21 at 03:31

LSerni · Answer 1 · 2021-02-23T21:38:48.033

1

When you write,

template <typename T>T* safe_malloc(const size_t num_elements)

you're saying that num_elements can hold a full size_t. But if that is the case, then multiplying it by sizeof(T) might make it overflow, since the result also has to be size_t.

return static_cast<T*>(malloc(sizeof(T) * num_elements));

I do not know how to easily make fortify stop complaining. Possibly you can cast the desired memory amount to a larger size representation, check explicitly that it does not exceed SIZE_MAX, and if it does not, cast it back to size_t. That ought to be clear enough for the static analyzer to understand what's going on.

template <typename T> T* safe_malloc(const size_t num_elements) {
  unsigned long long required;
  required = num_elements;
  required *= sizeof(T);
  if ((0 == required) || (required > SIZE_MAX)) {
    return NULL;
  } else {
    return static_cast<T*>(malloc(required);
  }
}

edited Feb 23 '21 at 21:38

answered Feb 23 '21 at 21:29

LSerni

22,521
4
51
60

Is that not what the division check is doing? It aborts if `num_elements > SIZE_MAX / sizeof(T)` which is the same as asserting that `num_elements * sizeof(T) <= SIZE_MAX`. – matoro Feb 23 '21 at 21:36
1

Yes. Your code works and has always worked. But I believe that the problem is that `fortify` doesn't understand what it does. I've added a different attempt (needn't check for negative numbers, as `size_t` is unsigned anyway) – LSerni Feb 23 '21 at 21:40
1

I really appreciate the attempt, but unfortunately that still wasn't enough - it's still complaining about there being integer overflow at the `malloc` line. I don't want to mark the answer as accepted just in case someone else with the same issue stumbles across it. – matoro Feb 23 '21 at 22:18
That's right. You are expected NOT to mark accepted an answer unless it REALLY helps you, otherwise there'd be no point. I tried casting `required` to `size_t`. It could be worthwhile to assign `required` to a `size_t` variable and use *that* variable in the malloc. I've tried looking for fortify examples (this kind of bound checking ought to be *common*!) but with no luck. – LSerni Feb 23 '21 at 23:05

Is this malloc wrapper safe?

1 Answers1