Horton

Thread

Commits

  1. Fix building with MSVC for TLS session disabling

  2. Fix macro placement in pg_config.h.in

  3. Disable all TLS session tickets

  1. tls 1.3: sending multiple tickets

    Andres Freund <andres@anarazel.de> — 2024-06-17T17:38:03Z

    Hi,

To investigate an unrelated issue, I set up key logging in the backend (we
probably should build that in) and looked at the decrypted data.  And noticed
that just after TLS setup finished the server sends three packets in a row:

C->S: TLSv1.3: finished
C->S: TLSv1.3: application data (startup message)
S->C: TLSv1.3: new session ticket
S->C: TLSv1.3: new session ticket
S->C: TLSv1.3: application data (authentication ok, parameter status+)


We try to turn off session resumption, but not completely enough for 1.3:
       SSL_OP_NO_TICKET
           SSL/TLS supports two mechanisms for resuming sessions: session ids and stateless session tickets.

           When using session ids a copy of the session information is cached on the server and a unique id is sent to the client. When the client  wishes  to
           resume it provides the unique id so that the server can retrieve the session information from its cache.

           When  using  stateless  session  tickets the server uses a session ticket encryption key to encrypt the session information. This encrypted data is
           sent to the client as a "ticket". When the client wishes to resume it sends the encrypted data back to the server.  The  server  uses  its  key  to
           decrypt the data and resume the session. In this way the server can operate statelessly - no session information needs to be cached locally.

           The  TLSv1.3  protocol  only  supports  tickets and does not directly support session ids. However, OpenSSL allows two modes of ticket operation in
           TLSv1.3: stateful and stateless. Stateless tickets work the same way as in TLSv1.2 and below.  Stateful tickets  mimic  the  session  id  behaviour
           available  in TLSv1.2 and below.  The session information is cached on the server and the session id is wrapped up in a ticket and sent back to the
           client. When the client wishes to resume, it presents a ticket in the same way as for stateless tickets. The server can then extract the session id
           from the ticket and retrieve the session information from its cache.

           By default OpenSSL will use stateless tickets. The SSL_OP_NO_TICKET option will cause stateless tickets to not be issued. In TLSv1.2 and below this
           means no ticket gets sent to the client at all. In TLSv1.3 a stateful ticket will be sent. This is a server-side option only.

           In TLSv1.3 it  is  possible  to  suppress  all  tickets  (stateful  and  stateless)  from  being  sent  by  calling  SSL_CTX_set_num_tickets(3)  or
           SSL_set_num_tickets(3).


Note the second to last paragraph: Because we use SSL_OP_NO_TICKET we trigger
use of stateful tickets. Which afaict are never going to be useful, because we
don't share the necessary state.

I guess openssl really could have inferred this from the fact that we *do*
call SSL_CTX_set_session_cache_mode(SSL_SESS_CACHE_OFF), b


Seems we ought to use SSL_CTX_set_num_tickets() to prevent issuing the useless
tickets?



It seems like a buglet in openssl that it forces each session tickets to be
sent in its own packet (it does an explicit BIO_flush(), so even if we
buffered between openssl and OS, as I think we should, we'd still send it
separately), but I don't really understand most of this stuff.

Greetings,

Andres Freund

  2. Re: tls 1.3: sending multiple tickets

    Daniel Gustafsson <daniel@yesql.se> — 2024-06-18T13:11:33Z

    > On 17 Jun 2024, at 19:38, Andres Freund <andres@anarazel.de> wrote:

> Note the second to last paragraph: Because we use SSL_OP_NO_TICKET we trigger
> use of stateful tickets. Which afaict are never going to be useful, because we
> don't share the necessary state.

Nice catch, I learned something new today.  I was under the impression that the
flag turned of all tickets but clearly not.

> I guess openssl really could have inferred this from the fact that we *do*
> call SSL_CTX_set_session_cache_mode(SSL_SESS_CACHE_OFF), b

Every day with the OpenSSL API is an adventure.

> Seems we ought to use SSL_CTX_set_num_tickets() to prevent issuing the useless
> tickets?

