Skip site navigation (1) Skip section navigation (2)

Re: SSL renegotiation

From: Troels Nielsen <bn(dot)troels(at)gmail(dot)com>
To: Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
Cc: Pg Hackers <pgsql-hackers(at)postgresql(dot)org>, Sean Chittenden <sean(at)chittenden(dot)org>
Subject: Re: SSL renegotiation
Date: 2013-07-11 01:38:02
Message-ID: CAOdE5WSyb4Y5sCwe1CqBYmLoK7ZAON3R7aBEa8OEX0LVnff_Og@mail.gmail.com (view raw or flat)
Thread:
Lists: pgsql-committerspgsql-hackers
Hi,

These are the relevant bits from Apache2.4's mod_ssl.

            SSL_renegotiate(ssl);
            SSL_do_handshake(ssl);

            if (SSL_get_state(ssl) != SSL_ST_OK) {
                ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02225)
                              "Re-negotiation request failed");
                ssl_log_ssl_error(SSLLOG_MARK, APLOG_ERR, r->server);

                r->connection->keepalive = AP_CONN_CLOSE;
                return HTTP_FORBIDDEN;
            }

            ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(02226)
                          "Awaiting re-negotiation handshake");

            /* XXX: Should replace setting state with SSL_renegotiate(ssl);
             * However, this causes failures in perl-framework currently,
             * perhaps pre-test if we have already negotiated?
             */
#ifdef OPENSSL_NO_SSL_INTERN
            SSL_set_state(ssl, SSL_ST_ACCEPT);
#else
            ssl->state = SSL_ST_ACCEPT;
#endif
            SSL_do_handshake(ssl);

            sslconn->reneg_state = RENEG_REJECT;

            if (SSL_get_state(ssl) != SSL_ST_OK) {
                ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02261)
                              "Re-negotiation handshake failed: "
                              "Not accepted by client!?");

                r->connection->keepalive = AP_CONN_CLOSE;
                return HTTP_FORBIDDEN;
            }

That code supports at least OpenSSL 0.9.7 and later.

Some explanation for it can be found here:

http://books.google.dk/books?id=IIqwAy4qEl0C&pg=PT184&lpg=PT184&dq=SSL_do_handshake&source=bl&ots=ma632U4vWv&sig=d2qqS0svhu4EwIZZaONdHoTulVI&hl=en&sa=X&ei=xdPdUczoDuf34QSzmoDQDg&ved=0CIIDEOgBMCo

The snippet there is probably the textbook implementation.

So the original code looks OK. Perhaps the check
on the return code of the first SSL_do_handshake (and SSL_renegotiate)
is unnecessary and may lead to unwarranted error messages, though.
And it may be wrong to continue the renegotiation if
the state is not SSL_ST_OK after the first SSL_do_handshake.

If the renegotiation fails, I suppose two things can be done:
1. Quit the connection
2. Carry on pretending nothing happened.

I think 2. is correct  in the vast majority of cases (as it looks like is
being done now).
And in that case: not resetting  port->count will make for a very slow
and awkward connection as new handshakes will be attempted again and again,
even if the other party persistently refuse to shake hands.

Kind Regards
Troels Nielsen


On Thu, Jul 11, 2013 at 12:34 AM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com>wrote:

> I think this block is better written as:
>
>         if (ssl_renegotiation_limit && port->count >
> ssl_renegotiation_limit * 1024L)
>         {
>             SSL_set_session_id_context(port->ssl, (void *) &SSL_context,
>                                        sizeof(SSL_context));
>             if (SSL_renegotiate(port->ssl) <= 0)
>                 ereport(COMMERROR,
>                         (errcode(ERRCODE_PROTOCOL_VIOLATION),
>                          errmsg("SSL renegotiation failure in
> renegotiate")));
>             else
>             {
>                 int    handshake;
>
>                 do {
>                     handshake = SSL_do_handshake(port->ssl);
>                     if (handshake <= 0)
>                         ereport(COMMERROR,
>                                 (errcode(ERRCODE_PROTOCOL_VIOLATION),
>                                  errmsg("SSL renegotiation failure in
> handshake, retrying")));
>                 } while (handshake <= 0);
>
>                 if (port->ssl->state != SSL_ST_OK)
>                     ereport(COMMERROR,
>                             (errcode(ERRCODE_PROTOCOL_VIOLATION),
>                              errmsg("SSL failed to send renegotiation
> request")));
>                 else
>                     port->count = 0;
>             }
>         }
>
> In other words, retry the SSL_do_handshake() until it reports OK, but
> not more than that; this seems to give better results in my (admittedly
> crude) experiments.  I am unsure about checking port->ssl->state after
> the handshake; it's probably pointless, really.
>
> In any case, the old code was calling SSL_do_handshake() even if
> SSL_renegotiate() failed; and it was resetting the port->count even if
> the handshake failed.  Both of these smell like bugs to me.
>
> --
> Álvaro Herrera                http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Training & Services
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

In response to

Responses

pgsql-hackers by date

Next:From: Prabakaran, VaishnaviDate: 2013-07-11 01:42:56
Subject: Re: Differences in WHERE clause of SELECT
Previous:From: Josh BerkusDate: 2013-07-11 01:08:09
Subject: Re: [PATCH] Add transforms feature

pgsql-committers by date

Next:From: Peter EisentrautDate: 2013-07-11 02:45:06
Subject: pgsql: doc: Replace link to pgFouine with pgBadger
Previous:From: Sean ChittendenDate: 2013-07-10 23:58:07
Subject: Re: [SPAM] SSL renegotiation

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group