Thread: Should we back-patch SSL renegotiation fixes?

Those of you who have been following
http://www.postgresql.org/message-id/flat/1d3bc192-970d-4b70-a5fe-38d2a9f762b3@me.com
are aware that Red Hat shipped a rather broken version of openssl last
week.  While waiting for them to fix it, I've been poking at the behavior,
and have found out that PG 9.4 and later are much less badly broken than
older branches.  In the newer branches you'll see a failure only after
transmitting 2GB within a session, whereas the older branches fail at
the second renegotiation attempt, which would typically be 1GB of data
and could be a lot less.

I do not know at this point whether these behaviors are really the same
bug or not, but I wonder whether it's time to consider back-patching the
renegotiation fixes we did in 9.4.  Specifically, I think maybe we should
back-patch 31cf1a1a4, 86029b31e, and 36a3be654.  (There are more changes
in master, but since those haven't yet shipped in any released branch,
and there's been a lot of other rework in the same area, those probably
are not back-patch candidates.)

Thoughts?
        regards, tom lane

On Tue, Jun 23, 2015 at 2:33 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Those of you who have been following
> http://www.postgresql.org/message-id/flat/1d3bc192-970d-4b70-a5fe-38d2a9f762b3@me.com
> are aware that Red Hat shipped a rather broken version of openssl last
> week.  While waiting for them to fix it, I've been poking at the behavior,
> and have found out that PG 9.4 and later are much less badly broken than
> older branches.  In the newer branches you'll see a failure only after
> transmitting 2GB within a session, whereas the older branches fail at
> the second renegotiation attempt, which would typically be 1GB of data
> and could be a lot less.
>
> I do not know at this point whether these behaviors are really the same
> bug or not, but I wonder whether it's time to consider back-patching the
> renegotiation fixes we did in 9.4.  Specifically, I think maybe we should
> back-patch 31cf1a1a4, 86029b31e, and 36a3be654.  (There are more changes
> in master, but since those haven't yet shipped in any released branch,
> and there's been a lot of other rework in the same area, those probably
> are not back-patch candidates.)
>
> Thoughts?

I have no clear idea how safe it is to back-port these fixes.

Just as a point of reference, we had a customer hit a problem similar
to bug #12769 on 9.3.x.  I think (but am not sure) that 272923a0a may
have been intended to fix that issue.  In a quick search, I didn't
find any other complaints about renegotiation-related issues from our
customers.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Jun 23, 2015 at 2:33 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I do not know at this point whether these behaviors are really the same
>> bug or not, but I wonder whether it's time to consider back-patching the
>> renegotiation fixes we did in 9.4.  Specifically, I think maybe we should
>> back-patch 31cf1a1a4, 86029b31e, and 36a3be654.  (There are more changes
>> in master, but since those haven't yet shipped in any released branch,
>> and there's been a lot of other rework in the same area, those probably
>> are not back-patch candidates.)
>> 
>> Thoughts?

> I have no clear idea how safe it is to back-port these fixes.

Well, it would mean that pre-9.5 branches all behave the same, which
would be an improvement in my book.  Also, ISTM that the 9.4 code
for renegotiation assumes a whole lot less than prior branches about
OpenSSL's internal behavior; so it ought to be more robust, even if
some bugs remain.

> Just as a point of reference, we had a customer hit a problem similar
> to bug #12769 on 9.3.x.  I think (but am not sure) that 272923a0a may
> have been intended to fix that issue.  In a quick search, I didn't
> find any other complaints about renegotiation-related issues from our
> customers.

The problem with trying to adopt code from HEAD is that it probably
depends on the rather invasive changes explained here:
http://www.postgresql.org/message-id/20150126101405.GA31719@awork2.anarazel.de
Even assuming that there's no dependency on the immediate-interrupt
changes, I'm afraid to back-patch anything that invasive.
        regards, tom lane

Robert Haas wrote:
> On Tue, Jun 23, 2015 at 2:33 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> > I do not know at this point whether these behaviors are really the same
> > bug or not, but I wonder whether it's time to consider back-patching the
> > renegotiation fixes we did in 9.4.  Specifically, I think maybe we should
> > back-patch 31cf1a1a4, 86029b31e, and 36a3be654.  (There are more changes
> > in master, but since those haven't yet shipped in any released branch,
> > and there's been a lot of other rework in the same area, those probably
> > are not back-patch candidates.)

Yes, +1 for backpatching.  Don't forget 5674460b and b1aebbb6.

I could reproduce problems trivially with COPY in psql without that and
a small renegotiation limit, as I recall.

> > Thoughts?
> 
> I have no clear idea how safe it is to back-port these fixes.
> 
> Just as a point of reference, we had a customer hit a problem similar
> to bug #12769 on 9.3.x.  I think (but am not sure) that 272923a0a may
> have been intended to fix that issue.

Maybe we should *also* backpatch that, then (and if so, then also its
fixup 1c2b7c087).  I do not think that the failure was introduced by
the fixes cited above.

> In a quick search, I didn't find any other complaints about
> renegotiation-related issues from our customers.

Other issues Andres was investigating seemed related to nonblocking
connections (which as I recall are used mostly by replication code).
Bug #12769 contained a link to the previous discussion.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
>> On Tue, Jun 23, 2015 at 2:33 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> I do not know at this point whether these behaviors are really the same
>>> bug or not, but I wonder whether it's time to consider back-patching the
>>> renegotiation fixes we did in 9.4.  Specifically, I think maybe we should
>>> back-patch 31cf1a1a4, 86029b31e, and 36a3be654.

> Yes, +1 for backpatching.  Don't forget 5674460b and b1aebbb6.

Huh?  5674460b is ancient, and we concluded that b1aebbb6 didn't represent
anything much more than cosmetic fixes.
        regards, tom lane

