Re: Recent vendor SSL renegotiation patches break PostgreSQL

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> 2010/2/22 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>> If you want to do it, I'd be fine with it.

> Seems easy enough, see attached. Comments?

Looks mostly okay to me, a few notes:

+ if (ssl_renegotiation_limit && port->count > ssl_renegotiation_limit * 1024)

You need "1024L" there to avoid risk of integer overflow when long is
wider than int --- MAX_KILOBYTES is set on the assumption that any such
multiplies will be done in long arithmetic. It doesn't matter that
count will never be wider than int, it can still get the wrong answer.

Also, the comment attached to ssl_renegotiation_limit needs to be fixed
to mention that zero disables renegotiation.

Also, the coding seems a bit confused about whether the
ssl_renegotiation_limit GUC exists when USE_SSL isn't set. I think we
have a project policy about whether GUCs should still exist when the
underlying support isn't compiled, but I forget what it is :-(.
Anyway you need to test that the patch compiles with USE_SSL off.

Also the xreflabel for the variable in the docs isn't consistent,
and you failed to mention the default value.

> This version is set to superuser only. It's a security related
> feature, so just letting a random user turn it off may be seen as
> wrong. On the other hand, this is just about the connection security,
> and if we have a malicious user on the other end, he can do a lot
> worse things than disable renegotiation (such as resending the
> plaintext after it's been decrypted).

> I'd therefore suggest we make it USERSET. Anything wrong in that discussion?

SUSET seems less surprising to me. I agree that it's hard to make
a concrete case for a user doing anything terribly bad with it,
but on the other hand is there much value in letting it be USERSET?
Anyway it's not very important to me either way.

> Also, do we want to add a specific <note> to the documentation saying
> this is the way around broken SSL libraries? Or leave that to release
> notes? Or just leave it to the mailinglist archives?

I think a short note in the description of the variable wouldn't be out
of place.

regards, tom lane