[PATCH] add ssl_protocols configuration option

[Dag-Erling Smørgrav]

The attached patches add an ssl_protocols configuration option which
control which versions of SSL or TLS the server will use. The syntax is
similar to Apache's SSLProtocols directive, except that the list is
colon-separated instead of whitespace-separated, although that is easy
to change if it proves unpopular.

Summary of the patch:

- In src/backend/libpq/be-secure.c:
- Add an SSLProtocols variable for the option.
- Add a function, parse_SSL_protocols(), that parses an ssl_protocols
string and returns a bitmask suitable for SSL_CTX_set_options().
- Change initialize_SSL() to call parse_SSL_protocols() and pass the
result to SSL_CTX_set_options().
- In src/backend/utils/misc/guc.c:
- Add an extern declaration for SSLProtocols.
- Add an entry in the ConfigureNamesString array for the
ssl_protocols option.
- In src/backend/utils/misc/postgresql.conf.sample:
- Add a sample ssl_protocols line.
- In doc/src/sgml/config.sgml:
- Document the ssl_protocols option.

The file names are slightly different in 9.5, since be-secure.c was
split in two and the declaration was moved into libpq.h.

The default is "ALL:-SSLv2" in 9.0-9.3 and "ALL:-SSL" in 9.4 and up.
This corresponds to the current hardcoded values, so the default
behavior is unchanged, but the admin now has the option to select a
different settings, e.g. if a serious vulnerability is found in TLS 1.0.

[Michael Paquier]

Post by Dag-Erling Smørgrav
The default is "ALL:-SSLv2" in 9.0-9.3 and "ALL:-SSL" in 9.4 and up.
This corresponds to the current hardcoded values, so the default
behavior is unchanged, but the admin now has the option to select a
different settings, e.g. if a serious vulnerability is found in TLS 1.0.

Please note that new features can only be added to the version currently in
development, aka 9.5. You should as well register your patch to the current
commit fest, I think you are still in time:
https://commitfest.postgresql.org/action/commitfest_view?id=24

[Dag-Erling Smørgrav]

Post by Michael Paquier
Please note that new features can only be added to the version
currently in development, aka 9.5.

I understand this policy. However, this new feature a) has absolutely
no impact unless the admin makes a conscious decision to use it and b)
will make life much easier for everyone if a POODLE-like vulnerability
is discovered in TLS.

Post by Michael Paquier
You should as well register your patch to the current commit fest, I
https://commitfest.postgresql.org/action/commitfest_view?id=24

Thanks for reminding me.

DES

[Alvaro Herrera]

Post by Dag-Erling Smørgrav

Post by Michael Paquier
Please note that new features can only be added to the version
currently in development, aka 9.5.

I understand this policy. However, this new feature a) has absolutely
no impact unless the admin makes a conscious decision to use it and b)
will make life much easier for everyone if a POODLE-like vulnerability
is discovered in TLS.

I see this as vaguely related to
http://www.postgresql.org/message-id/***@eldon.alvh.no-ip.org
where we want to have SSL behavior configurable in the back branches due
to renegotiation issues: there was talk in that thread about introducing
new GUC variables in back branches to control the behavior. The current
patch really doesn't add much in the way of features (SSLv3 support and
so on already exist in back branches) --- what it does add is a way to
control whether these are used.

[Tom Lane]

Post by Alvaro Herrera

Post by Dag-Erling Smørgrav
I understand this policy. However, this new feature a) has absolutely
no impact unless the admin makes a conscious decision to use it and b)
will make life much easier for everyone if a POODLE-like vulnerability
is discovered in TLS.

I see this as vaguely related to
where we want to have SSL behavior configurable in the back branches due
to renegotiation issues: there was talk in that thread about introducing
new GUC variables in back branches to control the behavior. The current
patch really doesn't add much in the way of features (SSLv3 support and
so on already exist in back branches) --- what it does add is a way to
control whether these are used.

