Use after free in SSL_free and SSL_set_bio

[pascal-cuoq]

The anti-pattern free (p); if (p != q) free (q); is used in two places in ssl_lib.c:

https://github.com/libressl-portable/openbsd/blob/c81daac11c5df39dd5215c024c103b54f054cbf6/src/lib/libssl/src/ssl/ssl_lib.c#L507

https://github.com/libressl-portable/openbsd/blob/c81daac11c5df39dd5215c024c103b54f054cbf6/src/lib/libssl/src/ssl/ssl_lib.c#L576

The use of p in p != q is a use-after-free (probably originally written to avoid a double-free, which is only a specialized kind of use-after-free). All uses-after-free are bad. The boolean condition p != q should be computed before freeing p.

[busterb]

Checking the freed pointer value is not a use-after-free. The pointer would have to be dereferenced first, and it is not here.

[pascal-cuoq]

By this logic, What would you say is the set of possible outputs of the following program?

#include <stdio.h>
#include <stdlib.h>

int main(void) {
  int *p = (int*)malloc(sizeof(int));
  int *q = (int*)realloc(p, sizeof(int));
  *p = 1;
  *q = 2;
  if (p == q) printf("%d %d\n", *p, *q);
}

It seems to me that if “checking [a] freed pointer value is not a use-after-free”, then a program that contains the line if (p == q) printf("%d %d\n", *p, *q); should only every print two identical values.

The above program, when compiled with Clang, prints “1 2”.

A dangling pointer is indeterminate. Using indeterminate memory for anything is undefined behavior. The above program invokes undefined behavior. It can do anything.

SSL_free and SSL_set_bio invoke undefined behavior for exactly the same reason as the above example. They can do anything for exactly the same reason.

See also: A dangling pointer is indeterminate. I co-wrote that post, I am not claiming that it is a second opinion, I am claiming that it contains examples that you can reproduce with your own version of GCC and/or Clang, the version that thousands of people will use to compile LibreSSL, and if the code compares dangling pointers, these compilers do not have enough respect for it to generate a binary that follows the rules of logic.

[bob-beck]

man 3 realloc please...

[pascal-cuoq]

It has been pointed out to me that the following modified example makes a stronger point (modification by Jesse Ruderman).

int main() {
int *p = (int*)malloc(sizeof(int));
if (p == NULL) { return 3; }
int *q = (int*)realloc(p, sizeof(int));
if (q == NULL) { return 3; }
*q = 2;
if (p == q) {
*p = 1;
printf(“%d %d\n”, *p, *q);
}
}

clang -Weverything -O r.c && ./a.out
1 2

[pascal-cuoq]

@bob-beck I don't know which man page you are referring to, but since LibreSSL aims to sane POSIX platforms, perhaps we can use the C99 standard instead?

J.2 Undefined behavior

… — The value of a pointer that refers to space deallocated by a call to the free or realloc function is used (7.20.3).

[jamesmulcahy]

"is used to access an object" is the key bit of that phrase. That isn't what's going on in your original code example.

[pascal-cuoq]

I temporarily copy-pasted the wrong line, sorry about that (too many references to realloc in J.2). “is used” is the key bit of the updated quote.

[bob-beck]

brent I'm not convinced this isn't an issue.. I'm re-opening until we decide for certain

[bob-beck]

At a minimum it's ugly as heck and should be redone more sanely. but this is an area to tread cautiously in.

[busterb]

OK, now that you actually explained the issue, it makes more sense. Agreed it's certainly ugly regardless of whether it's correct.

[bob-beck]

On Thu, Oct 15, 2015 at 04:26:37PM -0700, Brent Cook wrote:

OK, now that you actually explained the issue, it makes more sense. Agreed it's certainly ugly regardless of whether it's correct.

I have diffs mailed for it.

---

Reply to this email directly or view it on GitHub: https://github.com/libressl-portable/openbsd/issues/54#issuecomment-148550536

[bob-beck]

Fix commited - thanks