NPE in MessageDeliveryLogic (#1351)

Closed

Activities

Andrzej Wójcik (Tigase) opened 1 year ago

There is an NPE being thrown in the MessageDelieveryLogic class:

[2023-04-02 07:52:34:907] [SEVERE  ] [        in_15-sess-man ] AbstractMessageReceiver$QueueListener.run(): [in_15-sess-man] Exception during packet processing: from=c2s@ip-172-31-38-91.us-west-2.compute.internal/172.25.0.3_5223_172.31.30.147_22270, to=sess-man@ip-172-31-38-91.us-west-2.compute.internal, serverAuthorisedStanzaFrom=Optional[…@sure.im/…], DATA=<iq type="error" from="…@sure.im/…" xmlns="jabber:client" id="g5QCL9"/>, SIZE=91, XMLNS=jabber:client, PRIORITY=NORMAL, PERMISSION=NONE, TYPE=error, STABLE_ID=6f9f4b64-b340-4a6f-801f-64b70c850c6d
java.lang.NullPointerException: Cannot invoke "tigase.xmpp.jid.JID.getDomain()" because the return value of "tigase.server.Packet.getStanzaTo()" is null
	at tigase.xmpp.impl.MessageDeliveryLogic.preProcessFilter(MessageDeliveryLogic.java:268)
	at tigase.xmpp.impl.MessageAmp.preProcess(MessageAmp.java:160)
	at tigase.server.xmppsession.SessionManager.processPacket(SessionManager.java:1919)
	at tigase.cluster.SessionManagerClustered.processPacket(SessionManagerClustered.java:321)
	at tigase.cluster.SessionManagerClustered.processPacket(SessionManagerClustered.java:311)
	at tigase.server.AbstractMessageReceiver$QueueListener.run(AbstractMessageReceiver.java:1398)

In this code, there is an assumption, according to comments, that it works on/tests messages, while preProcess() method in XMPPPreprocessorIfc is called without any checks nor filtering, which means that any type of packet (iq/message/presence) may be tested with those rules.

Another issue is that this code assumes that to for the stanza is set. According to RFC, if there is no to, then the stanza should be processed locally by the server and I think in this case, server sent to the client (from the domain bare jid) some iq request and client in response removed to as it was not needed (according to the client). Then for iq this packet should not be blocked.

I think we need to add NPE protection, and make sure that this check is done only for message stanzas.

wojciech.kapcia@tigase.net commented 1 year ago

I concur. The check was intended only for message stanzas (as it dealt with that particular stanza). Other stanzas are processed in other parts of the system and have different semantics. AFAIR all message stanzas must possess to attribute and only IQ and Presence can do without (with the notion, they should be processed by the server on client behalf)

I added check for if the packet is a Message and NPE protection.

Andrzej Wójcik (Tigase) commented 1 year ago

@wojtek I'm not sure, but I think it may be allowed for message to be sent by the server to the client with empty from attribute if it is generated by the server (ie. Message Carbons). In response to this message, client could send <message/> with type set to error, but it would get to set to null in this case.

Andrzej Wójcik (Tigase) commented 1 year ago

But I think that your fix may cover this case as well.

wojciech.kapcia@tigase.net commented 1 year ago

Hmm, that could be possible though I added null check (and unittest), so it should be ok.

I just updated tigase.im.

wojciech.kapcia@tigase.net batch edited 3 months ago

Name	Previous Value	Current Value
Iterations	empty	tigase-server-8.4.0

Type	Bug
Priority	Normal
Assignee	wojciech.kapcia@tigase.net
Version	tigase-server-8.4.0

Iterations

tigase-server-8.4.0 Closed

#1344 Closed

Create pre-processor that would filter out error messages

Issue Votes (0)

Watchers (2)

Reference

tigase/_server/server-core#1351