On Tue, Jun 23, 2015 at 3:48 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Tue, Jun 23, 2015 at 2:33 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> I do not know at this point whether these behaviors are really the same
>>> bug or not, but I wonder whether it's time to consider back-patching the
>>> renegotiation fixes we did in 9.4.  Specifically, I think maybe we should
>>> back-patch 31cf1a1a4, 86029b31e, and 36a3be654.  (There are more changes
>>> in master, but since those haven't yet shipped in any released branch,
>>> and there's been a lot of other rework in the same area, those probably
>>> are not back-patch candidates.)
>>>
>>> Thoughts?
>
>> I have no clear idea how safe it is to back-port these fixes.
>
> Well, it would mean that pre-9.5 branches all behave the same, which
> would be an improvement in my book.  Also, ISTM that the 9.4 code
> for renegotiation assumes a whole lot less than prior branches about
> OpenSSL's internal behavior; so it ought to be more robust, even if
> some bugs remain.
>
>> Just as a point of reference, we had a customer hit a problem similar
>> to bug #12769 on 9.3.x.  I think (but am not sure) that 272923a0a may
>> have been intended to fix that issue.  In a quick search, I didn't
>> find any other complaints about renegotiation-related issues from our
>> customers.
>
> The problem with trying to adopt code from HEAD is that it probably
> depends on the rather invasive changes explained here:
> http://www.postgresql.org/message-id/20150126101405.GA31719@awork2.anarazel.de
> Even assuming that there's no dependency on the immediate-interrupt
> changes, I'm afraid to back-patch anything that invasive.

What commits actually resulted from that?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2015-06-23 14:33:21 -0400, Tom Lane wrote:
> I do not know at this point whether these behaviors are really the same
> bug or not, but I wonder whether it's time to consider back-patching the
> renegotiation fixes we did in 9.4.

I, by now, have come to a different conclusion. I think it's time to
entirely drop the renegotiation support.

While there's a security benefit of renegotiation by limiting the amount
of leaked data in case either client or server is exploited while the
connection is ongoing, the reality is that the renegotiation support in
openssl just isn't up to the task.

Both Heikki and I have spent a considerable amount of time trying to
find workarounds for the renegotiation bugs in openssl, but so far I
don't think that's bullet proof. Additionally the track record of
renegotiation both in ssl specification and in the openssl specification
is that it opens many more security holes than it fixes.

Greetings,

Andres Freund

Tom Lane wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> >> On Tue, Jun 23, 2015 at 2:33 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >>> I do not know at this point whether these behaviors are really the same
> >>> bug or not, but I wonder whether it's time to consider back-patching the
> >>> renegotiation fixes we did in 9.4.  Specifically, I think maybe we should
> >>> back-patch 31cf1a1a4, 86029b31e, and 36a3be654.
> 
> > Yes, +1 for backpatching.  Don't forget 5674460b and b1aebbb6.
> 
> Huh?  5674460b is ancient, and we concluded that b1aebbb6 didn't represent
> anything much more than cosmetic fixes.

Sorry, I mixed up 5674460b with 36a3be65 which you already mentioned ...
and I see that because of the conclusions from 272923a0a695 then the
one-char change in b1aebbb6 is not very interesting.

I do think that perhaps we should simplify the code down to what
272923a0a695 + 1c2b7c0879d8 do.


I also agree that the other changes by Andres and Heikki, which involve
making all communication use a nonblocking socket, seem too invasive to
backpatch; even with the insurance provided by beta+release, the
disruptiveness of changes seems a bit too high, considering that
387da18874a apparently cannot be used without 4f85fde8eb which is a bit
scary in itself.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Andres Freund <andres@anarazel.de> writes:
> I, by now, have come to a different conclusion. I think it's time to
> entirely drop the renegotiation support.

Well, that's a radical proposal, but I think we should take it seriously.

On balance I think I agree that SSL renegotiation has not been worth the
trouble.  And we definitely aren't testing it adequately, so if we wanted
to keep it then there's even *more* work that somebody ought to expend.

I assume we'd back-patch it, too?  (Probably not remove the
ssl_renegotiation_limit variable, but always act as though it were zero.)
If we still have to maintain the code in the back branches then we'd
continue to have to deal with its bugs for some time.
        regards, tom lane

On 2015-06-24 11:11:16 -0400, Tom Lane wrote:
> On balance I think I agree that SSL renegotiation has not been worth the
> trouble.  And we definitely aren't testing it adequately, so if we wanted
> to keep it then there's even *more* work that somebody ought to expend.

Right. Our code was nearly entirely broken for streaming replication for
*years* without anybody noticing. And even now it doesn't reliably
work. It's also pretty hard to test due to the required data volumes and
the vast number of different behaviours across openssl versions.

> I assume we'd back-patch it, too?  (Probably not remove the
> ssl_renegotiation_limit variable, but always act as though it were
> zero.)

Yes, I think so. Maybe log a warning at startup if set to nonzero
(startup is probably the best we can do).

Greetings,

Andres Freund

On 6/23/15 2:33 PM, Tom Lane wrote:
> I do not know at this point whether these behaviors are really the same
> bug or not, but I wonder whether it's time to consider back-patching the
> renegotiation fixes we did in 9.4.

If Red Hat fixes their bug, then PostgreSQL doesn't have any actual
problem anymore, does it?

On 2015-06-24 11:57:53 -0400, Peter Eisentraut wrote:
> On 6/23/15 2:33 PM, Tom Lane wrote:
> > I do not know at this point whether these behaviors are really the same
> > bug or not, but I wonder whether it's time to consider back-patching the
> > renegotiation fixes we did in 9.4.
> 
> If Red Hat fixes their bug, then PostgreSQL doesn't have any actual
> problem anymore, does it?

