mux: silent fail on stanzas with whitespace #315

Closed
opened 2 months ago by sxvghd · 2 comments

It seems like some clients (stanzas from Gajim's XML console for example) can send stanzas with whitespace in them. In that case after parsing the top level element, the first token and only token approached by the routers is the whitespace, which causes the stanza to not get matched to their respected handlers.
E.g the following XML through Gajim:

<iq from="test1@localhost/gajim.G2A7A0VC" id="kl2fax27" to="chat326@component.localhost" type="get" xmlns="jabber:client">
  <query xmlns="http://jabber.org/protocol/disco#items" />
</iq>

ends up looking like this to the handler:

---begin stanza---
xml.CharData([10 32 32][1])
Start Element - Space:  http://jabber.org/protocol/disco#items  Local:  query
xml.EndElement({{http://jabber.org/protocol/disco#items query}}[1])
xml.CharData([10][1])
xml.EndElement({{jabber:component:accept iq}}[1])
---end stanza---

In my opinion the muxer stanza routers should iterate until they find a valid start element token, but an error would also be better than a silent fail.

It seems like some clients (stanzas from Gajim's XML console for example) can send stanzas with whitespace in them. In that case after parsing the top level element, the first token and only token approached by the routers is the whitespace, which causes the stanza to not get matched to their respected handlers. E.g the following XML through Gajim: ``` <iq from="test1@localhost/gajim.G2A7A0VC" id="kl2fax27" to="chat326@component.localhost" type="get" xmlns="jabber:client"> <query xmlns="http://jabber.org/protocol/disco#items" /> </iq> ``` ends up looking like this to the handler: ``` ---begin stanza--- xml.CharData([10 32 32][1]) Start Element - Space: http://jabber.org/protocol/disco#items Local: query xml.EndElement({{http://jabber.org/protocol/disco#items query}}[1]) xml.CharData([10][1]) xml.EndElement({{jabber:component:accept iq}}[1]) ---end stanza--- ``` In my opinion the muxer stanza routers should iterate until they find a valid start element token, but an error would also be better than a silent fail.
SamWhited added the
bug
label 2 months ago
Owner

Thanks for the report! More context from the chat;

  • This is illegal behavior on Gajims part, and likely a bug that should be reported to them too
  • The best course of action IMO is to look for this and send a stream error if it happens, closing the connection (otherwise it becomes an awkward side channel that can possibly be used to smuggle information into a stanza, fingerprint clients, etc.)
Thanks for the report! More context from the chat; - This is illegal behavior on Gajims part, and likely a bug that should be reported to them too - The best course of action IMO is to look for this and send a stream error if it happens, closing the connection (otherwise it becomes an awkward side channel that can possibly be used to smuggle information into a stanza, fingerprint clients, etc.)
SamWhited added this to the v1.0.0 milestone 2 months ago
Owner

After discussing this on jdev@, it appears that I was wrong, this is allowed by the spec (and even if it weren't multiple clients send stanzas with whitespace in them and we need to remain compatible). This means that almost every Unmarshaler implementation in this package is wrong and some clients won't be able to communicate with us.

We can fix this by stripping whitespace-only tokens at the top level.

EDIT: that last part is wrong, we probably can't strip them at the top level because it's unclear what whitespace is significant or not. Instead let's just fix it on the multiplexer.

After discussing this on jdev@, it appears that I was wrong, this is allowed by the spec (and even if it weren't multiple clients send stanzas with whitespace in them and we need to remain compatible). This means that almost every Unmarshaler implementation in this package is wrong and some clients won't be able to communicate with us. We can fix this by stripping whitespace-only tokens at the top level. **EDIT:** that last part is wrong, we probably can't strip them at the top level because it's unclear what whitespace is significant or not. Instead let's just fix it on the multiplexer.
SamWhited referenced this issue from a commit 1 month ago
SamWhited closed this issue 1 month ago
Sign in to join this conversation.
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: mellium/xmpp#315
Loading…
There is no content yet.