Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

XEP-0388: Rework whole spec, namespace bump #1214

Merged
merged 3 commits into from
Mar 31, 2023
Merged

Conversation

tmolitor-stud-tu
Copy link
Contributor

This is a collaboration between @mwild1 and me. Most parts have already been discussed with the original author @dwd, too.

In short, this adds:

  • Namespace bump
  • Make SCRAM hashes (and other SASL mechanism families) upgradable
  • Add dependency on XEP-0440 for channel-binding agility
  • Add new <inline/> element as extension point to inline stream resumption and bind2 (and possibly others)
  • Clarify of <continue/> and tasks in general
  • Update security considerations and business rules
  • Add optional <user-agent/> element

@horazont
Copy link
Contributor

@dwd Ack please?

@horazont horazont added the Needs Author The XEP is experimental and the PR was not made by the author. The author needs to acknowledge it. label Oct 12, 2022
@tmolitor-stud-tu
Copy link
Contributor Author

For reference, here is the rendered version: https://dyn.eightysoft.de/final/xep-0388.html

Copy link
Contributor

@dwd dwd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a lot of changes to digest here, beyond even those we discussed (and I reserve my right to change my mind even when we did discuss!).

I don't think this is sensible to publish yet, and certainly not without extensible discussion on the list.

xep-0388.xml Outdated
<ul>
<li>Bump namespace</li>
<li>Make SCRAM hashes (and other SASL mechanism families) upgradable</li>
<li>Add dependency on &xep0440; and &rfc5802;</li>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not entirely sure this is right. Yes, both of these are very much sensible things to do, but realistically the only reason I can think of to make them a formal dependency is for MTI reasons, which I don't think are desirable to mess with in this specification.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"dependency" is meant as "refers to". I'll change that to say Add reference to &xep0440; and &rfc5802;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dwd does that solve your concerns?

xep-0388.xml Show resolved Hide resolved
xep-0388.xml Outdated Show resolved Hide resolved
xep-0388.xml Show resolved Hide resolved
xep-0388.xml Show resolved Hide resolved
xep-0388.xml Outdated Show resolved Hide resolved
<p>SASL security layers are sparingly used in the field, however, so this is thought to be a theoretical, rather than practical, concern.</p>
<p>As explained in § 13.8.3 of &rfc6120;, servers and clients SHOULD NOT support SASL PLAIN unless it is required by the authentication backend. Usage of PLAIN allows for trivial downgrade atacks by Man-In-The-Middle attackers circumventing any possible channel-binding or mutual authentication and even allowing the attacker to gain access to the cleartext password. If possible, PLAIN should be disabled by default on servers and clients and only be enabled when configured by the admin respective client user (if at all).</p>
<p>This protocol allows the client to provide a hint of its identity prior to authentication. This value MUST NOT be trusted to contain the real identity of the connecting client. Varying pre-authentication responses (such as supported SASL mechanisms) based on this value may allow unauthorized parties to discover whether an account exists, and even guess at what software might be used with that account (e.g. by observing whether the client has upgraded to specific mechanisms). Minimizing differences between the default response for unregistered JIDs and registered JIDs, as well as rate-limiting (to prevent enumeration attempts), are possible countermeasures. If the server does not know the client-identity it is RECOMMENDED to randomize the list of provided SASL mechanisms to reduce these risks even further.</p>
<p>SASL2 MUST only be used by Clients or offered by Servers after TLS negotiation, see § 5.2 and § 5.3.4 of &rfc6120;.</p>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd actually specify this as a mitigation to the first three points.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Obligatory comment that they are other methods to secure a stream than TLS.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iNPUTmice RFC 6120 contains the same wording for SASL1.

That's the reason I wanted to state the same for SASL2 as well.

@dwd why would mandatory TLS be a mitigation for these 3 points?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're presumably implying that if TLS is negotiated, the risk due to the first three points is mitigated. I'd state that explicitly.