It does, there are numerous bugs around renegotiation that exist with
upstream openssl and postgres. More in the older branches, but even in
HEAD we break regularly. Most only occur in replication connections (due
to copy both) and/or when using more complex clients where clients and
servers send data at the same time due to pipelining.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> On 2015-06-24 11:57:53 -0400, Peter Eisentraut wrote:
>> If Red Hat fixes their bug, then PostgreSQL doesn't have any actual
>> problem anymore, does it?

> It does, there are numerous bugs around renegotiation that exist with
> upstream openssl and postgres. More in the older branches, but even in
> HEAD we break regularly. Most only occur in replication connections (due
> to copy both) and/or when using more complex clients where clients and
> servers send data at the same time due to pipelining.

The lesson to learn from the Red Hat fiasco is that vendors are not
adequately testing renegotiation either.  All the more reason to get
out from under it.  I did not like being told that "Postgres fails and
$randomapp doesn't, therefore it's Postgres' problem" when actually
the difference was that $randomapp doesn't invoke renegotiation.
        regards, tom lane

<p dir="ltr"><br /> On Jun 24, 2015 5:13 PM, "Tom Lane" <<a
href="mailto:tgl@sss.pgh.pa.us">tgl@sss.pgh.pa.us</a>>wrote:<br /> ><br /> > Andres Freund <<a
href="mailto:andres@anarazel.de">andres@anarazel.de</a>>writes:<br /> > > I, by now, have come to a different
conclusion.I think it's time to<br /> > > entirely drop the renegotiation support.<br /> ><br /> > Well,
that'sa radical proposal, but I think we should take it seriously.<br /> ><br /><p dir="ltr">Yes. <p dir="ltr">Just
onmy phone right now, but wasn't renegotiation also an attack vector in one of the recent openssl bugs? <p
dir="ltr">/Magnus<br /> 

On Wed, Jun 24, 2015 at 11:11 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Andres Freund <andres@anarazel.de> writes:
>> I, by now, have come to a different conclusion. I think it's time to
>> entirely drop the renegotiation support.
>
> Well, that's a radical proposal, but I think we should take it seriously.
>
> On balance I think I agree that SSL renegotiation has not been worth the
> trouble.  And we definitely aren't testing it adequately, so if we wanted
> to keep it then there's even *more* work that somebody ought to expend.

I'd like to know what factors we are balancing against each other.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2015-06-24 12:57:03 -0400, Robert Haas wrote:
> On Wed, Jun 24, 2015 at 11:11 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Andres Freund <andres@anarazel.de> writes:
> >> I, by now, have come to a different conclusion. I think it's time to
> >> entirely drop the renegotiation support.
> >
> > Well, that's a radical proposal, but I think we should take it seriously.
> >
> > On balance I think I agree that SSL renegotiation has not been worth the
> > trouble.  And we definitely aren't testing it adequately, so if we wanted
> > to keep it then there's even *more* work that somebody ought to expend.
> 
> I'd like to know what factors we are balancing against each other.

Benefits:

SSL renegotiation limits the exposure of on-the-wire material that's
leaked if either client or server is hijacked during a existing
session. With renegotiation the client/server will hopefully only have
the currently negotiated symetric key, covering only
session_renegotiation_limit bytes, in memory.

That's nice, but from a practical point of view it's not worth all that
much. If the server has been hacked nearly all the data is accessible
anyway, and even if not, the hacker can just continue collecting data
going forward.  If the client has been hacked, it's likely that it has
been relegating data for the session to somewhere that's still
accessible.

For the server hacked case all that generally only matters if perfect
forward secrecy (PFS) has been employed, without that the session keys
can be recovered anyway as the private key will be accessible in memory.

All this only matters for hacks that access running processes.

Deficits:

SSL renegotiation has had several weaknesses (c.f. CVE-2009-3555/RFC
5746 , although I'm not 100% sure it's possible to apply to PG) on the
protocol level, at least partially leading to much worse vulnerabilities
than renegotiation in the pg style fixes. The openssl implementation has
had several bugs several of them unfixed years after they were reported
(#3712, #2481, #2146,...). By my reading of openssl's code the current
state is entirely broken for duplex communication.

The current draft of the next version of the TLS standard removes
renegotiation entirely.

Additionally supporting SSL renegotiation requires SSL_write/read
callers to always call the SSL_write/read after either function has
reported the need for additional reads/writes (note that SSL_read can
require writes and the other way round). We don't comply with that rule,
especially in the backbranches, because it's really hard to do that.

Our code currently uses crude hacks (c.f. comment around
SSL_clear_num_renegotiations(), and the loop around SSL_do_handshake()
in the back branches) to manage renegotiations. There's pending patches
to substantially increase the amount of ugly hacking to cope with us
misusing the SSL_read/write protocol.


Greetings,

Andres Freund

On 2015-06-24 19:35:51 +0200, Andres Freund wrote:
> Our code currently uses crude hacks (c.f. comment around
> SSL_clear_num_renegotiations(), and the loop around SSL_do_handshake()
> in the back branches) to manage renegotiations. There's pending patches
> to substantially increase the amount of ugly hacking to cope with us
> misusing the SSL_read/write protocol.

C.f.
http://archives.postgresql.org/message-id/54DE6FAF.6050005%40vmware.com

On 6/24/15 12:26 PM, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
>> On 2015-06-24 11:57:53 -0400, Peter Eisentraut wrote:
>>> If Red Hat fixes their bug, then PostgreSQL doesn't have any actual
>>> problem anymore, does it?
> 
>> It does, there are numerous bugs around renegotiation that exist with
>> upstream openssl and postgres. More in the older branches, but even in
>> HEAD we break regularly. Most only occur in replication connections (due
>> to copy both) and/or when using more complex clients where clients and
>> servers send data at the same time due to pipelining.
> 
> The lesson to learn from the Red Hat fiasco is that vendors are not
> adequately testing renegotiation either.  All the more reason to get
> out from under it.  I did not like being told that "Postgres fails and
> $randomapp doesn't, therefore it's Postgres' problem" when actually
> the difference was that $randomapp doesn't invoke renegotiation.

I'm fine with removing renegotiation.  But the original proposal was to
backpatch renegation changes, which seemed like replacing one problem
variation with another, and does not sound comfortable given recent
backpatching record.

On June 24, 2015 9:07:35 PM GMT+02:00, Peter Eisentraut <peter_e@gmx.net> wrote:
>On 6/24/15 12:26 PM, Tom Lane wrote:
>> Andres Freund <andres@anarazel.de> writes:
>>> On 2015-06-24 11:57:53 -0400, Peter Eisentraut wrote:
>>>> If Red Hat fixes their bug, then PostgreSQL doesn't have any actual
>>>> problem anymore, does it?
>> 
>>> It does, there are numerous bugs around renegotiation that exist
>with
>>> upstream openssl and postgres. More in the older branches, but even
>in
>>> HEAD we break regularly. Most only occur in replication connections
>(due
>>> to copy both) and/or when using more complex clients where clients
>and
>>> servers send data at the same time due to pipelining.
>> 
>> The lesson to learn from the Red Hat fiasco is that vendors are not
>> adequately testing renegotiation either.  All the more reason to get
>> out from under it.  I did not like being told that "Postgres fails
>and
>> $randomapp doesn't, therefore it's Postgres' problem" when actually
>> the difference was that $randomapp doesn't invoke renegotiation.
>
>I'm fine with removing renegotiation.  But the original proposal was to
>backpatch renegation changes, which seemed like replacing one problem
>variation with another, and does not sound comfortable given recent
>backpatching record.

Meh. The relevant branches already exist, as you can disable it today.

We could also just change the default in the back branches.

--- 
Please excuse brevity and formatting - I am writing this on my mobile phone.

On 6/24/15 3:13 PM, Andres Freund wrote:
> Meh. The relevant branches already exist, as you can disable it today.
> 
> We could also just change the default in the back branches.

One more argument for leaving everything alone.  If users don't like it,
they can turn it off themselves.

On 2015-06-24 15:41:22 -0400, Peter Eisentraut wrote:
> On 6/24/15 3:13 PM, Andres Freund wrote:
> > Meh. The relevant branches already exist, as you can disable it today.
> > 
> > We could also just change the default in the back branches.
> 
> One more argument for leaving everything alone.  If users don't like it,
> they can turn it off themselves.

Because it's so obvious to get there from "SSL error: unexpected
message", "SSL error: bad write retry" or "SSL error: unexpected record"
to disabling renegotiation. Right?  Search the archives and you'll find
plenty of those, mostly in relation to streaming rep. It took -hackers
years to figure out what causes those, how are normal users supposed to
a) correlate such errors with renegotiation b) evaluate what do about
it?

On Wed, Jun 24, 2015 at 3:41 PM, Peter Eisentraut <peter_e@gmx.net> wrote:
> On 6/24/15 3:13 PM, Andres Freund wrote:
>> Meh. The relevant branches already exist, as you can disable it today.
>>
>> We could also just change the default in the back branches.
>
> One more argument for leaving everything alone.  If users don't like it,
> they can turn it off themselves.

I find it very hard to disagree with that perspective.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Jun 24, 2015 at 3:49 PM, Andres Freund <andres@anarazel.de> wrote:
> On 2015-06-24 15:41:22 -0400, Peter Eisentraut wrote:
>> On 6/24/15 3:13 PM, Andres Freund wrote:
>> > Meh. The relevant branches already exist, as you can disable it today.
>> >
>> > We could also just change the default in the back branches.
>>
>> One more argument for leaving everything alone.  If users don't like it,
>> they can turn it off themselves.
>
> Because it's so obvious to get there from "SSL error: unexpected
> message", "SSL error: bad write retry" or "SSL error: unexpected record"
> to disabling renegotiation. Right?  Search the archives and you'll find
> plenty of those, mostly in relation to streaming rep. It took -hackers
> years to figure out what causes those, how are normal users supposed to
> a) correlate such errors with renegotiation b) evaluate what do about
> it?

We could document the issues, create release-note entries suggesting a
configuration change, and/or blog about it.

I don't accept the argument that there are not ways to tell users
about things they might want to do.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Jun 24, 2015 at 3:49 PM, Andres Freund <andres@anarazel.de> wrote:
>> On 2015-06-24 15:41:22 -0400, Peter Eisentraut wrote:
>>> One more argument for leaving everything alone.  If users don't like it,
>>> they can turn it off themselves.

>> Because it's so obvious to get there from "SSL error: unexpected
>> message", "SSL error: bad write retry" or "SSL error: unexpected record"
>> to disabling renegotiation. Right?  Search the archives and you'll find
>> plenty of those, mostly in relation to streaming rep. It took -hackers
>> years to figure out what causes those, how are normal users supposed to
>> a) correlate such errors with renegotiation b) evaluate what do about
>> it?

> We could document the issues, create release-note entries suggesting a
> configuration change, and/or blog about it.
> I don't accept the argument that there are not ways to tell users
> about things they might want to do.

I think there's a strong argument for changing the default setting to
zero (no renegotiation), even in the back branches.
        regards, tom lane

<p dir="ltr"><br /> On Jun 24, 2015 7:40 PM, "Andres Freund" <<a
href="mailto:andres@anarazel.de">andres@anarazel.de</a>>wrote:<br /> ><br /> > On 2015-06-24 12:57:03 -0400,
RobertHaas wrote:<br /> > > On Wed, Jun 24, 2015 at 11:11 AM, Tom Lane <<a
href="mailto:tgl@sss.pgh.pa.us">tgl@sss.pgh.pa.us</a>>wrote:<br /> > > > Andres Freund <<a
href="mailto:andres@anarazel.de">andres@anarazel.de</a>>writes:<br /> > > >> I, by now, have come to a
differentconclusion. I think it's time to<br /> > > >> entirely drop the renegotiation support.<br /> >
>><br /> > > > Well, that's a radical proposal, but I think we should take it seriously.<br /> > >
><br/> > > > On balance I think I agree that SSL renegotiation has not been worth the<br /> > > >
trouble. And we definitely aren't testing it adequately, so if we wanted<br /> > > > to keep it then there's
even*more* work that somebody ought to expend.<br /> > ><br /> > > I'd like to know what factors we are
balancingagainst each other.<br /> ><br /> > Benefits:<br /> ><br /> > SSL renegotiation limits the
exposureof on-the-wire material that's<br /> > leaked if either client or server is hijacked during a existing<br />
>session. With renegotiation the client/server will hopefully only have<br /> > the currently negotiated symetric
key,covering only<br /> > session_renegotiation_limit bytes, in memory.<br /> ><br /> > That's nice, but from
apractical point of view it's not worth all that<br /> > much. If the server has been hacked nearly all the data is
accessible<br/> > anyway, and even if not, the hacker can just continue collecting data<br /> > going forward. 
Ifthe client has been hacked, it's likely that it has<br /> > been relegating data for the session to somewhere
that'sstill<br /> > accessible.<br /> ><br /> > For the server hacked case all that generally only matters if
perfect<br/> > forward secrecy (PFS) has been employed, without that the session keys<br /> > can be recovered
anywayas the private key will be accessible in memory.<br /> ><br /> > All this only matters for hacks that
accessrunning processes.<br /> ><br /> > Deficits:<br /> ><br /> > SSL renegotiation has had several
weaknesses(c.f. CVE-2009-3555/RFC<br /> > 5746 , although I'm not 100% sure it's possible to apply to PG) on the<br
/>> protocol level, at least partially leading to much worse vulnerabilities<br /> > than renegotiation in the pg
stylefixes. The openssl implementation has<br /> > had several bugs several of them unfixed years after they were
reported<br/> > (#3712, #2481, #2146,...). By my reading of openssl's code the current<br /> > state is entirely
brokenfor duplex communication.<br /> ><br /> > The current draft of the next version of the TLS standard
removes<br/> > renegotiation entirely.<br /><p dir="ltr">I think this is a very strong argument against it. If the
standardspeople now think it's a bad idea, we shouldn't encourage it. <p dir="ltr">That's assuming they haven't
replacedit with something else (even more complicated of course), and not just removed it. But i don't think they did.
<pdir="ltr">/Magnus  

On 2015-06-24 17:20:31 -0400, Robert Haas wrote:
> On Wed, Jun 24, 2015 at 3:49 PM, Andres Freund <andres@anarazel.de> wrote:
> > On 2015-06-24 15:41:22 -0400, Peter Eisentraut wrote:
> >> On 6/24/15 3:13 PM, Andres Freund wrote:
> >> > Meh. The relevant branches already exist, as you can disable it today.
> >> >
> >> > We could also just change the default in the back branches.
> >>
> >> One more argument for leaving everything alone.  If users don't like it,
> >> they can turn it off themselves.
> >
> > Because it's so obvious to get there from "SSL error: unexpected
> > message", "SSL error: bad write retry" or "SSL error: unexpected record"
> > to disabling renegotiation. Right?  Search the archives and you'll find
> > plenty of those, mostly in relation to streaming rep. It took -hackers
> > years to figure out what causes those, how are normal users supposed to
> > a) correlate such errors with renegotiation b) evaluate what do about
> > it?
> 
> We could document the issues, create release-note entries suggesting a
> configuration change, and/or blog about it.

The situation is this: We have broken code using broken code. I think we
either got to apply, darn nontrivial, fixes from
http://archives.postgresql.org/message-id/54DE6FAF.6050005%40vmware.com
or we got to cripple the options.

It's also not the first breakage, we've applied a lot of bandaids to
this code already. Our way of doing renegotiation also has broken
several SSL client implementations...

Right now if you use streaming rep over ssl, it breaks after a couple
hundred megabytes with obscure errors. The user might or might not
notice that. He might just see errors around syncrep or something. Or
notice that logical decoding never finishes streaming out one huge as
xact, or ...

> I don't accept the argument that there are not ways to tell users
> about things they might want to do.

We probably could do that. But why would we want to? It's just as much
work, and puts the onus on more people?

Greetings,

Andres Freund

On 06/25/2015 03:03 PM, Andres Freund wrote:
> The situation is this: We have broken code using broken code. I think we
> either got to apply, darn nontrivial, fixes from
> http://archives.postgresql.org/message-id/54DE6FAF.6050005%40vmware.com
> or we got to cripple the options.
>
> It's also not the first breakage, we've applied a lot of bandaids to
> this code already. Our way of doing renegotiation also has broken
> several SSL client implementations...

Note that even with those patches, renegotiation is still broken in some 
scenarios: 
http://www.postgresql.org/message-id/54DCF736.2060207@vmware.com. As far 
as I can tell, OpenSSL's handling of renegotiation is fundamentally 
broken, and there is nothing we can do in the application to completely 
work around that.

+1 for changing the default to disable renegotiation, in all branches.

- Heikki

On 6/25/15 8:03 AM, Andres Freund wrote:
> Right now if you use streaming rep over ssl, it breaks after a couple
> hundred megabytes with obscure errors.

If it's that bad, then I tend to agree we should turn it off by default.

On 06/25/2015 06:15 AM, Peter Eisentraut wrote:
>
> On 6/25/15 8:03 AM, Andres Freund wrote:
>> Right now if you use streaming rep over ssl, it breaks after a couple
>> hundred megabytes with obscure errors.
>
> If it's that bad, then I tend to agree we should turn it off by default.
>
From an "in the wild perspective", we run into this all the time.

+1 to turn it off by default in all supported branches.

JD


-- 
Command Prompt, Inc. - http://www.commandprompt.com/  503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Announcing "I'm offended" is basically telling the world you can't
control your own emotions, so everyone else should do it for you.

On Thu, Jun 25, 2015 at 8:03 AM, Andres Freund <andres@anarazel.de> wrote:
>> I don't accept the argument that there are not ways to tell users
>> about things they might want to do.
>
> We probably could do that. But why would we want to? It's just as much
> work, and puts the onus on more people?

Because it doesn't force a behavior change down everyone's throat.

If it were just a question of back-porting fixes, even someone
invasive ones, well, maybe that's what we have to do; that's pretty
much exactly what we are planning to do for the MultiXact case, but
according to Heikki, this is broken even in master and can't really be
fixed unless and until OpenSSL gets their act together.  That's a hard
argument to argue with, and I think I'm on board with it.

But as a general point, we should be very reluctant to force behavior
changes on our users in released branches, because users don't like
that.  When there are reasonable alternatives to doing that, we should
choose them.  If we have no other reasonable choice here, so be it.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 06/26/2015 04:53 PM, Robert Haas wrote:
> On Thu, Jun 25, 2015 at 8:03 AM, Andres Freund <andres@anarazel.de> wrote:
>>> I don't accept the argument that there are not ways to tell users
>>> about things they might want to do.
>>
>> We probably could do that. But why would we want to? It's just as much
>> work, and puts the onus on more people?
>
> Because it doesn't force a behavior change down everyone's throat.

It's arguable whether this is a change in behaviour or not. SSL 
renegotiation is (supposed to be) completely transparent to the user.

- Heikki

On 2015-06-26 09:53:30 -0400, Robert Haas wrote:
> On Thu, Jun 25, 2015 at 8:03 AM, Andres Freund <andres@anarazel.de> wrote:
> >> I don't accept the argument that there are not ways to tell users
> >> about things they might want to do.
> >
> > We probably could do that. But why would we want to? It's just as much
> > work, and puts the onus on more people?
> 
> Because it doesn't force a behavior change down everyone's throat.

Generally I'd agree that that is a bad thing. But there's really not
much of a observable behaviour change in this case? Except that
connections using ssl break less often.

> If it were just a question of back-porting fixes, even someone
> invasive ones, well, maybe that's what we have to do; that's pretty
> much exactly what we are planning to do for the MultiXact case

There's no way we can reasonably "disable" multixacts, so I don't think
these situations are comparable.

> but according to Heikki, this is broken even in master and can't really be
> fixed unless and until OpenSSL gets their act together.

Yes, that's my conclusion as well, leading to my position in this
thread.

> That's a hard argument to argue with, and I think I'm on board with
> it.

Given that reported bugs for openssl around this have existed since
about 2002 without any effort at fixing, I think it's seriously unlikely
that they ever will.

Greetings,

Andres Freund

On Fri, Jun 26, 2015 at 9:59 AM, Andres Freund <andres@anarazel.de> wrote:
> Generally I'd agree that that is a bad thing. But there's really not
> much of a observable behaviour change in this case? Except that
> connections using ssl break less often.

Well, SSL renegotiation exists for a reason: to improve security.
It's not awesome that we're being forced to shut off features that are
designed to improve security.  But it seems we have little choice, at
least until we can support some other SSL implementation (and maybe
not even then).

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, Jun 26, 2015 at 9:59 AM, Andres Freund <andres@anarazel.de> wrote:
>> Generally I'd agree that that is a bad thing. But there's really not
>> much of a observable behaviour change in this case? Except that
>> connections using ssl break less often.

> Well, SSL renegotiation exists for a reason: to improve security.

That was the theory, yes, but the CVEs that have come out of it indicate
that whether it improves security *in practice* is a pretty debatable
topic.  The fact that the new TLS draft drops it altogether tells us
something about the conclusion the standards community has arrived at.
        regards, tom lane

On 2015-06-26 10:26:58 -0400, Robert Haas wrote:
> On Fri, Jun 26, 2015 at 9:59 AM, Andres Freund <andres@anarazel.de> wrote:
> > Generally I'd agree that that is a bad thing. But there's really not
> > much of a observable behaviour change in this case? Except that
> > connections using ssl break less often.
> 
> Well, SSL renegotiation exists for a reason: to improve security.

Well, except that even if it were implemented correctly it's far from
clear cut that it's a win:

If your argument is that key-rotation is beneficial because it gives an
attacker less encrypted material to analyze: That's not a good argument,
you're just giving him more information about the assymetric crypto side
of things instead about the session key which is ephemeral anyway.

I think they only real argument for it is that you want to limit the
amount of data you could decrypt if you gain access to the current
symmetric key via the client's memory . But that's not a particularly
large benefit.

> But it seems we have little choice, at least until we can support some
> other SSL implementation (and maybe not even then).

I read through one other SSL implementation (NSS), and I don't think
it's substantially better handled there. At least one other
implementations is ripping out support entirely already.

On 2015-06-24 16:41:48 +0200, Andres Freund wrote:
> I, by now, have come to a different conclusion. I think it's time to
> entirely drop the renegotiation support.

I think by now we essentially concluded that we should do that. What I'm
not sure yet is how: Do we want to rip it out in master and just change
the default in the backbranches, or do we want to rip it out in all
branches and leave a faux guc in place in the back branches. I vote for
the latter, but would be ok with both variants.

