Thread: pgsql: Rework SSL renegotiation code

pgsql: Rework SSL renegotiation code

From
Alvaro Herrera
Date:
Rework SSL renegotiation code

The existing renegotiation code was home for several bugs: it might
erroneously report that renegotiation had failed; it might try to
execute another renegotiation while the previous one was pending; it
failed to terminate the connection if the renegotiation never actually
took place; if a renegotiation was started, the byte count was reset,
even if the renegotiation wasn't completed (this isn't good from a
security perspective because it means continuing to use a session that
should be considered compromised due to volume of data transferred.)

The new code is structured to avoid these pitfalls: renegotiation is
started a little earlier than the limit has expired; the handshake
sequence is retried until it has actually returned successfully, and no
more than that, but if it fails too many times, the connection is
closed.  The byte count is reset only when the renegotiation has
succeeded, and if the renegotiation byte count limit expires, the
connection is terminated.

This commit only touches the master branch, because some of the changes
are controversial.  If everything goes well, a back-patch might be
considered.

Per discussion started by message
20130710212017.GB4941@eldon.alvh.no-ip.org

Branch
------
master

Details
-------
http://git.postgresql.org/pg/commitdiff/31cf1a1a43c45e53d9bb3134c07f92e722f097f9

Modified Files
--------------
src/backend/libpq/be-secure.c |   82 ++++++++++++++++++++++++++++++++---------
src/backend/utils/misc/guc.c  |    1 -
src/include/libpq/libpq-be.h  |    5 +++
3 files changed, 70 insertions(+), 18 deletions(-)


Re: pgsql: Rework SSL renegotiation code

From
Jaime Casanova
Date:
On Thu, Oct 10, 2013 at 9:47 PM, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> Rework SSL renegotiation code
>

Hi Álvaro,

Shouldn't this new variable be declared inside the #ifdef USE_SSL block?
My compiler is giving me a warning for the unused variable

--- a/src/backend/libpq/be-secure.c
+++ b/src/backend/libpq/be-secure.c
@@ -101,6 +101,9 @@ char       *ssl_crl_file;
  */
 int            ssl_renegotiation_limit;

+/* are we in the middle of a renegotiation? */
+static bool in_ssl_renegotiation = false;
+
 #ifdef USE_SSL
 static SSL_CTX *SSL_context = NULL;
 static bool ssl_loaded_verify_locations = false;


--
Jaime Casanova         www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación
Phone: +593 4 5107566         Cell: +593 987171157


Re: pgsql: Rework SSL renegotiation code

From
Alvaro Herrera
Date:
Vik Fearing wrote:
> On 09/23/2013 10:51 PM, Alvaro Herrera wrote:
> > + /* are we in the middle of a renegotiation? */
> > + static bool in_ssl_renegotiation = false;
>
> Since this was committed, I'm getting the following warning:
>
> be-secure.c:105:13: warning: ‘in_ssl_renegotiation’ defined but not used
> [-Wunused-variable]

Jaime Casanova wrote:

> Shouldn't this new variable be declared inside the #ifdef USE_SSL block?
> My compiler is giving me a warning for the unused variable

Yep, thanks guys.  Just pushed a fix.

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