Smack client may disconnect if invalid message is received

Smack Smack Dev
1

Hi, I have stumbled across this situation:

My (TCP) Smack client disconnects on certain incoming invalid messages. This happens even if a relaxed ParsingExceptionCallback is configured. This disconnected client causes an unwanted (temporary) service disruption.

Example: A message containing an invalid XForm like

<x xmlns="jabber:x:data">
  <field/>
</a>

makes DataFormProvider#parseField to raise a NullPointerException because the logic expects the fieldName (var) not to be null.

This isn’t properly handled by AbstractXMPPConnection#parseAndProcessStanza via ParsingExceptionCallback because the NullPointerException isn’t caught here. The connection gets closed instead.

I assume that this can be easily solved by catching the generic java.lang.Exception rather than XmlPullParserException | SmackParsingException | IOException | IllegalArgumentException inside #parseAndProcessStanza.

This would add more overall robustness to a Smack client because there might be other kind of exceptions lurking in the ExtensionElement implementations.

May I create a PR for applying my proposal?

Greetings Peter

BTW: Thank you very much for your great work on Smack and Openfire!

2

Hi Peter! Thanks for your feedback!

I’m not sure if catching additional (runtime) exceptions is a good or bad thing. The benefit that you describe may not outweigh the benefit of this identifying bugs in code. I’ll leave it up to @Flow to determine if additional exceptions should be caught.

In this example, I would consider DataFormProcessor#parseField to not throw a SmackParsingException to be a bug.

If you do want to make a contribution, then one that fixes DataFormProcessor would be a very welcome one! I suggest to make use of ParserUtils#getRequiredAttribute (and friends) to make that implementation more robust!

3

Hi @guus, thank you for your quicky reply!

Sure, adjusting the DataForm parser so that it handles such situation more graceful would be definitely a good thing.

Anyway, I’m aiming a solution that is even more robust against bugs inside the ExtensionElement handlers that have not been found yet but which would cause the same situation that the connection is closed. Malicious persons might use this for disrupting services.

When looking at parseAndProcessStanza I can’t imagine that there would be any kind of Exception in this context that would be worth not being caught and not being handled by the ParsingExceptionCallback. It may be even decided inside this ParsingExceptionCallback to re-throw the exception and terminate the connection.

I’m curious for your evaluation @Flow :slight_smile:

Best regards Peter

4

Before I comment on this, could you show us the stacktrace please?

5

Never mind, I assume it is probably in DataformProvider.java:243 fieldName.equals(FormField.FORM_TYPE). Correct?

6

Good morning!

Yes, it’s this place where the NPE is raised

java.lang.NullPointerException: Cannot invoke "String.equals(Object)" because "fieldName" is null
	at org.jivesoftware.smackx.xdata.provider.DataFormProvider.parseField(DataFormProvider.java:243)
	at org.jivesoftware.smackx.xdata.provider.DataFormProvider.parseField(DataFormProvider.java:144)
	at org.jivesoftware.smackx.xdata.provider.DataFormProvider.parse(DataFormProvider.java:92)
	at org.jivesoftware.smackx.xdata.provider.DataFormProvider.parse(DataFormProvider.java:61)
	at org.jivesoftware.smack.provider.Provider.parse(Provider.java:53)
	at org.jivesoftware.smack.util.PacketParserUtils.parseExtensionElement(PacketParserUtils.java:841)
	at org.jivesoftware.smack.util.PacketParserUtils.parseMessage(PacketParserUtils.java:186)
	at org.jivesoftware.smack.util.PacketParserUtils.parseStanza(PacketParserUtils.java:111)
	at org.jivesoftware.smack.AbstractXMPPConnection.parseAndProcessStanza(AbstractXMPPConnection.java:1449)
	at org.jivesoftware.smack.tcp.XMPPTCPConnection.access$1000(XMPPTCPConnection.java:131)
	at org.jivesoftware.smack.tcp.XMPPTCPConnection$PacketReader.parsePackets(XMPPTCPConnection.java:972)
	at org.jivesoftware.smack.tcp.XMPPTCPConnection$PacketReader.access$700(XMPPTCPConnection.java:916)
	at org.jivesoftware.smack.tcp.XMPPTCPConnection$PacketReader$1.run(XMPPTCPConnection.java:939)
	at java.base/java.lang.Thread.run(Thread.java:1589)

and this NPE falls through at

public abstract class AbstractXMPPConnection implements XMPPConnection {

   ...

    protected void parseAndProcessStanza(XmlPullParser parser)
                    throws XmlPullParserException, IOException, InterruptedException {
        ParserUtils.assertAtStartTag(parser);
        int parserDepth = parser.getDepth();
        Stanza stanza = null;
        try {
            stanza = PacketParserUtils.parseStanza(parser, incomingStreamXmlEnvironment);
        }
        catch (XmlPullParserException | SmackParsingException | IOException | IllegalArgumentException e) {
            CharSequence content = PacketParserUtils.parseContentDepth(parser,
                            parserDepth);
            UnparseableStanza message = new UnparseableStanza(content, e);
            ParsingExceptionCallback callback = getParsingExceptionCallback();
            if (callback != null) {
                callback.handleUnparsableStanza(message);
            }
        }
        ParserUtils.assertAtEndTag(parser);
        if (stanza != null) {
            processStanza(stanza);
        }
    }

Greetings Peter

7

First, thanks for reporting the issue. :slight_smile:

I’d like to avoid catch Exception when possible. But you are right that a faulty parser implementation should not open a DoS vector. Therefore I made the handling more robust with

I also fixed the original issue in the data form provider

I am going to release a new beta version soon™.

8

Thank you very much for fixing. Appreciated!

9

This topic was automatically closed 100 days after the last reply. New replies are no longer allowed.