On 2015-06-26 15:36:53 -0400, David G. Johnston wrote:
> On Fri, Jun 26, 2015 at 3:09 PM, Andres Freund <andres@anarazel.de> wrote:
> 
> > On 2015-06-24 16:41:48 +0200, Andres Freund wrote:
> > > I, by now, have come to a different conclusion. I think it's time to
> > > entirely drop the renegotiation support.
> >
> > I think by now we essentially concluded that we should do that. What I'm
> > not sure yet is how: Do we want to rip it out in master and just change
> > the default in the backbranches, or do we want to rip it out in all
> > branches and leave a faux guc in place in the back branches. I vote for
> > the latter, but would be ok with both variants.
> >
> >
> ​3. ​Change the "default" and make the guc impotent - in the back
> branches.  Its minimally invasive and accomplishes the same user-facing
> goal as "ripping it out".

What would be the point of that? The code is pretty localized, so
leaving it in, but unreachable, seems to have no benefits.

Andres Freund <andres@anarazel.de> writes:
> On 2015-06-24 16:41:48 +0200, Andres Freund wrote:
>> I, by now, have come to a different conclusion. I think it's time to
>> entirely drop the renegotiation support.

> I think by now we essentially concluded that we should do that. What I'm
> not sure yet is how: Do we want to rip it out in master and just change
> the default in the backbranches, or do we want to rip it out in all
> branches and leave a faux guc in place in the back branches. I vote for
> the latter, but would be ok with both variants.

I think the former is probably the saner answer.  It is less likely to
annoy people who dislike back-branch changes.  And it will be
significantly less work, considering that that code has changed enough
that you won't be able to just cherry-pick a removal patch.  I also fear
there's a nonzero chance of breaking stuff if you're careless about doing
the removal in one or more of the five active back branches ...
        regards, tom lane

On Sat, Jun 27, 2015 at 6:12 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Andres Freund <andres@anarazel.de> writes:
>> On 2015-06-24 16:41:48 +0200, Andres Freund wrote:
>>> I, by now, have come to a different conclusion. I think it's time to
>>> entirely drop the renegotiation support.
>
>> I think by now we essentially concluded that we should do that. What I'm
>> not sure yet is how: Do we want to rip it out in master and just change
>> the default in the backbranches, or do we want to rip it out in all
>> branches and leave a faux guc in place in the back branches. I vote for
>> the latter, but would be ok with both variants.
>
> I think the former is probably the saner answer.  It is less likely to
> annoy people who dislike back-branch changes.  And it will be
> significantly less work, considering that that code has changed enough
> that you won't be able to just cherry-pick a removal patch.  I also fear
> there's a nonzero chance of breaking stuff if you're careless about doing
> the removal in one or more of the five active back branches ...

+1 for removing on master and just disabling on back-branches.
-- 
Michael

<p dir="ltr"><br /> On Jun 27, 2015 8:07 AM, "Michael Paquier" <<a
href="mailto:michael.paquier@gmail.com">michael.paquier@gmail.com</a>>wrote:<br /> ><br /> > On Sat, Jun 27,
2015at 6:12 AM, Tom Lane <<a href="mailto:tgl@sss.pgh.pa.us">tgl@sss.pgh.pa.us</a>> wrote:<br /> > > Andres
Freund<<a href="mailto:andres@anarazel.de">andres@anarazel.de</a>> writes:<br /> > >> On 2015-06-24
16:41:48+0200, Andres Freund wrote:<br /> > >>> I, by now, have come to a different conclusion. I think
it'stime to<br /> > >>> entirely drop the renegotiation support.<br /> > ><br /> > >> I
thinkby now we essentially concluded that we should do that. What I'm<br /> > >> not sure yet is how: Do we
wantto rip it out in master and just change<br /> > >> the default in the backbranches, or do we want to rip
itout in all<br /> > >> branches and leave a faux guc in place in the back branches. I vote for<br /> >
>>the latter, but would be ok with both variants.<br /> > ><br /> > > I think the former is probably
thesaner answer.  It is less likely to<br /> > > annoy people who dislike back-branch changes.  And it will be<br
/>> > significantly less work, considering that that code has changed enough<br /> > > that you won't be
ableto just cherry-pick a removal patch.  I also fear<br /> > > there's a nonzero chance of breaking stuff if
you'recareless about doing<br /> > > the removal in one or more of the five active back branches ...<br />
><br/> > +1 for removing on master and just disabling on back-branches.<br /><br /><p dir="ltr">+1. Definitely
soundslike the safer choice. <p dir="ltr">/Magnus  

On 2015-06-27 15:07:05 +0900, Michael Paquier wrote:
> On Sat, Jun 27, 2015 at 6:12 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Andres Freund <andres@anarazel.de> writes:
> >> On 2015-06-24 16:41:48 +0200, Andres Freund wrote:
> >>> I, by now, have come to a different conclusion. I think it's time to
> >>> entirely drop the renegotiation support.
> >
> >> I think by now we essentially concluded that we should do that. What I'm
> >> not sure yet is how: Do we want to rip it out in master and just change
> >> the default in the backbranches, or do we want to rip it out in all
> >> branches and leave a faux guc in place in the back branches. I vote for
> >> the latter, but would be ok with both variants.
> >
> > I think the former is probably the saner answer.  It is less likely to
> > annoy people who dislike back-branch changes.  And it will be
> > significantly less work, considering that that code has changed enough
> > that you won't be able to just cherry-pick a removal patch.  I also fear
> > there's a nonzero chance of breaking stuff if you're careless about doing
> > the removal in one or more of the five active back branches ...
> 
> +1 for removing on master and just disabling on back-branches.

The problem with that approach is that it leaves people hanging in the
dry if they've uncommented the default value, or changed it. That
doesn't seem nice to me.

Andres

Andres Freund <andres@anarazel.de> writes:
> On 2015-06-27 15:07:05 +0900, Michael Paquier wrote:
>> +1 for removing on master and just disabling on back-branches.

> The problem with that approach is that it leaves people hanging in the
> dry if they've uncommented the default value, or changed it. That
> doesn't seem nice to me.

I think at least 99% of the people who are using a nondefault value of
ssl_renegotiation_limit are using zero and so would have no problem with
this at all.  Possibly 100% of them; there's not really much use-case for
changing from 512MB to some other nonzero value, is there?
        regards, tom lane

On 2015-06-27 12:10:49 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2015-06-27 15:07:05 +0900, Michael Paquier wrote:
> >> +1 for removing on master and just disabling on back-branches.
> 
> > The problem with that approach is that it leaves people hanging in the
> > dry if they've uncommented the default value, or changed it. That
> > doesn't seem nice to me.
> 
> I think at least 99% of the people who are using a nondefault value of
> ssl_renegotiation_limit are using zero and so would have no problem with
> this at all.  Possibly 100% of them; there's not really much use-case for
> changing from 512MB to some other nonzero value, is there?

While still at 2ndq I've seen some increase it to nonzero values to cope
with the connection breaks.

Andres

On Sat, Jun 27, 2015 at 06:13:36PM +0200, Andres Freund wrote:
> On 2015-06-27 12:10:49 -0400, Tom Lane wrote:
> > Andres Freund <andres@anarazel.de> writes:
> > > On 2015-06-27 15:07:05 +0900, Michael Paquier wrote:
> > >> +1 for removing on master and just disabling on back-branches.
> > 
> > > The problem with that approach is that it leaves people hanging in the
> > > dry if they've uncommented the default value, or changed it. That
> > > doesn't seem nice to me.
> > 
> > I think at least 99% of the people who are using a nondefault value of
> > ssl_renegotiation_limit are using zero and so would have no problem with
> > this at all.  Possibly 100% of them; there's not really much use-case for
> > changing from 512MB to some other nonzero value, is there?
> 
> While still at 2ndq I've seen some increase it to nonzero values to cope
> with the connection breaks.

We'd need to be triply confident that we know better than the DBA before
removing flexibility in back branches.  +1 for just changing the default.
Suppose some security policy mandates a particular key rotation interval; the
minor release would force an awkward decision on that user.  DBAs who have
customized ssl_renegotiation_limit are more likely than most to notice the
release note and make an informed decision.

On 2015-07-01 23:32:23 -0400, Noah Misch wrote:
> We'd need to be triply confident that we know better than the DBA before
> removing flexibility in back branches.
> +1 for just changing the default.

I think we do. But I also think that I pretty clearly lost this
argument, so let's just change the default.

Is anybody willing to work on this?

On 2015-07-11 21:09:05 +0900, Michael Paquier wrote:
> Something like the patches attached

Thanks for that!

> could be considered, one is for master
> and REL9_5_STABLE to remove ssl_renegotiation_limit, the second one for
> ~REL9_4_STABLE to change the default to 0.

> diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
> index c669f75..16c0ce5 100644
> --- a/doc/src/sgml/config.sgml
> +++ b/doc/src/sgml/config.sgml
> @@ -1040,7 +1040,7 @@ include_dir 'conf.d'
>          cryptanalysis when large amounts of traffic can be examined, but it
>          also carries a large performance penalty. The sum of sent and received
>          traffic is used to check the limit. If this parameter is set to 0,
> -        renegotiation is disabled. The default is <literal>512MB</>.
> +        renegotiation is disabled. The default is <literal>0</>.

I think we should put in a warning or at least note about the dangers of
enabling it (connection breaks, exposure to several open openssl bugs).


Thanks,

Andres

Hi,

Attached are:

a) a slightly evolved version of Michael's patch disabling renegotiation  by default that I'm planning to apply to 9.4
-9.0
 

b) a patch removing renegotiation entirely from master and 9.5

Unless somebody protests soon I'm going to push something like that
after having dinner.

I am wondering whether b) ought to remove Port->count, but I'm currently
leaning to leaving it in place for now; perhaps adding a comment in the
struct.  I'm actually thinking we very well might want to add something
like it to all backends, but more importantly it'd make the diff larger
with mostly unrelated changes.

Regards,

Andres

On 2015-07-28 18:59:02 +0200, Andres Freund wrote:
> Attached are:
>
> a) a slightly evolved version of Michael's patch disabling renegotiation
>    by default that I'm planning to apply to 9.4 - 9.0
>
> b) a patch removing renegotiation entirely from master and 9.5
>
> Unless somebody protests soon I'm going to push something like that
> after having dinner.
>
> I am wondering whether b) ought to remove Port->count, but I'm currently
> leaning to leaving it in place for now; perhaps adding a comment in the
> struct.  I'm actually thinking we very well might want to add something
> like it to all backends, but more importantly it'd make the diff larger
> with mostly unrelated changes.

And really attached.

On 2015-07-28 18:59:02 +0200, Andres Freund wrote:
> Unless somebody protests soon I'm going to push something like that
> after having dinner.

Done.

Andres Freund wrote:
> On 2015-07-28 18:59:02 +0200, Andres Freund wrote:
> > Unless somebody protests soon I'm going to push something like that
> > after having dinner.
> 
> Done.

Yay!

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Wed, Jul 29, 2015 at 2:00 AM, Andres Freund <andres@anarazel.de> wrote:
> On 2015-07-28 18:59:02 +0200, Andres Freund wrote:
>> Attached are:
>>
>> a) a slightly evolved version of Michael's patch disabling renegotiation
>>    by default that I'm planning to apply to 9.4 - 9.0
>>
>> b) a patch removing renegotiation entirely from master and 9.5

Note: there was already upthread a patch for that:
http://www.postgresql.org/message-id/CAB7nPqQnjiDixR5qNJ86QnM++sKpyTedTNLF_vNPmVtu5xOZyQ@mail.gmail.com
But it doesn't matter much. Thanks for the final push.
-- 
Michael