Re: fe-secure.c and SSL/TLS

I believe fe-secure.c has a few opportunities for improvement. I
believe the first three are features requests/improvements, but the
fourth and fifth could be a security vulnerabilities.

*****

`initialize_SSL` uses the following around line 970 (which is much
better than most):

SSL_context = SSL_CTX_new(TLSv1_method());

The TLSv1_method locks selection to TLS 1.0. Its good because SSL v2
and v3 are removed; but it could be improved upon to make TLS 1.1 and
1.2 available.

The attached shows a sample connection under Wireshark that uses
TLSv1_method. Notice the Handshake Protocol (not TLSv1 Record)
advertises 1.0 (0x301) as the highest level.

Unfortunately, OpenSSL does not provide a TLSv1_and_above_method. I
believe a better way to do things would be:

SSL_context = SSL_CTX_new(SSLv23_method());
flags = SSL_OP_ALL | SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3;
SSL_CTX_set_options(SSL_context, flags);

SSL_OP_NO_COMPRESSION might also be a good choice.

That will set the minimum to TLS 1.0, and the maximum to TLS 1.2. It
also prefers ciphers like ECDHE-ECDSA-AES256-GCM-SHA384 (the order is
built into OpenSSL).

*****

If you take the opportunity above, then you can trim the ciphers being
used (and improve the security) by providing similar to the following.
It will also remove weak/wounded/unneeded ciphers. (I prefer GCM over
both CBC and RC4. If I have to use one or the other, I think CBC is
the lesser of the evils).

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:"

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

**********

`pqsecure_open_client` makes the connection to the server. I believe
Server Name Indicator (SNI) could be used to take advantage of the TLS
1.0 feature:

SSL_set_tlsext_host_name(SSL_context, HOST_NAME);

**********

There are three checks needed to verify a SSL/TLS connection under
OpenSSL. I see two of them, but I don't see the third.

The three checks are:

(1) non-NULL server certifcate
(2) SSL_get_verify_result must return X509_V_OK
(3) hostname must be verified

Items (1) and (3) are present in `open_client_SSL` around line 1500.
(By the way, OpenSSL 1.1.0 will offer to perform the hostname checks -
its in HEAD now).

Item (2) fetches the result from OpenSSL performing the customary
checks like chaining to a valid root and Not_Before/Not_After. If the
verify_callback always return 1, then SSL_get_verify_result will
always return X509_V_OK. If the verify_callback returns a 0, then
SSL_get_verify_result will *not* return X509_V_OK.

I believe this is a vulnerability. A rogue server can present a forged
a certificate that does not forma a valid chain (i.e., not chained to
the root specififed by SSL_CTX_load_verify_locations), and Postgres
will accept it as genuine as long as the name matches because
SSL_get_verify_result is not called.

**********

Looking at the rules in `wildcard_certificate_match` around line 720,
there might be a problem with ccTLDs. Mozilla maintains a list of them
at http://publicsuffix.org/ (alt,
https://mxr.mozilla.org/mozilla-central/source/netwerk/dns/effective_tld_names.dat?raw=1).

The potential problem is a rogue server could present a wildcard
certificate for a ccTLD like '*.com.eu' and the code might match if
some conditions are met. I believe the conditions include tricking the
client into using, for example, 'example.com.eu'. I don't believe its
easy to exploit, but the potential looks like its there.

On 11/12/13, 11:49 PM, Jeffrey Walton wrote:
> I believe fe-secure.c has a few opportunities for improvement. I
> believe the first three are features requests/improvements, but the
> fourth and fifth could be a security vulnerabilities.

Please create patches and send them to the next commit fest. Also note
that the current commit fest contains a few SSL-related patches, which
might overlap with your suggestions:
https://commitfest.postgresql.org/action/commitfest_view?id=20

Thanks Peter.

On Fri, Nov 22, 2013 at 8:22 AM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> On 11/12/13, 11:49 PM, Jeffrey Walton wrote:
>> I believe fe-secure.c has a few opportunities for improvement. I
>> believe the first three are features requests/improvements, but the
>> fourth and fifth could be a security vulnerabilities.
>
> Please create patches and send them to the next commit fest.
That is a pretty cool concept.

> that the current commit fest contains a few SSL-related patches, which
> might overlap with your suggestions:
> https://commitfest.postgresql.org/action/commitfest_view?id=20
I kind of disagree with this from
http://www.postgresql.org/message-id/20131114231105.GA23669@gmail.com:

Main goal is to leave low-level ciphersuite details to
OpenSSL guys and give clear impression to Postgres
admins what it is about.

I would argue nothing should be left to chance, and the project should
take control of everything. But I don't really have a dog in the fight
;)

From this comment at
http://www.postgresql.org/message-id/20131114231105.GA23669@gmail.com:

!aNULL
Needed to disable suites that do not authenticate
server. DEFAULT includes !aNULL by default.

If server authentication is desired, then SSL_get_verify_result should
be called in addition to the name checks when in an enterprise
environment (i.e., a CAfile was provided) or the client knows who to
trust (by whatever means).

Ommiting SSL_get_verify_result basically results in an ADH-like
protocol :) Its OK for opportunistic encryption, but its not OK for an
enterprise deployment running a private PKI or the client knows who to
trust.

Also, what about eNULL? Is it OK to send authenticated plain text
(that's what the eNULL:!aNULL combination provides).

Jeff