Thread: BUG #16070: A double-free bug in interfaces/libpq/fe-secure-openssl.c

The following bug has been logged on the website:

Bug reference:      16070
Logged by:          rongxin wu
Email address:      wurongxin@xmu.edu.cn
PostgreSQL version: 10.10
Operating system:   ubuntu 18
Description:

In
https://github.com/postgres/postgres/blob/REL_10_STABLE/src/interfaces/libpq/fe-secure-openssl.c,
at Line 1206, it would call the function "ENGINE_finish" and free
conn->engine. At Line 1207, it would call the function "ENGINE_free" and
free conn->engine again. This would lead to a double free bug. Please read
the following code snippet. 

https://github.com/postgres/postgres/blob/REL_10_STABLE/src/interfaces/libpq/fe-secure-openssl.c
896. static int
897. initialize_SSL(PGconn *conn)
898. {
...
1206.                 ENGINE_finish(conn->engine);
1207.                 ENGINE_free(conn->engine);
...

int ENGINE_finish(ENGINE *e)
{
...
    to_return = engine_unlocked_finish(e, 1);
...
    return to_return;
}

int engine_unlocked_finish(ENGINE *e, int unlock_for_handlers)
{
    ...
    if (!engine_free_util(e, 0)) {
        ENGINEerr(ENGINE_F_ENGINE_UNLOCKED_FINISH,
ENGINE_R_FINISH_FAILED);
        return 0;
    }
    return to_return;
}

int engine_free_util(ENGINE *e, int not_locked)
{
    ...
    OPENSSL_free(e);  // a free wrapper
    return 1;
}

int ENGINE_free(ENGINE *e)
{
    return engine_free_util(e, 1);
}

Please help to check. Thanks.

PG Bug reporting form <noreply@postgresql.org> writes:
> In
> https://github.com/postgres/postgres/blob/REL_10_STABLE/src/interfaces/libpq/fe-secure-openssl.c,
> at Line 1206, it would call the function "ENGINE_finish" and free
> conn->engine. At Line 1207, it would call the function "ENGINE_free" and
> free conn->engine again. This would lead to a double free bug.

I don't really believe this; if there were a double-free problem here,
we'd surely have noticed it long since.

Taking a look at the OpenSSL source code, it looks like engine_free_util
decrements a reference count and doesn't actually delete anything until
that's gone to zero.  So maybe the refcount is 2 at the beginning of
this sequence?

            regards, tom lane

On Sun, Oct 20, 2019 at 10:17:11AM -0400, Tom Lane wrote:
> I don't really believe this; if there were a double-free problem here,
> we'd surely have noticed it long since.

Yeah.

> Taking a look at the OpenSSL source code, it looks like engine_free_util
> decrements a reference count and doesn't actually delete anything until
> that's gone to zero.  So maybe the refcount is 2 at the beginning of
> this sequence?

The docs of OpenSSL mention the use of both successively, where
ENGINE_free() does the cleanup after ENGINE_by_id(), and
ENGINE_finish() cleans up after ENGINE_init():
https://www.openssl.org/docs/man1.1.0/man3/ENGINE_finish.html

And an actual issue is that we have no coverage for it:
https://coverage.postgresql.org/src/interfaces/libpq/fe-secure-openssl.c.gcov.html
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> The docs of OpenSSL mention the use of both successively, where
> ENGINE_free() does the cleanup after ENGINE_by_id(), and
> ENGINE_finish() cleans up after ENGINE_init():
> https://www.openssl.org/docs/man1.1.0/man3/ENGINE_finish.html

Yeah, that reference page pretty definitely agrees with what
we're doing.

> And an actual issue is that we have no coverage for it:
> https://coverage.postgresql.org/src/interfaces/libpq/fe-secure-openssl.c.gcov.html

Oh, hmm ... I'd supposed that the code in question was exercised
in normal SSL connections, but now I see it's not so.  It looks
like you need a non-default SSL "engine" to be available??  Might
be hard to test this as a routine thing if it requires additional
software.

            regards, tom lane