Recent vendor SSL renegotiation patches break PostgreSQL
in [PgSql]
|
Prev: [HACKERS] Partial Page Writes documentaiton mention
Next: [COMMITTERS] pgsql: Assorted cleanups in preparation for using a map file to support
From: Magnus Hagander on 23 Feb 2010 06:03 2010/2/22 Tom Lane <tgl(a)sss.pgh.pa.us>: > Magnus Hagander <magnus(a)hagander.net> writes: >> 2010/2/22 Tom Lane <tgl(a)sss.pgh.pa.us>: >> You'd still have to turn it off on the server side if you have a >> *single* client that has the broken patch, but that's still a lot >> better than nothing. > > Well, if it's a GUC it can be set per-user or per-database, so there's > at least some hope of not having to turn it off for everyone. > >> Think it's worth taking a stab at? > > If you want to do it, I'd be fine with it. Seems easy enough, see attached. Comments? 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? (That would also for example allow npgsql to always set it to 0, if it's known to be broken) 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? -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
From: Magnus Hagander on 23 Feb 2010 06:04 2010/2/23 Albe Laurenz <laurenz.albe(a)wien.gv.at>: > Tom Lane wrote: >>>>> One way to deal with it would be to expose the whole renegotiation >>>>> setting as a user configuratble option. So they can set *when* we >>>>> renegotiate, which would also let them turn it off completely. >>>> >>>> Well, that might be a reasonable thing to do, because it's not just a >>>> temporary kluge (that we won't know when to remove) but is adding an >>>> arguably-useful-in-other-ways knob. >> >>> You'd still have to turn it off on the server side if you have a >>> *single* client that has the broken patch, but that's still a lot >>> better than nothing. >> >> Well, if it's a GUC it can be set per-user or per-database, so there's >> at least some hope of not having to turn it off for everyone. >> >> > Think it's worth taking a stab at? >> >> If you want to do it, I'd be fine with it. > > +1 > > That would help me with a different problem: > SSL renegotiation is broken with Npgsql, the cause is Bug 321325 > in the Mono security library > https://bugzilla.novell.com/show_bug.cgi?id=321325 > which does not look like it is ever going to be fixed. *ouch* > Up to now I have crippled SSL renegotiation in our servers with a patch, > because I figured that bad SSL is better than no SSL. Given the major security hole in the whole project, SSL without renegotiation was a *lot* more secure than SSL *with* renegotiation, until very recently :-) But patching the server is always annoying... -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Tom Lane on 24 Feb 2010 11:27 Magnus Hagander <magnus(a)hagander.net> writes: > 2010/2/22 Tom Lane <tgl(a)sss.pgh.pa.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Magnus Hagander on 24 Feb 2010 11:40 2010/2/24 Tom Lane <tgl(a)sss.pgh.pa.us>: > Magnus Hagander <magnus(a)hagander.net> writes: >> 2010/2/22 Tom Lane <tgl(a)sss.pgh.pa.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 Check. > Also, the comment attached to ssl_renegotiation_limit needs to be fixed > to mention that zero disables renegotiation. Check. > 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. Yeah, that's clearly wrong. The extern is there, but not the definition. Fixed, and will test. I personally find it highly annoying when a GUC goes away, so I'm all for always having them there. And I thought that was our policy for new ones, but I can't find a reference to it... > Also the xreflabel for the variable in the docs isn't consistent, > and you failed to mention the default value. You mean add _limit to it, right? I looked at the parameter next to it, and none of them include the default value in the description. But I see now that's the exception, rather than the rule. Fixed. >> 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. The use case would be for example npgsql (or npgsql clients) being able to disable it from the client side, because they know they can't deal with it. Even in the case that the server doesn't know that. >> 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. Ok, will add. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Tom Lane on 24 Feb 2010 11:47
Magnus Hagander <magnus(a)hagander.net> writes: > 2010/2/24 Tom Lane <tgl(a)sss.pgh.pa.us>: >> 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 :-(. > I personally find it highly annoying when a GUC goes away, so I'm all > for always having them there. And I thought that was our policy for > new ones, but I can't find a reference to it... I see that ssl_ciphers is made to go away when USE_SSL isn't set, so the most consistent thing in the near term would be to do the same. Revisiting the whole issue seems like not material for back-patching. >> Also the xreflabel for the variable in the docs isn't consistent, > You mean add _limit to it, right? Right. >> 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? > The use case would be for example npgsql (or npgsql clients) being > able to disable it from the client side, because they know they can't > deal with it. Even in the case that the server doesn't know that. Fair enough, USERSET it is then. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers |