Обсуждение: Docs: Encourage strong server verification with SCRAM
Hi all, As touched on in past threads, our SCRAM implementation is slightly nonstandard and doesn't always protect the entirety of the authentication handshake: - the username in the startup packet is not covered by the SCRAM crypto and can be tampered with if channel binding is not in effect, or by an attacker holding only the server's key - low iteration counts accepted by the client make it easier than it probably should be for a MITM to brute-force passwords (note that PG16's scram_iterations GUC, being server-side, does not mitigate this) - by default, a SCRAM exchange can be exited by the server prior to sending its verifier, skipping the client's server authentication step (this is mitigated by requiring channel binding, and PG16 adds require_auth=scram-sha-256 to help as well) These aren't currently considered security vulnerabilities, but it'd be good for the documentation to call them out, considering mutual authentication is one of the design goals of the SCRAM RFC. (I'd also like to shore up these problems, eventually, to make SCRAM-based mutual authn viable with Postgres. But that work has stalled a bit on my end.) Here's a patch to explicitly warn people away from SCRAM as a form of server authentication, and nudge them towards a combination with verified TLS or gssenc. I've tried to keep the text version-agnostic, to make a potential backport easier. Is this a good place for the warning to go? Should I call out that GSS can't use channel binding, or promote the use of TLS versus GSS for SCRAM, or just keep it simple? Thanks, --Jacob
Вложения
Greetings, * Jacob Champion (jchampion@timescale.com) wrote: > As touched on in past threads, our SCRAM implementation is slightly > nonstandard and doesn't always protect the entirety of the > authentication handshake: > > - the username in the startup packet is not covered by the SCRAM > crypto and can be tampered with if channel binding is not in effect, > or by an attacker holding only the server's key Encouraging channel binding when using TLS certainly makes a lot of sense, particularly in environments where the trust anchors are not strongly managed (that is- if you trust the huge number of root certificates that a system may have installed). Perhaps explicitly encouraging users to *not* trust every root server installed on their client for their PG connections would also be a good idea. We should probably add language to that effect to this section. We do default to having channel binding being used though. Breaking down exactly the cases at risk (perhaps including what version certain things were introduced) and what direct action administrators can take to ensure their systems are as secure as possible (and maybe mention what things that doesn't address still, so they're aware of what risks still exist) would be good. > - low iteration counts accepted by the client make it easier than it > probably should be for a MITM to brute-force passwords (note that > PG16's scram_iterations GUC, being server-side, does not mitigate > this) This would be good to improve on. > - by default, a SCRAM exchange can be exited by the server prior to > sending its verifier, skipping the client's server authentication step > (this is mitigated by requiring channel binding, and PG16 adds > require_auth=scram-sha-256 to help as well) Yeah, encouraging this would also be good and should be mentioned as something to do when one is using SCRAM. Clearly that would go into a PG16 version of this. > These aren't currently considered security vulnerabilities, but it'd > be good for the documentation to call them out, considering mutual > authentication is one of the design goals of the SCRAM RFC. (I'd also > like to shore up these problems, eventually, to make SCRAM-based > mutual authn viable with Postgres. But that work has stalled a bit on > my end.) Improving the documentation certainly is a good plan. > Here's a patch to explicitly warn people away from SCRAM as a form of > server authentication, and nudge them towards a combination with > verified TLS or gssenc. I've tried to keep the text version-agnostic, > to make a potential backport easier. Is this a good place for the > warning to go? Should I call out that GSS can't use channel binding, > or promote the use of TLS versus GSS for SCRAM, or just keep it > simple? Adding channel binding to GSS (which does support it, we just don't implement it today..) when using TLS or another encryption method for transport would be a good improvement to make, particularly right now as we don't yet support encryption with SSPI, meaning that you need to use TLS to get session encryption when one of the systems is on Windows. I do hope to add support for encryption with SSPI during this next release cycle and would certainly welcome help from anyone else who is interested in this. I have to admit that the patch as presented strikes me as a warning without really providing steps for how to address the issues mentioned though; there's a reference to what was just covered at the bottom but nothing really new. The current documentation leads with the warnings and then goes into steps to take to address the risk, and I'd say it would make more sense to put this wording about SCRAM not being a complete solution to this issue above the steps that one can take to reduce the spoofing risk, similar to how the documentation currently is. Perhaps more succinctly- maybe we should be making adjustments to the current language instead of just adding a new paragraph. Thanks, Stephen
Вложения
On Tue, May 23, 2023 at 05:02:50PM -0400, Stephen Frost wrote: > * Jacob Champion (jchampion@timescale.com) wrote: >> As touched on in past threads, our SCRAM implementation is slightly >> nonstandard and doesn't always protect the entirety of the >> authentication handshake: >> >> - the username in the startup packet is not covered by the SCRAM >> crypto and can be tampered with if channel binding is not in effect, >> or by an attacker holding only the server's key > > Encouraging channel binding when using TLS certainly makes a lot of > sense, particularly in environments where the trust anchors are not > strongly managed (that is- if you trust the huge number of root > certificates that a system may have installed). Perhaps explicitly > encouraging users to *not* trust every root server installed on their > client for their PG connections would also be a good idea. We should > probably add language to that effect to this section. Currently the user name is not treated by the backend, like that in auth-scram.c: /* * Read username. Note: this is ignored. We use the username from the * startup message instead, still it is kept around if provided as it * proves to be useful for debugging purposes. */ state->client_username = read_attr_value(&p, 'n'); Could it make sense to cross-check that with the contents of the startup package instead, with or without channel binding? >> - low iteration counts accepted by the client make it easier than it >> probably should be for a MITM to brute-force passwords (note that >> PG16's scram_iterations GUC, being server-side, does not mitigate >> this) > > This would be good to improve on. Hmm. Interesting. > > - by default, a SCRAM exchange can be exited by the server prior to > > sending its verifier, skipping the client's server authentication step > > (this is mitigated by requiring channel binding, and PG16 adds > > require_auth=scram-sha-256 to help as well) > > Yeah, encouraging this would also be good and should be mentioned as > something to do when one is using SCRAM. Clearly that would go into a > PG16 version of this. Agreed to improve the docs about ways to mitigate any risks. -- Michael
Вложения
Greetings, * Michael Paquier (michael@paquier.xyz) wrote: > On Tue, May 23, 2023 at 05:02:50PM -0400, Stephen Frost wrote: > > * Jacob Champion (jchampion@timescale.com) wrote: > >> As touched on in past threads, our SCRAM implementation is slightly > >> nonstandard and doesn't always protect the entirety of the > >> authentication handshake: > >> > >> - the username in the startup packet is not covered by the SCRAM > >> crypto and can be tampered with if channel binding is not in effect, > >> or by an attacker holding only the server's key > > > > Encouraging channel binding when using TLS certainly makes a lot of > > sense, particularly in environments where the trust anchors are not > > strongly managed (that is- if you trust the huge number of root > > certificates that a system may have installed). Perhaps explicitly > > encouraging users to *not* trust every root server installed on their > > client for their PG connections would also be a good idea. We should > > probably add language to that effect to this section. > > Currently the user name is not treated by the backend, like that in > auth-scram.c: > > /* > * Read username. Note: this is ignored. We use the username from the > * startup message instead, still it is kept around if provided as it > * proves to be useful for debugging purposes. > */ > state->client_username = read_attr_value(&p, 'n'); > > Could it make sense to cross-check that with the contents of the > startup package instead, with or without channel binding? Not without breaking things we support today and for what seems like an unclear benefit given that we've got channel binding today (though perhaps we need to make sure there's ways to force it on both sides to be on and to encourage everyone to do that- which is what this change is generally about, I think). As I recall, the reason we do it the way we do is because the SASL spec that SCRAM is implemented under requires the username to be utf8 encoded while we support other encodings, and I don't think we want to break non-utf8 usage. Thanks, Stephen
Вложения
On Tue, May 23, 2023 at 09:46:58PM -0400, Stephen Frost wrote: > Not without breaking things we support today and for what seems like an > unclear benefit given that we've got channel binding today (though > perhaps we need to make sure there's ways to force it on both sides to > be on and to encourage everyone to do that- which is what this change is > generally about, I think). > > As I recall, the reason we do it the way we do is because the SASL spec > that SCRAM is implemented under requires the username to be utf8 encoded > while we support other encodings, and I don't think we want to break > non-utf8 usage. Yup, I remember this one, the encoding not being enforced by the protocol has been quite an issue when this was implemented, still I was wondering whether that's something that could be worth revisiting at some degree. -- Michael
Вложения
Greetings, * Michael Paquier (michael@paquier.xyz) wrote: > On Tue, May 23, 2023 at 09:46:58PM -0400, Stephen Frost wrote: > > Not without breaking things we support today and for what seems like an > > unclear benefit given that we've got channel binding today (though > > perhaps we need to make sure there's ways to force it on both sides to > > be on and to encourage everyone to do that- which is what this change is > > generally about, I think). > > > > As I recall, the reason we do it the way we do is because the SASL spec > > that SCRAM is implemented under requires the username to be utf8 encoded > > while we support other encodings, and I don't think we want to break > > non-utf8 usage. > > Yup, I remember this one, the encoding not being enforced by the > protocol has been quite an issue when this was implemented, still I > was wondering whether that's something that could be worth revisiting > at some degree. To the extent that there was an issue when it was implemented ... it's now been implemented and so that was presumably overcome (though I don't really specifically recall what the issues were there? Seems like it wouldn't matter at this point though), so I don't understand why we would wish to revisit it. Thanks, Stephen
Вложения
On Tue, May 23, 2023 at 10:01:03PM -0400, Stephen Frost wrote: > To the extent that there was an issue when it was implemented ... it's > now been implemented and so that was presumably overcome (though I don't > really specifically recall what the issues were there? Seems like it > wouldn't matter at this point though), so I don't understand why we > would wish to revisit it. Well, there are IMO two sides issues here: 1) libpq sends an empty username, which can be an issue if attempting to make this layer more pluggable with other things that are more compilation than PG with the SCRAM exchange (OpenLDAP is one, there could be others). 2) The backend ignoring the username means that it is not mixed in the hashes. Relying on channel binding mitigates the range of problems for 2), now one thing is that the default sslmode does not enforce an SSL connection, either, though this default has been discussed a lot. So I am really wondering whether there wouldn't be some room for a more compliant, strict HBA option with scram-sha-256 where we'd only allow UTF-8 compliant strings. Just some food for thoughts. And we could do better about the current state that's 1), anyway. -- Michael
Вложения
> On 23 May 2023, at 23:02, Stephen Frost <sfrost@snowman.net> wrote: > * Jacob Champion (jchampion@timescale.com) wrote: >> - low iteration counts accepted by the client make it easier than it >> probably should be for a MITM to brute-force passwords (note that >> PG16's scram_iterations GUC, being server-side, does not mitigate >> this) > > This would be good to improve on. The mechanics of this are quite straighforward, the problem IMHO lies in how to inform and educate users what a reasonable iteration count is, not to mention what an iteration count is in the first place. > Perhaps more succinctly- maybe we should be making adjustments to the > current language instead of just adding a new paragraph. +1 -- Daniel Gustafsson
On 5/24/23 05:04, Daniel Gustafsson wrote: >> On 23 May 2023, at 23:02, Stephen Frost <sfrost@snowman.net> wrote: >> * Jacob Champion (jchampion@timescale.com) wrote: > >>> - low iteration counts accepted by the client make it easier than it >>> probably should be for a MITM to brute-force passwords (note that >>> PG16's scram_iterations GUC, being server-side, does not mitigate >>> this) >> >> This would be good to improve on. > > The mechanics of this are quite straighforward, the problem IMHO lies in how to > inform and educate users what a reasonable iteration count is, not to mention > what an iteration count is in the first place. Yeah, the education around this is tricky. >> Perhaps more succinctly- maybe we should be making adjustments to the >> current language instead of just adding a new paragraph. > > +1 I'm 100% on board for doing that as well, but the "instead of" suggestion makes me think I didn't explain my goal very well. For example, Stephen, you said > I have to admit that the patch as presented strikes me as a warning > without really providing steps for how to address the issues mentioned > though; there's a reference to what was just covered at the bottom but > nothing really new. but the last sentence of my patch *is* the suggested step: > + ... It's recommended to employ strong server > + authentication, using one of the above anti-spoofing measures, to prevent > + these attacks. In other words: if you're thinking about authenticating the server via SCRAM, over an otherwise anonymous key exchange, you should probably reconsider. The current architecture of libpq doesn't really seem to be set up for this, and my conversations with security@ have been focusing around the argument that people who want strong guarantees should be authenticating the server using a TLS certificate before doing anything else, so we don't need to consider our departures from the spec to be vulnerabilities. I think that's a reasonable stance to take, as long as we're also explicitly warning people away from the mutual-auth use case. To change this, aspects of the code that we don't currently consider security problems would probably need to be reclassified. If we're depending on SCRAM for server authentication, we can't trust a single byte that the server sends to us until after the SCRAM verifier comes back and checks out. Compared to all the other authentication types, that's unusual; I don't think it's really been a focus for the project before. --Jacob
On 5/23/23 21:37, Michael Paquier wrote: > On Tue, May 23, 2023 at 10:01:03PM -0400, Stephen Frost wrote: >> To the extent that there was an issue when it was implemented ... it's >> now been implemented and so that was presumably overcome (though I don't >> really specifically recall what the issues were there? Seems like it >> wouldn't matter at this point though), so I don't understand why we >> would wish to revisit it. > > Well, there are IMO two sides issues here: > 1) libpq sends an empty username, which can be an issue if attempting > to make this layer more pluggable with other things that are more > compilation than PG with the SCRAM exchange (OpenLDAP is one, there > could be others). > 2) The backend ignoring the username means that it is not mixed in the > hashes. +1 > Relying on channel binding mitigates the range of problems for 2), (Except for username tampering with possession of the server key alone. As spec'd, that shouldn't be possible. But for the vast majority of users, I think that's probably not on the list of plausible concerns.) > now > one thing is that the default sslmode does not enforce an SSL > connection, either, though this default has been discussed a lot. So > I am really wondering whether there wouldn't be some room for a more > compliant, strict HBA option with scram-sha-256 where we'd only allow > UTF-8 compliant strings. Just some food for thoughts. > > And we could do better about the current state that's 1), anyway. I would definitely like to see improvements in this area. I don't think we'd need to break compatibility with clients, either (unless the server operator explicitly wanted to). The hard part is mainly on the server side, when dealing with a mismatch between the startup packet and the SCRAM header. --Jacob
Greetings, * Jacob Champion (jchampion@timescale.com) wrote: > On 5/24/23 05:04, Daniel Gustafsson wrote: > >> On 23 May 2023, at 23:02, Stephen Frost <sfrost@snowman.net> wrote: > >> Perhaps more succinctly- maybe we should be making adjustments to the > >> current language instead of just adding a new paragraph. > > > > +1 > > I'm 100% on board for doing that as well, but the "instead of" > suggestion makes me think I didn't explain my goal very well. For > example, Stephen, you said > > > I have to admit that the patch as presented strikes me as a warning > > without really providing steps for how to address the issues mentioned > > though; there's a reference to what was just covered at the bottom but > > nothing really new. > > but the last sentence of my patch *is* the suggested step: > > > + ... It's recommended to employ strong server > > + authentication, using one of the above anti-spoofing measures, to prevent > > + these attacks. I was referring specifically to that ordering as not being ideal or in line with the rest of the flow of that section. We should integrate the concerns higher in the section where we outline the reason these things matter and then follow those with the specific steps for how to address them, not give a somewhat unclear reference from the very bottom back up to something above. Thanks, Stephen
Вложения
On Thu, May 25, 2023 at 10:29 AM Stephen Frost <sfrost@snowman.net> wrote: > I was referring specifically to that ordering as not being ideal or in > line with the rest of the flow of that section. We should integrate the > concerns higher in the section where we outline the reason these things > matter and then follow those with the specific steps for how to address > them, not give a somewhat unclear reference from the very bottom back up > to something above. Ah, I misunderstood. I'll give that a shot. Thanks! --Jacob
On 5/25/23 1:29 PM, Stephen Frost wrote: > Greetings, > > * Jacob Champion (jchampion@timescale.com) wrote: >> On 5/24/23 05:04, Daniel Gustafsson wrote: >>>> On 23 May 2023, at 23:02, Stephen Frost <sfrost@snowman.net> wrote: >>>> Perhaps more succinctly- maybe we should be making adjustments to the >>>> current language instead of just adding a new paragraph. >>> >>> +1 >> >> I'm 100% on board for doing that as well, but the "instead of" >> suggestion makes me think I didn't explain my goal very well. For >> example, Stephen, you said >> >>> I have to admit that the patch as presented strikes me as a warning >>> without really providing steps for how to address the issues mentioned >>> though; there's a reference to what was just covered at the bottom but >>> nothing really new. >> >> but the last sentence of my patch *is* the suggested step: >> >>> + ... It's recommended to employ strong server >>> + authentication, using one of the above anti-spoofing measures, to prevent >>> + these attacks. > > I was referring specifically to that ordering as not being ideal or in > line with the rest of the flow of that section. We should integrate the > concerns higher in the section where we outline the reason these things > matter and then follow those with the specific steps for how to address > them, not give a somewhat unclear reference from the very bottom back up > to something above. Caught up on this exchange. Some of my comments may be out-of-order. Overall, +1 to tightening the language around the docs in this area. However, to paraphrase Stephen, I think the language, as currently written, makes the problem sound scarier than it actually is, and I agree that we should just inline it above. There may also be some follow-up development work we can do to mitigate the issues. I think it's fine for us to recommend using channel binding, but if we're concerned about server spoofing / rogue servers, we should also recommend using sslmode=verify-full to ensure server-identity. That doesn't necessarily help in the case the server itself has gone rogue, but that mitigates the MITM risk. The SCRAM RFC is also very clear that you should be using TLS[1]. I really don't think the "startup packet" is an actual issue, but I think recommending good TLS / channel binding mitigates this. For the iteration count, I'm generally ambivalent here. I think again, if you're using good TLS, this is most likely mitigated. If you're connecting to a rogue server using good TLS, you likely have other issues to deal with. However, there may be a libpq feature here that lets a client specify a minimum iteration count it will accept for purposes of SCRAM. Thanks, Jonathan [1] https://www.rfc-editor.org/rfc/rfc5802#section-9
Вложения
On Thu, May 25, 2023 at 10:48 AM Jonathan S. Katz <jkatz@postgresql.org> wrote: > Overall, +1 to tightening the language around the docs in this area. > > However, to paraphrase Stephen, I think the language, as currently > written, makes the problem sound scarier than it actually is, and I > agree that we should just inline it above. How does v2 look? I more or less divided the current text into a local section and a network section. (I'm still not clear on where in the current text you're wanting me to inline a sudden aside on SCRAM; it doesn't really seem to fit in any of the existing paragraphs.) --Jacob
Вложения
On 5/25/23 3:27 PM, Jacob Champion wrote: > On Thu, May 25, 2023 at 10:48 AM Jonathan S. Katz <jkatz@postgresql.org> wrote: >> Overall, +1 to tightening the language around the docs in this area. >> >> However, to paraphrase Stephen, I think the language, as currently >> written, makes the problem sound scarier than it actually is, and I >> agree that we should just inline it above. > > How does v2 look? I more or less divided the current text into a local > section and a network section. (I'm still not clear on where in the > current text you're wanting me to inline a sudden aside on SCRAM; it > doesn't really seem to fit in any of the existing paragraphs.) I read through the proposal and like this much better. I missed Stephen's point on the "where" to put it in this section; I actually don't know if I agree with that (he says while painting the bikeshed), given the we spend two paragraphs describing how to prevent spoofing in general over the network, vs. just during SCRAM authentication. I rewrote this to just focus on server spoofing that can occur with SCRAM authentication and did some wordsmithing. I was torn on keeping in the part of offline analysis of an intercepted hash, given one can do this with md5 as well, but perhaps it helps elaborate on the consequences. Thanks, Jonathan
Вложения
On Thu, May 25, 2023 at 6:10 PM Jonathan S. Katz <jkatz@postgresql.org> wrote: > I read through the proposal and like this much better. Great! > I rewrote this to just focus on server spoofing that can occur with > SCRAM authentication and did some wordsmithing. I was torn on keeping in > the part of offline analysis of an intercepted hash, given one can do > this with md5 as well, but perhaps it helps elaborate on the consequences. This part: > + To prevent server spoofing from occurring when using > + <link linkend="auth-password">scram-sha-256</link> password authentication > + over a network, you should ensure you are connecting using SSL. seems to backtrack on the recommendation -- you have to use sslmode=verify-full, not just SSL, to avoid handing a weak(er) hash to an untrusted party. --Jacob
On 5/26/23 6:47 PM, Jacob Champion wrote: > On Thu, May 25, 2023 at 6:10 PM Jonathan S. Katz <jkatz@postgresql.org> wrote: >> + To prevent server spoofing from occurring when using >> + <link linkend="auth-password">scram-sha-256</link> password authentication >> + over a network, you should ensure you are connecting using SSL. > > seems to backtrack on the recommendation -- you have to use > sslmode=verify-full, not just SSL, to avoid handing a weak(er) hash to > an untrusted party. The above assumes that the reader reviewed the previous paragraph and followed the guidelines there. However, we can make it explicit. Please see attached. Thanks, Jonathan
Вложения
On Sun, May 28, 2023 at 02:21:53PM -0400, Jonathan S. Katz wrote: > The above assumes that the reader reviewed the previous paragraph and > followed the guidelines there. However, we can make it explicit. Please see > attached. Yeah, I was under the same impression as Jacob that we don't insist enough in this new paragraph about what sslmode ought to be when I initially read the patch. However, looking at the html page produced, what you have to refer to the previous paragraph looks OK to me. -- Michael
Вложения
On Sun, May 28, 2023 at 2:22 PM Jonathan S. Katz <jkatz@postgresql.org> wrote: > The above assumes that the reader reviewed the previous paragraph and > followed the guidelines there. However, we can make it explicit. Please > see attached. LGTM! Thanks, --Jacob
On Wed, May 31, 2023 at 10:08:39AM -0400, Jacob Champion wrote: > LGTM! Okay. Does anybody have any comments and/or objections? -- Michael
Вложения
> On 31 May 2023, at 23:14, Michael Paquier <michael@paquier.xyz> wrote: > On Wed, May 31, 2023 at 10:08:39AM -0400, Jacob Champion wrote: >> LGTM! > > Okay. Does anybody have any comments and/or objections? LGTM. As a small nitpick, I think this sentence is a little bit misleading: "..can use offline analysis to determine the hashed password from the client" It's true that an attacker kan use offline analysis but it makes it sound easier than it might be in practice. I would have written "to potentially determine". -- Daniel Gustafsson
On Thu, Jun 01, 2023 at 10:22:28AM +0200, Daniel Gustafsson wrote: > It's true that an attacker kan use offline analysis but it makes it sound > easier than it might be in practice. I would have written "to potentially > determine". Hmm. Okay by me. -- Michael
Вложения
On Thu, Jun 01, 2023 at 08:28:32AM -0400, Michael Paquier wrote: > Hmm. Okay by me. Took me some time to get back to it, but applied this way. -- Michael
Вложения
On Sat, Jun 3, 2023 at 2:50 PM Michael Paquier <michael@paquier.xyz> wrote: > Took me some time to get back to it, but applied this way. Thanks all! --Jacob
On 6/5/23 11:22 AM, Jacob Champion wrote: > On Sat, Jun 3, 2023 at 2:50 PM Michael Paquier <michael@paquier.xyz> wrote: >> Took me some time to get back to it, but applied this way. > > Thanks all! +1; thank you! Jonathan