This looks to me like re-fighting the last war. Such a GUC has zero value
*unless* some situation exactly like the POODLE bug comes up again, and
the odds of that are not high.

Moreover, the GUC could easily be misused to decrease rather than increase
one's security, if it's carelessly set. Remember that we only recently
fixed bugs that prevented use of the latest TLS version. If we have a
setting like this, I fully anticipate that people will set it to "only use
TLS 1.2" and then forget that they ever did that; years from now they'll
still be using 1.2 when it's deprecated.

So I think the argument that this is a useful security contribution is
pretty weak; and on the whole we don't need another marginally-useful GUC.

regards, tom lane

[Magnus Hagander]

Post by Tom Lane

Post by Alvaro Herrera

Post by Dag-Erling Smørgrav
I understand this policy. However, this new feature a) has absolutely
no impact unless the admin makes a conscious decision to use it and b)
will make life much easier for everyone if a POODLE-like vulnerability
is discovered in TLS.

I see this as vaguely related to
where we want to have SSL behavior configurable in the back branches due
to renegotiation issues: there was talk in that thread about introducing
new GUC variables in back branches to control the behavior. The current
patch really doesn't add much in the way of features (SSLv3 support and
so on already exist in back branches) --- what it does add is a way to
control whether these are used.

This looks to me like re-fighting the last war. Such a GUC has zero value
*unless* some situation exactly like the POODLE bug comes up again, and
the odds of that are not high.
Moreover, the GUC could easily be misused to decrease rather than increase
one's security, if it's carelessly set. Remember that we only recently
fixed bugs that prevented use of the latest TLS version. If we have a
setting like this, I fully anticipate that people will set it to "only use
TLS 1.2" and then forget that they ever did that; years from now they'll
still be using 1.2 when it's deprecated.

If anything, I think the default should be "default", and then we have
that map out to something. Because once you've initdb'ed, the config
file wil be stuck with a default and we can't change that in a minor
release *if* something like POODLE shows up again. It would require an
admin action, and in this case, it would make us less secure. If we
had a GUC that took the keyword "default" which would mean "whatever
the current version of postgresql thinks is the best" then we could
change the default in a security update if something showed up.

In a case like POODLE we probably wouldn't have done it anyway, as it
doesn't affect our connections (we're not http) and it would have the
potential of breaking some third party clients. However, if something
really bad showed up, we might want to do that.

Post by Tom Lane
So I think the argument that this is a useful security contribution is
pretty weak; and on the whole we don't need another marginally-useful GUC.

Having the guc could certainly be useful in some cases. If we do, we
should of course *also* have a corresponding configuration option in
libpq, so I'd say this patch is incomplete if we do want to do it.
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

[Tom Lane]

Post by Magnus Hagander
If anything, I think the default should be "default", and then we have
that map out to something. Because once you've initdb'ed, the config
file wil be stuck with a default and we can't change that in a minor
release *if* something like POODLE shows up again. It would require an
admin action, and in this case, it would make us less secure. If we
had a GUC that took the keyword "default" which would mean "whatever
the current version of postgresql thinks is the best" then we could
change the default in a security update if something showed up.

That's pretty much isomorphic to just setting the value in the code,
no?

Post by Magnus Hagander
Having the guc could certainly be useful in some cases. If we do, we
should of course *also* have a corresponding configuration option in
libpq, so I'd say this patch is incomplete if we do want to do it.

True. But both of those scenarios posit that no *code* changes are
needed, which is infrequently the case.

And you still have the problem that if an admin does change the setting
away from "default", and forgets to revert that after his next update,
he could in the long run be less secure not more so. Client-side
settings would likely be even harder to get rid of than server-side.

And in the end, if we set values like this from PG --- whether
hard-wired or via a GUC --- the SSL library people will have exactly
the same perspective with regards to *our* values. And not without
reason; we were forcing very obsolete settings up till recently,
because nobody had looked at the issue for a decade. I see no reason
to expect that that history won't repeat itself.

regards, tom lane

[Magnus Hagander]

Post by Tom Lane

Post by Magnus Hagander
If anything, I think the default should be "default", and then we have
that map out to something. Because once you've initdb'ed, the config
file wil be stuck with a default and we can't change that in a minor
release *if* something like POODLE shows up again. It would require an
admin action, and in this case, it would make us less secure. If we
had a GUC that took the keyword "default" which would mean "whatever
the current version of postgresql thinks is the best" then we could
change the default in a security update if something showed up.

That's pretty much isomorphic to just setting the value in the code,
no?

No, it would let the user (temporarily) override it.

Post by Tom Lane

Post by Magnus Hagander
Having the guc could certainly be useful in some cases. If we do, we
should of course *also* have a corresponding configuration option in
libpq, so I'd say this patch is incomplete if we do want to do it.

True. But both of those scenarios posit that no *code* changes are
needed, which is infrequently the case.

Definitely - it's still very borderline if it's useful.

Post by Tom Lane
And you still have the problem that if an admin does change the setting
away from "default", and forgets to revert that after his next update,
he could in the long run be less secure not more so. Client-side
settings would likely be even harder to get rid of than server-side.

True.

Post by Tom Lane
And in the end, if we set values like this from PG --- whether
hard-wired or via a GUC --- the SSL library people will have exactly
the same perspective with regards to *our* values. And not without
reason; we were forcing very obsolete settings up till recently,
because nobody had looked at the issue for a decade. I see no reason
to expect that that history won't repeat itself.

The best part would be if we could just leave it up to the SSL
library, but at least the openssl one doesn't have an API that lets us
do that, right? We *have* to pick something...
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

[Tom Lane]

Post by Magnus Hagander

Post by Tom Lane
And in the end, if we set values like this from PG --- whether
hard-wired or via a GUC --- the SSL library people will have exactly
the same perspective with regards to *our* values. And not without
reason; we were forcing very obsolete settings up till recently,
because nobody had looked at the issue for a decade. I see no reason
to expect that that history won't repeat itself.

The best part would be if we could just leave it up to the SSL
library, but at least the openssl one doesn't have an API that lets us
do that, right? We *have* to pick something...

As far as protocol version goes, I think our existing coding basically
says "prefer newest available version, but at least TLS 1.0". I think
that's probably a reasonable approach.

If the patch exposed a GUC that set a "minimum" version, rather than
calling out specific acceptable protocols, it might be less risky.

regards, tom lane

[Magnus Hagander]

Post by Tom Lane

Post by Magnus Hagander

Post by Tom Lane
And in the end, if we set values like this from PG --- whether
hard-wired or via a GUC --- the SSL library people will have exactly
the same perspective with regards to *our* values. And not without
reason; we were forcing very obsolete settings up till recently,
because nobody had looked at the issue for a decade. I see no reason
to expect that that history won't repeat itself.

The best part would be if we could just leave it up to the SSL
library, but at least the openssl one doesn't have an API that lets us
do that, right? We *have* to pick something...

As far as protocol version goes, I think our existing coding basically
says "prefer newest available version, but at least TLS 1.0". I think
that's probably a reasonable approach.

Yes, it does that. Though it only does it on 9.4,but with the facts we know
now, what 9.4+ does is perfectly safe.

/Magnus

[Dag-Erling Smørgrav]

Post by Magnus Hagander
Yes, it does that. Though it only does it on 9.4,but with the facts we
know now, what 9.4+ does is perfectly safe.

Agreed.

DES

[Dag-Erling Smørgrav]

Post by Tom Lane
As far as protocol version goes, I think our existing coding basically
says "prefer newest available version, but at least TLS 1.0". I think
that's probably a reasonable approach.

The client side forces TLS 1.0:

SSL_context = SSL_CTX_new(TLSv1_method());

In typical OpenSSL fashion, this does *not* mean 1.0 or higher. It
means 1.0 exactly.

Post by Tom Lane
If the patch exposed a GUC that set a "minimum" version, rather than
calling out specific acceptable protocols, it might be less risky.

Not necessarily. Someone might find a weakness in TLS 1.1 which is not
present in 1.0 because it involves a specific algorithm or mode that 1.0
does not support.

DES

[Dag-Erling Smørgrav]

Post by Tom Lane
And in the end, if we set values like this from PG --- whether
hard-wired or via a GUC --- the SSL library people will have exactly
the same perspective with regards to *our* values. And not without
reason; we were forcing very obsolete settings up till recently,
because nobody had looked at the issue for a decade. I see no reason
to expect that that history won't repeat itself.

I'm not sure what you're saying here, but - I'm not sure how familiar
you are with the OpenSSL API, but it's insecure by default. There is
*no other choice* for an application than to explicitly select which
protocols it wants to use (or at least which protocols it wants to
avoid). And you can't change OpenSSL, because a ton of old crappy
software is going to break.

DES

[Dag-Erling Smørgrav]

Post by Magnus Hagander
If anything, I think the default should be "default", and then we have
that map out to something. Because once you've initdb'ed, the config
file wil be stuck with a default and we can't change that in a minor
release *if* something like POODLE shows up again.

Actually, I had that in an earlier version of the patch, but I thought
it was too obscure / circular. I can easily re-add it.

Post by Magnus Hagander
In a case like POODLE we probably wouldn't have done it anyway, as it
doesn't affect our connections (we're not http)

If I understand correctly, imaps has been shown to be vulnerable as
well, so I wouldn't be so sure.

Post by Magnus Hagander
Having the guc could certainly be useful in some cases. If we do, we
should of course *also* have a corresponding configuration option in
libpq, so I'd say this patch is incomplete if we do want to do it.

I can update the patch to include the client side.

DES

[Martijn van Oosterhout]

Post by Dag-Erling Smørgrav

Post by Magnus Hagander
In a case like POODLE we probably wouldn't have done it anyway, as it
doesn't affect our connections (we're not http)

If I understand correctly, imaps has been shown to be vulnerable as
well, so I wouldn't be so sure.

Reference? It's an SSL3 specific attack, so most servers are not
vulnerable because TLS will negotiate to the highest supported
protocol. So unless one of the client/server doesn't support TLS1.0
there's no issue. The only reason http is vulnerable is because
browsers do protocol downgrading, something strictly forbidden by the
spec.

Additionally, the man-in-the-middle must be able to control the padding
in the startup packet, which just isn't possible (no scripting language
in the client).

Since you can already specify the cipher list, couldn't you just add
-SSLv3 to the cipher list and be done?

Have a nice day,

[Dag-Erling Smørgrav]

Post by Martijn van Oosterhout

Post by Dag-Erling Smørgrav
If I understand correctly, imaps has been shown to be vulnerable as
well, so I wouldn't be so sure.

Reference?

Sorry, no reference. I was told that Thunderbird was vulnerable to
POODLE when talking imaps.

Post by Martijn van Oosterhout
Since you can already specify the cipher list, couldn't you just add
-SSLv3 to the cipher list and be done?

I didn't want to change the existing behavior; all I wanted was to give
users a way to do so if they wish.

DES

[Martijn van Oosterhout]

Post by Dag-Erling Smørgrav

Post by Martijn van Oosterhout

Post by Dag-Erling Smørgrav
If I understand correctly, imaps has been shown to be vulnerable as
well, so I wouldn't be so sure.

Reference?

Sorry, no reference. I was told that Thunderbird was vulnerable to
POODLE when talking imaps.

Ugh, found it. It does the same connection fallback stuff as firefox.

https://securityblog.redhat.com/2014/10/20/can-ssl-3-0-be-fixed-an-analysis-of-the-poodle-attack/

Post by Dag-Erling Smørgrav

Post by Martijn van Oosterhout
Since you can already specify the cipher list, couldn't you just add
-SSLv3 to the cipher list and be done?

I didn't want to change the existing behavior; all I wanted was to give
users a way to do so if they wish.

