As mentioned in the timestamp parsing thread, it is an embarassingly dumb idea to crash your connection on attacker-controlled “invalid” input. If the attacker in question happens to be an outdated version of Gajim (like the latest 1.1.2 release), the following happens:
AbstractXMPPConnection: Connection XMPPTCPConnection[censored/yaxim] (0) closed with error
AbstractXMPPConnection: java.lang.IllegalArgumentException: No enum constant org.jivesoftware.smack.packet.Presence.Mode.error
AbstractXMPPConnection: at java.lang.Enum.valueOf(Enum.java:257)
AbstractXMPPConnection: at org.jivesoftware.smack.packet.Presence$Mode.valueOf(Presence.java:395)
AbstractXMPPConnection: at org.jivesoftware.smack.packet.Presence$Mode.fromString(Presence.java:432)
AbstractXMPPConnection: at org.jivesoftware.smack.util.PacketParserUtils.parsePresence(PacketParserUtils.java:544)
AbstractXMPPConnection: at org.jivesoftware.smack.util.PacketParserUtils.parseStanza(PacketParserUtils.java:159)
AbstractXMPPConnection: at org.jivesoftware.smack.AbstractXMPPConnection.parseAndProcessStanza(AbstractXMPPConnection.java:1050)
AbstractXMPPConnection: at org.jivesoftware.smack.tcp.XMPPTCPConnection.access$500(XMPPTCPConnection.java:151)
AbstractXMPPConnection: at org.jivesoftware.smack.tcp.XMPPTCPConnection$PacketReader.parsePackets(XMPPTCPConnection.java:1078)
AbstractXMPPConnection: at org.jivesoftware.smack.tcp.XMPPTCPConnection$PacketReader.access$300(XMPPTCPConnection.java:1034)AbstractXMPPConnection: at org.jivesoftware.smack.tcp.XMPPTCPConnection$PacketReader$1.run(XMPPTCPConnection.java:1050)
AbstractXMPPConnection: at java.lang.Thread.run(Thread.java:764)
I think I have explained to you the rationale and how you can prevent this from terminating the connection (setup a parsing exception callback) multiple times already.
I am not sure why the ‘invalid’ is in quotes, a value of ‘error’ for the ‘show’ element is not specified nor are custom values allowd, so it violates the XMPP standard (cf. RFC 6121 § 4.7.2.1).
Indeed. I’m just saying that it is a bad design to have this as default behavior because it allows remote users to DoS smack-based applications.
Because it is syntactically valid XML, and thus not a reason for my server to filter it out, to terminate the s2s connection or to do other evil things preventing me from seeing this semantically invalid XMPP data.
org.jxmpp.stringprep.XmppStringprepException: XmppStringprepException caused by '"مدرسة¤الهاكرز¤الأحرار"@conference.jabber.org': org.jxmpp.stringprep.XmppSt
at org.jxmpp.jid.impl.JidCreate.from(JidCreate.java:162)
at org.jivesoftware.smack.util.ParserUtils.getJidAttribute(ParserUtils.java:79)
at org.jivesoftware.smack.util.ParserUtils.getJidAttribute(ParserUtils.java:71)
at org.jivesoftware.smackx.disco.provider.DiscoverItemsProvider.parse(DiscoverItemsProvider.java:54)
at org.jivesoftware.smackx.disco.provider.DiscoverItemsProvider.parse(DiscoverItemsProvider.java:36)
at org.jivesoftware.smack.provider.Provider.parse(Provider.java:43)
at org.jivesoftware.smack.util.PacketParserUtils.parseIQ(PacketParserUtils.java:616)
at org.jivesoftware.smack.util.PacketParserUtils.parseStanza(PacketParserUtils.java:157)
at org.jivesoftware.smack.AbstractXMPPConnection.parseAndProcessStanza(AbstractXMPPConnection.java:1047)
at org.jivesoftware.smack.tcp.XMPPTCPConnection.access$600(XMPPTCPConnection.java:154)
at org.jivesoftware.smack.tcp.XMPPTCPConnection$PacketReader.parsePackets(XMPPTCPConnection.java:1083)
at org.jivesoftware.smack.tcp.XMPPTCPConnection$PacketReader.access$300(XMPPTCPConnection.java:1035)
at org.jivesoftware.smack.tcp.XMPPTCPConnection$PacketReader$1.run(XMPPTCPConnection.java:1052)
at java.lang.Thread.run(Thread.java:761)
Caused by: org.jxmpp.stringprep.XmppStringprepException: Localpart must not contain '"'
at org.jxmpp.stringprep.simple.SimpleXmppStringprep.localprep(SimpleXmppStringprep.java:74)
at org.jxmpp.stringprep.XmppStringPrepUtil.localprep(XmppStringPrepUtil.java:62)
at org.jxmpp.jid.parts.Localpart.from(Localpart.java:165)
at org.jxmpp.jid.impl.LocalAndDomainpartJid.<init>(LocalAndDomainpartJid.java:46)
at org.jxmpp.jid.impl.JidCreate.from(JidCreate.java:105)
at org.jxmpp.jid.impl.JidCreate.from(JidCreate.java:160)
... 13 more
As it happens in a single disco#items <item/>, there is no way to recover the remaining thousands of items, I have to drop the whole IQ result.
While I see why your first impulse is to believe that this may be bad design, I do not agree that this is actually the case. My experience is, that this helps developers at a very early stage of their projects, usually before they release the first alpha or beta version, to become aware of this situation, and the possible solution (each having a different drawback). Smack cannot decide what to do per default, it is up to the application developer to decide how to handle it.
But invalid in XMPP.
Most servers will not look at the data/extension elements which are not relevant for routing or the server operation, for various reasons. But I fail to see why you bring up server behavior into this argument.
An example for invalid XMPP? Appears so.
With what Smack currently offers you, yes. And I see why alternative options, some of which I would possibly not oppose, may be desirable. But on the other side it tells a lot of our current state of compliance within the federated XMPP network if even jabber.org does not follow to the XMPP specification1
1: Assuming that SimpleXmppStringprep is right about ‘"’ not being allowed in the localpart.
I tend to agree with Georg here. Why not log a warning, so that developers can see their error, but attackers are not able to close other connections. Would that be a good compromise?
It (=) is allowed, because it it is in the ASCII7 range, which is explicitly allowed by RFC8264’s IdentifierClass, which is used for JID localparts. The JID is invalid nonetheless because it contains a symbol character U+00A4.
No, I do not think so. Some developers would simply ignore the error logs. I am going to say again that I believe that this default behavior raises awareness for the situation at a very early stage of the development process (which is desirable). And even if there is a alpha/beta/stable release which does not setup a custom parsing exception handler, then the vendor can easily setup one with just 1-3 lines of code and push a new version.
(Assuming you mean “It (‘"’ U+0022)” here) If it only where so easy: RFC 7622 § 3.3.1. explicitly forbids U+0022 in the localpart.
This is not how software development works. If you want to raise awareness, introduce an explicit required ParsingExceptionCallback parameter in the XMPPConnection / Configuration constructor in the next API bump.
You can then create a bunch of default implementations, like the IgnoreParsingExceptionCallback, the DisconnectParsingExceptionCallback and the LogParsingExceptionCallback, so that developers can actually make a conscious decision.
So true . On the other hand XEP-106 somehow allows it again. Not sure how your JidCreate.from() implementation works here, but assuming it checks the raw value which comes from network, it works correct then of course.
It is my understanding that XEP-0106 allows for ‘"’ to appear in unescaped JIDs. However you will only find escaped JIDs on the wire, where XEP-0106 does not relax the restrictions.
Well the user has to tell if he wants to create an Jid instance from an unesacped or escaped String/CharSequence. Obviously escaped is the default (since it is what an actual JID represents), hence there is Jid.fromUnescaped(CharSequence).
Thanks for this discussion. Just wanted to add that also Psi priority can DoS XMPPTCPConnection. We had to learn this the hard way and wished Smack would have kept priority range check inside the API. Nowadays we ignore the faulty presence stanza that’s sent by any client and have other (inconsistency) problems. As Flow already indicated compliance within the federated XMPP network is difficult (if not impossible) to achieve.
Assuming that it means what I believe that it means (that smack should just silently mask the invalid value), you possibly would have end up spending hours in debugging a strange behavior under certain circumstances if this would have been Smack’s policy.
I just do not believe that there is an easy “one-size-fits-all” solution. But…
An implementation MAY choose to accept or send only data that has been explicitly validated against the schemas provided in this document, but such behavior is OPTIONAL
Clients […] SHOULD ignore any non-conformant elements or attributes on the incoming XML stream.
I believe “non-conformant” means not valid as per the XML schema.
Alternatively to Georg’s suggestion, you could provide a boolean configuration property, which by default validates against the schema, but which developers could switch off, in the sense of the “OPTIONAL” behavior from § 11.4.
In this case you could set smart defaults, like setting the presence priority to the min value if it is -2000.
I like your pursuit to strict parsing, but it shouldn’t be too strict or imo (attackers should not be able to drop connections). Just imagine if web browsers would crash or display an empty page, if the page’s HTML does not conform to the Doctype declaration.
Many sites aren’t even well-formed, e.g. have missing closing elements.
In the case of a malformed JID, like in the example above, you could also return a jid-malformed error, or enable application developers to do so, if they want to. (Maybe it’s already possible in the exception callback).
Another pitfall to think about:
I bet there are legacy JIDs which are valid by RFC 6122, but are no longer valid by as per RFC 7622 or vice versa.
I think that is a prime argument for my case. The world would be a better place if browsers had never begun to accept invalid HTML. The code of relaxed HTML parser is one of most terrible things I have ever seen.
Yep, reporting the error back to the originating entity is what I am working towards to.
Great. I think Georg wants a callback to be mandatory to be set, which I think is reasonable idea.
Hehe … probably you are right. I hate it, too, but it was just an extreme example. Web browsers are far too loose with interpreting even non-wellformed HTML, which is really bad.
Yes. Everything is better in the parallel universe of strict XML schema compliance. However, in our universe, shipping a library that crashes the connection on incompliant remote input is like shipping a browser that crashes by default. Nobody would be using it, except for people who can’t switch to a better alternative.
That’s interesting, but I’m not sure how useful this would be in the general case. For incoming direct messages, you might make the sending user aware of the error. For misconfigured clients, MUCs, bots, services etc. this is not going to achieve anything, except maybe a busy loop like this.