Agreed, in 1.1.1 and above as the API was only introduced then.  LibreSSL added
the API in 3.5.4 but only for compatibility since it doesn't support TLS
tickets at all.

> It seems like a buglet in openssl that it forces each session tickets to be
> sent in its own packet (it does an explicit BIO_flush(), so even if we
> buffered between openssl and OS, as I think we should, we'd still send it
> separately), but I don't really understand most of this stuff.

I don't see anything in the RFCs so not sure.

The attached applies this, and I think this is backpatching material since we
arguably fail to do what we say in the code.  AFAIK we don't have a hard rule
against backpatching changes to autoconf/meson?

--
Daniel Gustafsson

  3. Re: tls 1.3: sending multiple tickets

    Heikki Linnakangas <hlinnaka@iki.fi> — 2024-07-24T05:44:16Z

    On 18/06/2024 16:11, Daniel Gustafsson wrote:
>> On 17 Jun 2024, at 19:38, Andres Freund <andres@anarazel.de> wrote:
>> Seems we ought to use SSL_CTX_set_num_tickets() to prevent issuing the useless
>> tickets?
> 
> Agreed, in 1.1.1 and above as the API was only introduced then.  LibreSSL added
> the API in 3.5.4 but only for compatibility since it doesn't support TLS
> tickets at all.

Wow, that's a bizarre API. The OpenSSL docs are not clear on what the 
possible values for SSL_CTX_set_num_tickets() are. It talks about 0, and 
mentions that 2 is the default, but what does it mean to set it to 1, or 
5, for example?

Anyway, it's pretty clear that SSL_CTX_set_num_tickets(0) can be used to 
disable tickets, so that's fine.

>> It seems like a buglet in openssl that it forces each session tickets to be
>> sent in its own packet (it does an explicit BIO_flush(), so even if we
>> buffered between openssl and OS, as I think we should, we'd still send it
>> separately), but I don't really understand most of this stuff.
> 
> I don't see anything in the RFCs so not sure.
> 
> The attached applies this, and I think this is backpatching material since we
> arguably fail to do what we say in the code.  AFAIK we don't have a hard rule
> against backpatching changes to autoconf/meson?

Looks good to me. Backpatching autoconf/meson changes is fine, we've 
done it before.

