be-secure.c and SSL/TLS - Mailing list pgsql-bugs

I believe fe-secure.c has a few opportunities for improvement. A lot
of these are similar to the client code in fe-secure.c.

I believe the first eight are features requests/improvements, but the
last three could be a security vulnerabilities.

**********

Around line 140, there's a 512-bit modulus. That's a tad bit small for
2013 because it only offers about 64-bits of security.

    static const char file_dh512[] =
    "-----BEGIN DH PARAMETERS-----\n\
    MEYCQQD1Kv884bEpQBgRjXyEpwpy1obEAxnIByl6ypUM2Zafq9AKUJsCRtMIPWak\n\
    XUGfnHy9iUsiGSa6q6Jew1XpKgVfAgEC\n\
    -----END DH PARAMETERS-----\n";

I don't think anyone would miss it if it went away.

**********

If Postgres is supporting US Federal, then the 1024-bit modulus should
go away, too. Or it should be configurable via pg_hba.conf. (SP800-57
requires 2048-bit or above, even though it takes 10x the processing
power over 1024-bit).

**********

`initialize_SSL` starts around line 725. `SSL_CTX_set_options` is
called around line 795, but it only bans SSLv3 with SSL_OP_NO_SSLv2.
In 2013, SSL_OP_NO_SSLv3 is appropriate, too.

SSL_OP_NO_COMPRESSION might also be a good choice.

**********

Around line 795, the following is called.

    /* set up the allowed cipher list */
    if (SSL_CTX_set_cipher_list(SSL_context, SSLCipherSuites) != 1)

But I don't see where SSLCipherSuites is set to anything other than
NULL. (Unfortunately, I don't know the behavior when using NULL).

**********

You can trim the ciphers being used (and perhaps improve the security)
by providing a cipher list similar to the following. It will also
remove weak/wounded/unneeded ciphers.

Removing the unneeded ciphers cuts the list down from 50+ to fewer
than 20. Also, you should be OK with DSS and DSA because you call
SSL_CTX_set_tmp_dh_callback.

const char* PREFERRED_CIPHERS =

    /* TLS 1.2 only */
    "ECDHE-ECDSA-AES256-GCM-
SHA384:"
    "ECDHE-RSA-AES256-GCM-SHA384:"
    "ECDHE-ECDSA-AES128-GCM-SHA256:"
    "ECDHE-RSA-AES128-GCM-SHA256:"

    /* TLS 1.2 only */
    "DHE-DSS-AES256-GCM-SHA384:"
    "DHE-RSA-AES256-GCM-SHA384:"
    "DHE-DSS-AES128-GCM-SHA256:"
    "DHE-RSA-AES128-GCM-SHA256:"

    /* TLS 1.0 only */
    "DHE-DSS-AES256-SHA:"
    "DHE-RSA-AES256-SHA:"
    "DHE-DSS-AES128-SHA:"
    "DHE-RSA-AES128-SHA:"

    /* SSL 3.0 and TLS 1.0 */
    "EDH-DSS-DES-CBC3-SHA:"
    "EDH-RSA-DES-CBC3-SHA:"
    "DH-DSS-DES-CBC3-SHA:"
    "DH-RSA-DES-CBC3-SHA";

    res = SSL_set_cipher_list(ssl, PREFERRED_CIPHERS);
    if(res != 1)
        /* handle error */

Even though the last four cipher suites are used for SSL and TLS,
SSL_OP_NO_SSLv3 will ensure they are used with only TLS v1.0 or above.

**********

Around line 810, a CRL is loaded unconditionally. I think there might
be an opportunity for improvement. If client certs are not in use,
then there's no reason to load the CRL (the server would stop using
the revoked cert). Perhaps that loading should be deferred until
pg_hba.conf is parsed.

**********

At line 857, a flag is set:

        /* Set flag to remember CA store is successfully loaded */
        ssl_loaded_verify_locations = true;

It seems like the value of the flag should depend upon
`X509_STORE_load_locations` and `X509_STORE_set_flags`.
`X509_STORE_set_flags` calls `X509_VERIFY_PARAM_set_flags`.
`X509_VERIFY_PARAM_set_flags` returns a [useless] result (and not the
old flags), so 1 is good and otherwise is bad.

**********

Postgres takes care to ensure cipher suites with perfect forward
secrecy are used. Its a great effort and sorely needed.

However, session renegotiation breaks PFS. Around line 330:

    if (SSL_renegotiate(port->ssl) <= 0)
        ereport(COMMERROR,
            (errcode(ERRCODE_PROTOCOL_VIOLATION),
            errmsg("SSL renegotiation failure")));

It breaks PFS because the premaster secret and session_id are retained
for future renegotiations. If the server is operating in a farm *and*
in-memory IPC is not available, then it gets written to disk, too.

**********

If client certificates are in use, then the logic around line 950
kicks in. The client's certificate is extracted at line 955, and the
subject's commnName is extracted. So far, so good.

If an adversary sends a malformed certificate such that
X509_NAME_get_text_by_NID fails, then the server will accept the
connection by dropping into the 'return 0` around line 1000. That is,
it will bypass the logic that's used to return failures.

**********

If certificates are in use, then the server is performing the
customary checks on the client certificate. As with a client, the
server must call SSL_get_verify_result to get the result of the
customary checks performed by the OpenSSL library. I don't see where
that's being called on the server side.

You can see an example of what OpenSSL uses in its own source code by
examining s_server.c around lines 2420. It call
`SSL_get_verify_result` looking for X509_V_OK, and then calls
`SSL_get_peer_certificate` to dump the user's information.