I think we should just disable SSL3.0 altogether. The only way this
could cause problems is if people are using PostgreSQL with an OpenSSL
library from last century. As for client libraries, even Windows XP
supports TLS1.0.

Have a nice day,

[Dag-Erling Smørgrav]

Post by Martijn van Oosterhout

Post by Dag-Erling Smørgrav

Post by Martijn van Oosterhout
Since you can already specify the cipher list, couldn't you just
add -SSLv3 to the cipher list and be done?

I didn't want to change the existing behavior; all I wanted was to
give users a way to do so if they wish.

I think we should just disable SSL3.0 altogether. The only way this
could cause problems is if people are using PostgreSQL with an OpenSSL
library from last century. As for client libraries, even Windows XP
supports TLS1.0.

As far as I'm concerned (i.e. as far as FreeBSD and the University of
Oslo are concerned), I couldn't care less about anything older than
0.9.8, which is what FreeBSD 8 and RHEL5 have, but I don't feel
comfortable making that decision for other people. On the gripping
hand, no currently supported version of libpq uses anything older than
TLS; 9.0 through 9.3 use TLS 1.0 only while 9.4 uses TLS 1.0 or higher.

DES

[Alvaro Herrera]

Post by Dag-Erling Smørgrav

Post by Martijn van Oosterhout

Post by Dag-Erling Smørgrav

Post by Martijn van Oosterhout
Since you can already specify the cipher list, couldn't you just
add -SSLv3 to the cipher list and be done?

I didn't want to change the existing behavior; all I wanted was to
give users a way to do so if they wish.

I think we should just disable SSL3.0 altogether. The only way this
could cause problems is if people are using PostgreSQL with an OpenSSL
library from last century. As for client libraries, even Windows XP
supports TLS1.0.

As far as I'm concerned (i.e. as far as FreeBSD and the University of
Oslo are concerned), I couldn't care less about anything older than
0.9.8, which is what FreeBSD 8 and RHEL5 have, but I don't feel
comfortable making that decision for other people. On the gripping
hand, no currently supported version of libpq uses anything older than
TLS; 9.0 through 9.3 use TLS 1.0 only while 9.4 uses TLS 1.0 or higher.

OpenSSL just announced a week or two ago that they're abandoning support
for 0.9.8 by the end of next year[1], which means its replacements have
been around for a really long time. I think it's fine to drop 0.9.7
support --- we already dropped support for 0.9.6 with the renegotiation
rework[2] in the 9.4 timeframe.

OpenSSL 0.9.7 has already not gotten fixes for all the latest flurry of
security issues, so anyone *is* using SSL but not at least the 0.9.8
branch, they are in trouble.

[1] http://openssl.6102.n7.nabble.com/OpenSSL-0-9-8-End-Of-Life-Announcement-td54155.html
[2] http://www.postgresql.org/message-id/***@eldon.alvh.no-ip.org

[Dag-Erling Smørgrav]

Post by Alvaro Herrera
OpenSSL just announced a week or two ago that they're abandoning support
for 0.9.8 by the end of next year[1], which means its replacements have
been around for a really long time.

RHEL5 still has 0.9.8e with backported patches and will be supported
until 2017-03-31.

FreeBSD 8.4, 9.1, 9.2 and 9.3 all have 0.9.8y with backported patches.
8.4, 9.1 and 9.2 all expire before OpenSSL 0.9.8, but 9.3 will be
supported until 2016-12-31.

0.9.8 and 1.0.1 are not binary compatible, so upgrading is *not* an
option. We (as in FreeBSD) will have to make do - either develop our
own patches or adapt RedHat's.

Post by Alvaro Herrera
OpenSSL 0.9.7 has already not gotten fixes for all the latest flurry of
security issues, so anyone *is* using SSL but not at least the 0.9.8
branch, they are in trouble.

The latest 0.9.8 still only has TLS 1.0, unless they're planning to
backport 1.1 and 1.2 (which I seriously doubt).

DES

[Tom Lane]