-- 
Heikki Linnakangas
Neon (https://neon.tech)

  4. Re: tls 1.3: sending multiple tickets

    Daniel Gustafsson <daniel@yesql.se> — 2024-07-26T11:55:29Z

    > On 24 Jul 2024, at 07:44, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> 
> On 18/06/2024 16:11, Daniel Gustafsson wrote:
>>> On 17 Jun 2024, at 19:38, Andres Freund <andres@anarazel.de> wrote:
>>> Seems we ought to use SSL_CTX_set_num_tickets() to prevent issuing the useless
>>> tickets?
>> Agreed, in 1.1.1 and above as the API was only introduced then.  LibreSSL added
>> the API in 3.5.4 but only for compatibility since it doesn't support TLS
>> tickets at all.
> 
> Wow, that's a bizarre API. The OpenSSL docs are not clear on what the possible values for SSL_CTX_set_num_tickets() are. It talks about 0, and mentions that 2 is the default, but what does it mean to set it to 1, or 5, for example?

It means that 1 or 5 tickets can be sent to the user, OpenSSL accepts an
arbitrary number of tickets (tickets can be issued at other points during the
connection than the handshake AFAICT).

> Anyway, it's pretty clear that SSL_CTX_set_num_tickets(0) can be used to disable tickets, so that's fine.
> 
>>> It seems like a buglet in openssl that it forces each session tickets to be
>>> sent in its own packet (it does an explicit BIO_flush(), so even if we
>>> buffered between openssl and OS, as I think we should, we'd still send it
>>> separately), but I don't really understand most of this stuff.
>> I don't see anything in the RFCs so not sure.
>> The attached applies this, and I think this is backpatching material since we
>> arguably fail to do what we say in the code.  AFAIK we don't have a hard rule
>> against backpatching changes to autoconf/meson?
> 
> Looks good to me. Backpatching autoconf/meson changes is fine, we've done it before.

Thanks for review, I've applied this backpatched all the way.

--
Daniel Gustafsson

  5. Re: tls 1.3: sending multiple tickets

    Marina Polyakova <m.polyakova@postgrespro.ru> — 2024-07-26T12:03:31Z

    Hello!

On 2024-07-26 14:55, Daniel Gustafsson wrote:
> Thanks for review, I've applied this backpatched all the way.

It looks like the recommended way of using autoheader [1] is now broken. 
The attached patch fixes the master branch for me.

[1] 
https://www.postgresql.org/message-id/30511.1546097762%40sss.pgh.pa.us

-- 
Marina Polyakova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

  6. Re: tls 1.3: sending multiple tickets

    Daniel Gustafsson <daniel@yesql.se> — 2024-07-26T12:27:03Z

    > On 26 Jul 2024, at 14:03, Marina Polyakova <m.polyakova@postgrespro.ru> wrote:
> On 2024-07-26 14:55, Daniel Gustafsson wrote:
>> Thanks for review, I've applied this backpatched all the way.
> 
> It looks like the recommended way of using autoheader [1] is now broken. The attached patch fixes the master branch for me.

Thanks for the report, I'll fix it.  Buildfarm animal hamerkop also reminded me
that I had managed to stash the old MSVC buildsystem changes (ENOTENOUGHCOFFEE)
so fixing that at the same time.

--
Daniel Gustafsson

  7. Re: tls 1.3: sending multiple tickets

    Marina Polyakova <m.polyakova@postgrespro.ru> — 2024-07-26T12:47:01Z

    On 2024-07-26 15:27, Daniel Gustafsson wrote:
>> On 26 Jul 2024, at 14:03, Marina Polyakova 
>> <m.polyakova@postgrespro.ru> wrote:
>> It looks like the recommended way of using autoheader [1] is now 
>> broken. The attached patch fixes the master branch for me.
> 
> Thanks for the report, I'll fix it.  Buildfarm animal hamerkop also 
> reminded me
> that I had managed to stash the old MSVC buildsystem changes 
> (ENOTENOUGHCOFFEE)
> so fixing that at the same time.

Thank you!

-- 
Marina Polyakova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

  8. Re: tls 1.3: sending multiple tickets

    Robert Haas <robertmhaas@gmail.com> — 2024-07-26T14:08:20Z

    On Fri, Jul 26, 2024 at 8:27 AM Daniel Gustafsson <daniel@yesql.se> wrote:
> Thanks for the report, I'll fix it.  Buildfarm animal hamerkop also reminded me
> that I had managed to stash the old MSVC buildsystem changes (ENOTENOUGHCOFFEE)
> so fixing that at the same time.

I was just looking at this commit and noticing that nothing in the
commit message explains why we want to turn off tickets. So then I
looked at the comments in the patch and that didn't explain it either.
So then I read through this thread and that also didn't explain it.

I don't doubt that you're doing the right thing here but it'd be nice
to document why it's the right thing someplace.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  9. Re: tls 1.3: sending multiple tickets

    Daniel Gustafsson <daniel@yesql.se> — 2024-07-26T14:23:41Z

    > On 26 Jul 2024, at 16:08, Robert Haas <robertmhaas@gmail.com> wrote:
> 
> On Fri, Jul 26, 2024 at 8:27 AM Daniel Gustafsson <daniel@yesql.se> wrote:
>> Thanks for the report, I'll fix it.  Buildfarm animal hamerkop also reminded me
>> that I had managed to stash the old MSVC buildsystem changes (ENOTENOUGHCOFFEE)
>> so fixing that at the same time.
> 
> I was just looking at this commit and noticing that nothing in the
> commit message explains why we want to turn off tickets. So then I
> looked at the comments in the patch and that didn't explain it either.
> So then I read through this thread and that also didn't explain it.

Sorry for the lack of detail, I probably Stockholm syndromed myself after
having spent some time in this code.

We turn off TLS session tickets for two reasons: a) we don't support TLS
session resumption, and some resumption capable client libraries can experience
connection failures if they try to use tickets received in the setup (Npgsql at
least had this problem in the past); b) it's network overhead in the connection
setup phase which doesn't give any value due to us not supporting their use.

TLS tickets were disallowed in 2017 in 97d3a0b09 but as Andres found out,
TLSv1.3 session tickets had a new API which we didn't call and thus issued
tickets.

> I don't doubt that you're doing the right thing here but it'd be nice
> to document why it's the right thing someplace.

I can add a summary of the above in the comment for future readers if you think
that would be useful.

--
Daniel Gustafsson

  10. Re: tls 1.3: sending multiple tickets

    Robert Haas <robertmhaas@gmail.com> — 2024-07-26T18:29:49Z

    On Fri, Jul 26, 2024 at 10:23 AM Daniel Gustafsson <daniel@yesql.se> wrote:
> We turn off TLS session tickets for two reasons: a) we don't support TLS
> session resumption, and some resumption capable client libraries can experience
> connection failures if they try to use tickets received in the setup (Npgsql at
> least had this problem in the past); b) it's network overhead in the connection
> setup phase which doesn't give any value due to us not supporting their use.
>
> TLS tickets were disallowed in 2017 in 97d3a0b09 but as Andres found out,
> TLSv1.3 session tickets had a new API which we didn't call and thus issued
> tickets.

Thanks much for this explanation.

> > I don't doubt that you're doing the right thing here but it'd be nice
> > to document why it's the right thing someplace.
>
> I can add a summary of the above in the comment for future readers if you think
> that would be useful.

I think having (a) and (b) from above at the end of the "Disallow SSL
session tickets" comment would be helpful.

While I'm complaining, the bit about SSL renegotiation could use a
better comment, too. One of my chronic complaints about comments is
that they should say why we're doing things, not what we're doing. To
me, having a comment that says "Disallow SSL renegotiation" followed
immediately by SSL_CTX_set_options(context, SSL_OP_NO_RENEGOTIATION)
is a good example of what not to do, because, I mean, I can guess
without the comment what that does. Actually, that comment is quite
well-written in terms of explaining why we do it in different ways
depending on the OpenSSL version; it just fails to explain why it's
important in the first place. I'm pretty sure I know in that case that
there are CVEs about that topic, but that's just because I happen to
remember the mailing list discussion on it, and I don't think every
hacker is contractually required to remember that.

I don't want to sound like I'm giving orders and I think it's really
up to you how much effort you want to put in here, but I feel like any
place where we are doing X because of some property of a non-PG code
base with which a particular reader might not be familiar, we should
have a comment explaining why we're doing it. And especially if it's
security-relevant.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  11. Re: tls 1.3: sending multiple tickets

    Daniel Gustafsson <daniel@yesql.se> — 2024-07-29T09:56:55Z

    > On 26 Jul 2024, at 20:29, Robert Haas <robertmhaas@gmail.com> wrote:

> One of my chronic complaints about comments is
> that they should say why we're doing things, not what we're doing.

Agreed.

> I feel like any
> place where we are doing X because of some property of a non-PG code
> base with which a particular reader might not be familiar, we should
> have a comment explaining why we're doing it. And especially if it's
> security-relevant.

I'm sure there are more interactions with OpenSSL, and TLS in general, which
warrants better comments but the attached takes a stab at the two examples in
question here to get started (to avoid perfect get in the way of progress). 

--
Daniel Gustafsson

  12. Re: tls 1.3: sending multiple tickets

    Robert Haas <robertmhaas@gmail.com> — 2024-07-29T15:34:39Z

    On Mon, Jul 29, 2024 at 5:57 AM Daniel Gustafsson <daniel@yesql.se> wrote:
> I'm sure there are more interactions with OpenSSL, and TLS in general, which
> warrants better comments but the attached takes a stab at the two examples in
> question here to get started (to avoid perfect get in the way of progress).

+1.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  13. Re: tls 1.3: sending multiple tickets

    Andres Freund <andres@anarazel.de> — 2024-07-29T20:08:53Z

    On 2024-07-26 13:55:29 +0200, Daniel Gustafsson wrote:
> Thanks for review, I've applied this backpatched all the way.

Thanks for working on this!