xep-0388.xml Outdated
<p>As explained in § 13.8.3 of &rfc6120;, servers and clients SHOULD NOT support SASL PLAIN unless it is required by the authentication backend. Usage of PLAIN allows for trivial downgrade atacks by Man-In-The-Middle attackers circumventing any possible channel-binding or mutual authentication and even allowing the attacker to gain access to the cleartext password. If possible, PLAIN should be disabled by default on servers and clients and only be enabled when configured by the admin respective client user (if at all).</p>
<p>This protocol allows the client to provide a hint of its identity prior to authentication. This value MUST NOT be trusted to contain the real identity of the connecting client. Varying pre-authentication responses (such as supported SASL mechanisms) based on this value may allow unauthorized parties to discover whether an account exists, and even guess at what software might be used with that account (e.g. by observing whether the client has upgraded to specific mechanisms). Minimizing differences between the default response for unregistered JIDs and registered JIDs, as well as rate-limiting (to prevent enumeration attempts), are possible countermeasures. If the server does not know the client-identity it is RECOMMENDED to randomize the list of provided SASL mechanisms to reduce these risks even further.</p>
<p>SASL2 MUST only be used by Clients or offered by Servers after TLS negotiation, see § 5.2 and § 5.3.4 of &rfc6120;.</p>
<p>Clients MUST use channel-binding, if available, when requesting an upgrade to make sure no MITM can eavesdrop that hash and subsequently use it for authentication. Note that a client can always choose to not upgrade SASL mechanisms if it can not use channel-binding or the connection is otherwise deemed not secure enough.</p>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, we're somewhat into MTI territory .

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah sorry, I can change that to a SHOULD. That's the MTI for channel-binding you are referring above, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed it to a SHOULD

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can mark this as resolved if it was the MTI you referred to above.

<p>This protocol allows the client to provide a hint of its identity prior to authentication. This value MUST NOT be trusted to contain the real identity of the connecting client. Varying pre-authentication responses (such as supported SASL mechanisms) based on this value may allow unauthorized parties to discover whether an account exists, and even guess at what software might be used with that account (e.g. by observing whether the client has upgraded to specific mechanisms). Minimizing differences between the default response for unregistered JIDs and registered JIDs, as well as rate-limiting (to prevent enumeration attempts), are possible countermeasures. If the server does not know the client-identity it is RECOMMENDED to randomize the list of provided SASL mechanisms to reduce these risks even further.</p>
<p>SASL2 MUST only be used by Clients or offered by Servers after TLS negotiation, see § 5.2 and § 5.3.4 of &rfc6120;.</p>
<p>Clients MUST use channel-binding, if available, when requesting an upgrade to make sure no MITM can eavesdrop that hash and subsequently use it for authentication. Note that a client can always choose to not upgrade SASL mechanisms if it can not use channel-binding or the connection is otherwise deemed not secure enough.</p>
<p>Clients MUST NOT send, and servers MUST NOT process, authentication requests included in the TLS 0-RTT ("early data") extension. Such data is vulnerable to replay attacks, and without adequate protection this could allow an attacker to disrupt established sessions or cause other side-effects depending on the inline features negotiated. This rule may be overridden by later specifications, provided they define appropriate protocols to mitigate these issues. Examples of these attacks and mitigations are discussed in Section 8 of &rfc8446;.</p>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would make this a SHOULD NOT. I think you list enough explanation and potential mitigations to cover it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm, any thoughts @mwild1 ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've no strong feelings, and don't object to this change. I'm curious why you would relax a 'MUST NOT' that relates to a security concern, but sure... maybe there are closed trusted environments that want to do TLS 0-RTT without negotiation or replay protection. I would likewise hope that the explanation is enough to tell developers not to do this under any scenario where there might be a risk.

FWIW the TLS spec itself says:

Therefore, in normal operation, clients will not know which, if any, of these mechanisms servers actually implement and hence MUST only send early data which they deem safe to be replayed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's more that if you're using a multi-trip mechanism like SCRAM, then 0-RTT is fine for sending the SCRAM initial response, and it's no more or less secure. An attacker can at best repeat the client nonce, but that's hardly an issue. The problem is more likely to be that channel binding is trickier.

xep-0388.xml Show resolved Hide resolved
@Flowdalic
Copy link
Contributor

For reference, here is the rendered version: https://dyn.eightysoft.de/final/xep-0388.html

It would probably help to have a diff from the current version. Especially for an upcoming list disccusion. I usually create HTML diffs for XEPs using htmldiff.

@mwild1
Copy link
Contributor

mwild1 commented Oct 15, 2022

I generally agree with @dwd's points. The main thing I'll defend is having <user-agent> in here.