Post by Dag-Erling Smørgrav

Post by Alvaro Herrera
OpenSSL 0.9.7 has already not gotten fixes for all the latest flurry of
security issues, so anyone *is* using SSL but not at least the 0.9.8
branch, they are in trouble.

The latest 0.9.8 still only has TLS 1.0, unless they're planning to
backport 1.1 and 1.2 (which I seriously doubt).

The upshot of this conversation still seems to be that we don't need to
do anything. Unless I'm misunderstanding something:

(1) No currently supported (or even recently supported) version of either
the backend or libpq will select protocol less than TLS 1.0 unless forced
to via (poorly chosen) configuration settings.

(2) Anyone who is feeling paranoid about shutting off SSLv3 despite (1)
can do so via the existing ssl_ciphers GUC parameter.

Seems to me that's sufficient, not only for now but for the future;
existing OpenSSL practice is that the ciphers string includes categories
corresponding to protocol versions, so you can shut off an old
protocol version there if you need to.

regards, tom lane

[Dag-Erling Smørgrav]

Post by Tom Lane
Anyone who is feeling paranoid about shutting off SSLv3 despite (1)
can do so via the existing ssl_ciphers GUC parameter [...] the ciphers
string includes categories corresponding to protocol versions, so you
can shut off an old protocol version there if you need to.

The overlap between SSL 3.0 and TLS 1.0 is 100%:

% openssl ciphers SSLv2 | md5
fe5ff23432f119364a1126ca0776c5db
% openssl ciphers SSLv3 | md5
bde4e4a10b9c3f323c0632ad067e293a
% openssl ciphers TLSv1 | md5
bde4e4a10b9c3f323c0632ad067e293a
% openssl ciphers TLSv1.2 | md5
26c375b6bdefb018b9dd7df463658320

Thus, if you disable all SSL 3.0 ciphers, you also disable TLS 1.0.

DES

[Dag-Erling Smørgrav]

Post by Tom Lane
This looks to me like re-fighting the last war. Such a GUC has zero value
*unless* some situation exactly like the POODLE bug comes up again, and
the odds of that are not high.

Many people would have said the exact same thing before POODLE, and
BEAST, and CRIME, and Heartbleed. You never know what sort of bugs or
weaknesses will show up or when; all you know is that there are a lot of
people working very hard to find these things and exploit them, and that
they *will* succeeded, again and again and again. You can gamble that
PostgreSQL will not be vulnerable due to specific details of its
protocol or how it uses TLS, but that's a gamble which you will
eventually lose.

Post by Tom Lane
Moreover, the GUC could easily be misused to decrease rather than increase
one's security, if it's carelessly set.

That's the user's responsibility.

DES

[Michael Paquier]

Post by Dag-Erling Smørgrav

Post by Tom Lane
This looks to me like re-fighting the last war. Such a GUC has zero

value

Post by Tom Lane
*unless* some situation exactly like the POODLE bug comes up again, and
the odds of that are not high.

Many people would have said the exact same thing before POODLE, and
BEAST, and CRIME, and Heartbleed. You never know what sort of bugs or
weaknesses will show up or when; all you know is that there are a lot of
people working very hard to find these things and exploit them, and that
they *will* succeeded, again and again and again. You can gamble that
PostgreSQL will not be vulnerable due to specific details of its
protocol or how it uses TLS, but that's a gamble which you will
eventually lose.

There are some companies, without naming them, that have increased the
resources dedicated to analyze existing security protocols and libraries,
so even if the chances are low, IMO the occurence that similar problems
show up are getting to increase wit the time.

Post by Dag-Erling Smørgrav

Post by Tom Lane
Moreover, the GUC could easily be misused to decrease rather than

increase

Post by Tom Lane
one's security, if it's carelessly set.

That's the user's responsibility.

I actually just had a user knocking about having a way to control the
protocols used by server... So, changing my opinion on the matter, that
would be nice to have even such a parameter on back-branches, with
'default' to let the server decide which one is better.