Handle unexpected SASL <challenge>s

[kousu]

A malicious/misbehaving server can crash xmpp.js here by offering FAST but then responding to the <initial-response> with a <challenge> instead of a <success> or <failure>. That would cause xmpp.js to run

        await mech.challenge(decode(element.text()));

but mech.challenge isn't defined for HT-SHA-256-NONE, which is the one and only FAST mechanism supported at the moment.

[sonnyp]

Thanks, can you please refer to the spec in the code/PR and adjust accordingly if needed?

A test is also needed https://github.com/xmppjs/xmpp.js/blob/main/CONTRIBUTING.md#making-changes

[singpolyma]

This change makes sense to me, though the other way to go would be to require SASL mechanisms to implement challenge and the HT-* could just throw in there. I have not strong opinion either way, LGTM

[kousu]

Ah of course, I should have read that. I found this while working on something else so I didn't give it the time it needed.

I agree with @singpolyma and added the exception into the HT-* code directly, and I've added a test! The test is verbose so if there's tricks to cutting it down I'd appreciate them.

---

Writing the test was good, it found a corner case -- in the unlikely (but not illegal) case of the server ONLY offering FAST and misbehaves/rejects our FAST token -- which I worked around by adding this also not the most elegant thing.

https://github.com/xmppjs/xmpp.js/blob/4b8e2dad35e98cdae08e8d109269c050c1260024/packages/sasl2/index.js#L119-L123

---

I don't think there's a good part of the spec to cite. https://xmpp.org/extensions/xep-0388.html#challenge says

Server Challenges MAY then be sent. Each Challenge MUST be responded to by a Client in a Client Response. [...] Authentication may complete in one of three ways. It may complete successfully, in which case the client is authenticated. It may also fail, in which case the client is not authenticated and the stream and session state remain entirely unchanged.

https://xmpp.org/extensions/xep-0484.html#fast-auth says

The client authenticates normally using SASL2, using the FAST SASL mechanism it previously selected, and the token provided by the server.

neither one says what happens if the server <challenge>s a mechanism that doesn't do challenges. They either assumed that all SASL mechanisms would do challenges or that servers would never misbehave and send a challenge when they shouldn't. Actually, if we're being really picky, the HT-* methods should actually respond with a <response> instead of throwing internally, but the spec doesn't say what to put in there -- the empty string?

---

Thank you for taking the time to look at this and for keeping the xmpp ecosystem lively, @sonnyp.

[kousu]

Is there anything more you expect from the test coverage than what I've done so far?

[sonnyp]

To be honest I'm not too sure why we should protect against misbehaving servers.

XMPP assumes the server is trust worthy. If XMPP.js was to start adding safeguards for every possible server misbehave, we would never see the end of it.

Could you clarify how and why you encountered this in the first place? Maybe there is a good reason I don't see to add a safeguard here.

Why do you consider it a bug in XMPP.js rather than in the server ?

[sonnyp]

@kousu ping

[kousu]

I came across this while working on client-side SASL. I'd patched the server to help and was experimenting. So sure, I probably broke the carefully orchestrated flow that xmpp.js was expecting. But flows that easy to knock off off the rails aren't robust, and I want my code to be robust.

Maybe, notice that my patch isn't actually handling a specific misbehaviour, it's adding a catch-all exception handler around the pre-existing error. Without catching the error I was seeing, xmpp.js was choking silently and iirc I had to reload the page to get it working again. This makes sure that any errors in the new-ish FAST code bubble up.

I guess we just have different attitudes here. I barely trust code I write, so I definitely don't trust remote code. If xmpp.js is by design not safe against servers misbehaving then I guess I'm bringing this to the wrong place.

[sonnyp]

Thanks for taking the time to elaborate and consider various positions.

I would consider merging this if there was a known server impl behaving like so.

If xmpp.js is by design not safe against servers misbehaving

"By design" is misleading.

In any case, it's not even about xmpp.js.

XMPP itself is not safe against server misbehaving. The main server is considered trustworthy.