Honestly, I spent the past 5 years somehow thinking SASL2 already had a client identifier in it. I guess I made that up somewhere along the way, but I do think it makes sense. It's not the end of the world if it's in an separate XEP, but it's a solution to so many problems, and necessary for some SASL mechanisms (e.g. HT-*) because they don't provide any other key via which the server can determine which credentials it should be comparing against (maybe that's something to fix in those mechanisms, but there are other use cases too, including just debugging and diagnostic purposes).

As for XEP-0198 integration, the intent is not to define it in this spec, merely to say it's possible. It's covered by PR #1215 updating XEP-0198 itself. I prefer that PR over XEP-0397, as it reuses more of the existing protocol and fulfils more use cases without involving any extra stuff (e.g. token auth mechanisms).

Anyway, this should all be on the mailing list. If someone sparks a thread, I'll be happy to ^C^V everything there. If nobody does, I'll start one soon.

@tmolitor-stud-tu
Copy link
Contributor Author

There's a lot of changes to digest here, beyond even those we discussed (and I reserve my right to change my mind even when we did discuss!).

Sure! I did not intend to imply that our discussionwas something like a pre-approval. I just wanted to note that you are informed that I was working on this XEP. Of course you can change your mind any time.

I don't think this is sensible to publish yet, and certainly not without extensible discussion on the list.

I think the way forward if to address some of your issues we don't need to discuss about and leave the rest for an open discussion on the list. Does this seem sensible to you?

Quick warning: I'm really short at time currently. Several days may pass until I'm able to answer.

@tmolitor-stud-tu
Copy link
Contributor Author

I marked everything addressed in my last commits as "solved" to redirect the focus to unresolved things.
Feel free to "unresolve" things if you see the need for additional changes and/or start a discussion about this on-list.

@tmolitor-stud-tu
Copy link
Contributor Author

First rendered version of the split-out SCRAM upgrade tasks ProtoXEP: https://dyn.eightysoft.de/xeps/xep-scram-upgrade.html

@tmolitor-stud-tu
Copy link
Contributor Author

lgtm

current rendered version: https://dyn.eightysoft.de/final/xep-0388.html
current diff: https://dyn.eightysoft.de/final/xep-0388-diff.html

@mwild1 lgtm, too?

xep-0388.xml Outdated
<p>Servers capable of SASL2 offer a stream feature of &lt;mechanisms/>, qualified by the "urn:xmpp:sasl:1" namespace. This in turn contains one or more &lt;mechanism/> elements in the same namespace, and potentially other elements (for example, the &lt;hostname/> element defined within XEP-0233).</p>
<p>Note that SASL2 is impossible for clients to initiate without at least one mechanism being available, and therefore MUST NOT be offered.</p>
<section2 topic="SASL mechanism negotiation" anchor="feature">
<p>Clients capable of SASL2 MUST send the desired bare JID they want to authenticate for in the "from" attribute of the stream-header unless they don't know it (e.g. when using the GSS-API SASL mechanism etc.) according to section 4.7.1 of &rfc6120;. If the "from" attribute is not empty and the domain used in this attribute does not match the domain used in the "to" attribute the server SHOULD respond with an invalid-from error as defined in §4.9.3.9 of &rfc6120;. Providing the bare JID in the "from" attribute saves one round-trip.</p>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is restating RFC 6120 imperfectly - it doesn't, for example, refer to RFC 6120's suggestion that the JID shouldn't be supplied before TLS is in place, and additionally, this phrases as a "MUST unless you can't", rather than the "SHOULD and here's the reasons why not" that RFC 6120 uses. The last sentence is key here - if the "from" attribute is missing then this may incur an additional round-trip - so I suggest putting that first and referring to RFC 6120 §4.7.1 for further details.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a sentence that TLS SHOULD be in place when specifying the from-attribute is a good idea.

The from-attribute is a MUST, because SASL2 is designed to not work properly without it for most authentication methods: the server can not specify in it's stream features which methods it supports unless it knows which user wants to authenticate.

That's okay only, if the server only offers mechanisms like SASL EXTERNAL etc.

That means the if the "from" attribute is missing then this may incur an additional round-trip is an explanation why the protocol was designed the way it is rather than an explanation for the MUST.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about this again, I think this whole paragraph should be moved to the upgrade tasks ProtoXEP.
Without upgrade tasks (and the resulting dependence of the mechanism list on the user in the from attribute), giving the from attribute won't even save an additional round-trip: SASL2 behaves exactly like SASL1 here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But... All you're doing is raising the SHOULD in RFC6120 §4.7.1 to MUST. If you restate everything else people will think any omissions are intentional.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've moved that MUST to the protoXEP and rephrased it. It should be clearer now: https://dyn.eightysoft.de/final/xep-scram-upgrade.html#protocol
Or do you think it's still not clear?

<p>SASL security layers are sparingly used in the field, however, so this is thought to be a theoretical, rather than practical, concern.</p>
<p>As explained in § 13.8.3 of &rfc6120;, servers and clients SHOULD NOT support SASL PLAIN unless it is required by the authentication backend. Usage of PLAIN allows for trivial downgrade atacks by Man-In-The-Middle attackers circumventing any possible channel-binding or mutual authentication and even allowing the attacker to gain access to the cleartext password. If possible, PLAIN should be disabled by default on servers and clients and only be enabled when configured by the admin respective client user (if at all).</p>
<p>This protocol allows the client to provide a hint of its identity prior to authentication. This value MUST NOT be trusted to contain the real identity of the connecting client. Varying pre-authentication responses (such as supported SASL mechanisms) based on this value may allow unauthorized parties to discover whether an account exists, and even guess at what software might be used with that account (e.g. by observing whether the client has upgraded to specific mechanisms). Minimizing differences between the default response for unregistered JIDs and registered JIDs, as well as rate-limiting (to prevent enumeration attempts), are possible countermeasures. If the server does not know the client-identity it is RECOMMENDED to randomize the list of provided SASL mechanisms to reduce these risks even further.</p>
<p>SASL2 MUST only be used by Clients or offered by Servers after TLS negotiation, see § 5.2 and § 5.3.4 of &rfc6120;.</p>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're presumably implying that if TLS is negotiated, the risk due to the first three points is mitigated. I'd state that explicitly.

<p>This protocol allows the client to provide a hint of its identity prior to authentication. This value MUST NOT be trusted to contain the real identity of the connecting client. Varying pre-authentication responses (such as supported SASL mechanisms) based on this value may allow unauthorized parties to discover whether an account exists, and even guess at what software might be used with that account (e.g. by observing whether the client has upgraded to specific mechanisms). Minimizing differences between the default response for unregistered JIDs and registered JIDs, as well as rate-limiting (to prevent enumeration attempts), are possible countermeasures. If the server does not know the client-identity it is RECOMMENDED to randomize the list of provided SASL mechanisms to reduce these risks even further.</p>
<p>SASL2 MUST only be used by Clients or offered by Servers after TLS negotiation, see § 5.2 and § 5.3.4 of &rfc6120;.</p>
<p>Clients MUST use channel-binding, if available, when requesting an upgrade to make sure no MITM can eavesdrop that hash and subsequently use it for authentication. Note that a client can always choose to not upgrade SASL mechanisms if it can not use channel-binding or the connection is otherwise deemed not secure enough.</p>
<p>Clients MUST NOT send, and servers MUST NOT process, authentication requests included in the TLS 0-RTT ("early data") extension. Such data is vulnerable to replay attacks, and without adequate protection this could allow an attacker to disrupt established sessions or cause other side-effects depending on the inline features negotiated. This rule may be overridden by later specifications, provided they define appropriate protocols to mitigate these issues. Examples of these attacks and mitigations are discussed in Section 8 of &rfc8446;.</p>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's more that if you're using a multi-trip mechanism like SCRAM, then 0-RTT is fine for sending the SCRAM initial response, and it's no more or less secure. An attacker can at best repeat the client nonce, but that's hardly an issue. The problem is more likely to be that channel binding is trickier.

@tmolitor-stud-tu
Copy link
Contributor Author

It's more that if you're using a multi-trip mechanism like SCRAM, then 0-RTT is fine for sending the SCRAM initial response, and it's no more or less secure. An attacker can at best repeat the client nonce, but that's hardly an issue. The problem is more likely to be that channel binding is trickier.

We just did not want to go down this rabbit hole and rather leave the 0-RTT stuff to be weakened and defined in more detail by other/future XEPs:
It might work for SCRAM, but there might be other/future SASL mechanisms that would be more vulnerable if 0-RTT was used. Going the "default deny" route rather than the "default allow" seems like a sensible thing to do here.

@tmolitor-stud-tu
Copy link
Contributor Author

You're presumably implying that if TLS is negotiated, the risk due to the first three points is mitigated. I'd state that explicitly.

I still don't understand what "3 points" you are referring to.

@dwd
Copy link
Contributor

dwd commented Nov 19, 2022

It's more that if you're using a multi-trip mechanism like SCRAM, then 0-RTT is fine for sending the SCRAM initial response, and it's no more or less secure. An attacker can at best repeat the client nonce, but that's hardly an issue. The problem is more likely to be that channel binding is trickier.

We just did not want to go down this rabbit hole and rather leave the 0-RTT stuff to be weakened and defined in more detail by other/future XEPs: It might work for SCRAM, but there might be other/future SASL mechanisms that would be more vulnerable if 0-RTT was used. Going the "default deny" route rather than the "default allow" seems like a sensible thing to do here.

You're not doing "default deny", you're doing "deny".

@dwd
Copy link
Contributor

dwd commented Nov 19, 2022

You're presumably implying that if TLS is negotiated, the risk due to the first three points is mitigated. I'd state that explicitly.

I still don't understand what "3 points" you are referring to.

The first three points made in this section, about additional exposure and so on.

@tmolitor-stud-tu
Copy link
Contributor Author

It's more that if you're using a multi-trip mechanism like SCRAM, then 0-RTT is fine for sending the SCRAM initial response, and it's no more or less secure. An attacker can at best repeat the client nonce, but that's hardly an issue. The problem is more likely to be that channel binding is trickier.

We just did not want to go down this rabbit hole and rather leave the 0-RTT stuff to be weakened and defined in more detail by other/future XEPs: It might work for SCRAM, but there might be other/future SASL mechanisms that would be more vulnerable if 0-RTT was used. Going the "default deny" route rather than the "default allow" seems like a sensible thing to do here.

You're not doing "default deny", you're doing "deny".

But that "deny" could be overwritten by a later xep, no?

@tmolitor-stud-tu
Copy link
Contributor Author

You're presumably implying that if TLS is negotiated, the risk due to the first three points is mitigated. I'd state that explicitly.

I still don't understand what "3 points" you are referring to.

The first three points made in this section, about additional exposure and so on.

Ah, I understand.
Fixed here: 18fb293

@Neustradamus
Copy link

To follow

@tmolitor-stud-tu
Copy link
Contributor Author

@Kev this isn't "needs author" anymore, Dave already confirmed this on standards@

@dwd can you confirm this here again?

@tmolitor-stud-tu
Copy link
Contributor Author

@dwd I'm sorry, I can't find your mail at standards@ confirming this can be merged. Am i hallucinating?

@tmolitor-stud-tu
Copy link
Contributor Author

@Kev This is not NeedsAuthor anymore, but ReadyToMerge!

See here: https://mail.jabber.org/pipermail/standards/2023-January/039133.html

- Add reference to SASL Channel-Binding Type Capability (XEP-0440) [1] and RFC 5802 [2]
- Update security considerations and business rules
- Clarify <continue/> and tasks
- Add expansion point to inline stream resumption and BIND2 (and possibly others)
- Add optional <user-agent/> element
@guusdk guusdk removed the Needs Author The XEP is experimental and the PR was not made by the author. The author needs to acknowledge it. label Feb 9, 2023
@guusdk
Copy link
Contributor

guusdk commented Feb 9, 2023

@tmolitor-stud-tu I believe you can make the XEP validation happy if you reverse the order of lines 15 and 16. <sig> should go before <approver>, not the other way around.

Update: I was allowed to and have pushed a commit with this change to your branch.

@tmolitor-stud-tu
Copy link
Contributor Author

tmolitor-stud-tu commented Feb 9, 2023

Update: I was allowed to and have pushed a commit with this change to your branch.

Perfect, thanks!

@guusdk
Copy link
Contributor

guusdk commented Feb 9, 2023

XEP state is currently 'deferred'. Changes are not purely editorial. As per https://github.com/xsf/xeps/blob/master/docs/TRIAGING.md this requires a move to 'experimental' state.

@guusdk
Copy link
Contributor

guusdk commented Mar 9, 2023

In an attempt to move this forward (and after consultation in editors@) I've added a commit that changes the status from the XEP from Deferred to Experimental.

@Kev
Copy link
Member

Kev commented Mar 9, 2023

In an attempt to move this forward (and after consultation in editors@) I've added a commit that changes the status from the XEP from Deferred to Experimental.

Consultation, but then sadly not doing what I asked. I'll try to unravel this when I have a chance.

@guusdk
Copy link
Contributor

guusdk commented Mar 9, 2023

not doing what I asked

I blame poor instructions :)

Should be fixed now.

@Kev Kev merged commit b5c14fb into xsf:master Mar 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants