Openfire (ARCHIVED)

According to RFC 3920 (Page 51) an 'id' attribute is required for IQ stanzas. Openfire does simply assume it's there, without testing. This causes a NullPointerException for IQ packets of type 'result' or 'error':

java.lang.NullPointerException
at java.util.concurrent.ConcurrentHashMap.remove(ConcurrentHashMap.java:922)
at org.jivesoftware.openfire.IQRouter.handle(IQRouter.java:284)
at org.jivesoftware.openfire.IQRouter.route(IQRouter.java:101)
at org.jivesoftware.openfire.spi.PacketRouterImpl.route(PacketRouterImpl.java:68)
at org.jivesoftware.openfire.net.StanzaHandler.processIQ(StanzaHandler.java:311)
at org.jivesoftware.openfire.net.ClientStanzaHandler.processIQ(ClientStanzaHandler.java:79)
at org.jivesoftware.openfire.net.StanzaHandler.process(StanzaHandler.java:276)
at org.jivesoftware.openfire.net.StanzaHandler.process(StanzaHandler.java:175)
at org.jivesoftware.openfire.nio.ConnectionHandler.messageReceived(ConnectionHandler.java:133)

When catching the NullPointerException the ConnectionHandler does close the client connection to the sender of the packet.

Because IQ packets of type 'get' or 'set' can still pass the server and because most clients seem to response to such bad packets by simply "copying" the not existing 'id' attribute, this bug can be used to kick other users. A simple jabber:iq:version request without 'id' attribute is sufficient.

Serveral clients have bugs that produce IQ packets without 'id' attribute. Such as Miranda 0.7.10, Trillian 3.1.7.0 and CenterIM 4.22.5. Old versions of Miranda (confirmed for 0.5.1) produce even this jabber:iq:version bug as described above. So when they come online they can kick all users in their roster, and the affected users can not even login again.

I wrote a plugin that seems to disable this bug. It's attached here. However, this only a temporary solution.

A better solution would maybe this untested patch:

if (IQ.Type.result == packet.getType() || IQ.Type.error == packet.getType()) {
    if (packet.getID() != null) {    // <---------------------------------------  here!
        // The server got an answer to an IQ packet that was sent from the server
        IQResultListener iqResultListener = resultListeners.remove(packet.getID());
        if (iqResultListener != null) {
            resultTimeout.remove(packet.getID());
            if (iqResultListener != null) {
                try {
                    iqResultListener.receivedAnswer(packet);
                }
                catch (Exception e) {
                    Log.error("Error processing answer of remote entity", e);
                }
                return;
            }
        }
    }
}

Getting OF 3.6.1 out of the door today and no time to test a fix for this. Sending an IQ packet with no ID is not legal and should be considered invalid XML. XMPP Core recommends using the following stream error when:

"<bad-format/> – the entity has sent XML that cannot be processed; this error MAY be used instead of the more specific XML-related errors, such as <bad-namespace-prefix/>, <invalid-xml/>, <restricted-xml/>, <unsupported-encoding/>, and <xml-not-well-formed/>, although the more specific errors are preferred."

A stream error would imply closing the connection to the client that sent the invalid XML. We can either go this way or we can be more flexible and just ignore the invalid IQ packet by responding with an IQ error. This fix would happen in StanzaHandler around line 276. We should just check that the received IQ has an ID attribute.

What do you think?

What do you think?

Hm, many clients have this bug. At least Miranda 0.7.10, Trillian 3.1.7.0 and CenterIM 4.22.5. I think >30% of my users do use Miranda. I don't want to kick them all. Enforcing standards is a good idea, but not with this costs.

Would it be possible to intercept the packet with PacketInteceptor before it does reach "StanzaHandler around line 276" ? Then everyone who does not want to kick his users, could still use the Dropper plugin. I could modify the plugin, so it does send a message to the user every hour (etc.), which says that his client is producing invalid packets and should update his client (or slam the client devs on their feet, so they fix this bug in their client)

If this interception is possible, if have no problem with your fix. I would use my plugin (as I currently do) and would remove it as soon as 95% of all users have a client which conforms to the standard.

Improved version of the Dropper plugin, which solves this bug as temp solution:
http://www.igniterealtime.org/community/message/183025

PacketInterceptors are triggered much later in the process. The routers are the ones that trigger them. The StanzaHandler is basically the entry door to the system and that is where "broken" XML should be solved. IMO, the fix needs to be placed in the entry door. Now, what the fix implies is another story. From your description it seems that you want to make it configurable what you want to do when an invalid XML packet arrives. If packets will still be able to get into the server then we need to fix the known NPE besides fixing StanzaHandler. However, we should know that other NPE could still happen since IQ packets may go to components that will have the same problem.

Have the developers of those clients been contacted and this problem reported? I think I would configure OF to disconnect clients by default and let admins use a system property to disable the disconnection and let IQ packets get into the server.

As long as only the offending client is kicked and not others I think that is appropriate. Currently, others an an offending client's buddy list are also kicked. My site is held at 3.5.2 until this is fixed appropriately.

@Mark Keisler:
You can use the plugin I linked above. I'm using this since two months now without problems.

@Gaston Dombiak:
I did not report this myself. However, I told some of my users (which where using the affected clients) to report the problem. I don't know if they did so.

If packets will still be able to get into the server then we need to fix the known NPE besides fixing StanzaHandler. However, we should know that other NPE could still happen since IQ packets may go to components that will have the same problem.

What about doing that same that the plugin does? (simply adding that ID attribute)

However, best way would be solving this issue on client side.

Ok, gave it a quick test, looks good. However, I suggest a default value of true instead of false for the new system property xmpp.server.validation.enabled

IMHO not kicking either side is the best solution usabiliti-wise if not standards-wise.
This patch seems to work for us:

— src/java/org/xmpp/packet/IQ.java 2008-11-05 16:16:16 +0000
+++ src/java/org/xmpp/packet/IQ.java 2008-11-16 00:35:37 +0000
@@ -67,6 +67,10 @@
*/
public IQ(Element element) {
super(element);
+ if (getID() == null) { + String id = String.valueOf(random.nextInt(1000) + "-" + sequence++); + setID(id); + }
}

/**
@@ -80,6 +84,10 @@
*/
public IQ(Element element, boolean skipValidation) {
super(element, skipValidation);
+ if (getID() == null) {+ String id = String.valueOf(random.nextInt(1000) + "-" + sequence++);+ setID(id);+ } }
}

/**
@@ -95,6 +103,10 @@
// Copy cached JIDs (for performance reasons)
this.toJID = iq.toJID;
this.fromJID = iq.fromJID;
+ if (getID() == null) { + String id = String.valueOf(random.nextInt(1000) + "-" + sequence++); + setID(id); + }
}

/**

Miranda Bug-Report
http://bugs.miranda-im.org/view.php?id=501

CenterIM Bug-Report
http://bugzilla.centerim.org/show_bug.cgi?id=119