Smack 4.0.2 Bug report involving fromFilter in IQReplyFilter

Hello,

I am currently working with Smack 4.0.2 on android (using the asmack 4.0.2 lib) and found the following bug related to SASL-Authentication to the facebook chat:

When connecting to chat.facebook.com via SASL PLAIN mechanism, I couldn’t connect and got the the following log output:

W/IQReplyFilter(7334): Rejected potentially spoofed reply to IQ-packet. Filter settings: packetId=25bp0-0, to=null, local=null, server=chat.facebook.com. Received packet with from=user.name.1@chat.facebook.com

When digging further into the issue, I found out that problem originates in the IQReplyFilter class which, according to the javadoc checks the “from” adress to ignore forged replies:

(Excerpt from javadoc)

We accept a from address if one of the following is true:
It matches the to address of the request.
The to address of the request was empty and the from address matches either the bare jid of the server or the (bare or full jid) of the client.
To to was our bare address and the from is empty.

Well, I thought, the facebook server responds with a “from” adress of user.name.1@chat.facebook.com, so actually IT SHOULD WORK right? (that’s case 2 of the conditions mentioned in the javadoc above: the from matches the bare jid of the client)

So why doesn’t it work?

**Digging further into the code of IQReplyFilter I found the problem: **(see marked-up code and explanation below)

public IQReplyFilter(IQ iqPacket, XMPPConnection conn) {
        to = iqPacket.getTo();
        if (conn.getUser() == null) {
            // We have not yet been assigned a username, this can happen if the connection is
            // in an early stage, i.e. when performing the SASL auth.
            local = null;
        } else {
            local = conn.getUser().toLowerCase(Locale.US);
        }
        server = conn.getServiceName().toLowerCase(Locale.US);
        packetId = iqPacket.getPacketID();         PacketFilter iqFilter = new OrFilter(new IQTypeFilter(IQ.Type.ERROR), new IQTypeFilter(IQ.Type.RESULT));
        PacketFilter idFilter = new PacketIDFilter(iqPacket);
        iqAndIdFilter = new AndFilter(iqFilter, idFilter);
        fromFilter = new OrFilter();
        fromFilter.addFilter(FromMatchesFilter.createFull(to));
        if (to == null) {
            if (local != null)
                fromFilter.addFilter(FromMatchesFilter.createBare(local));
            fromFilter.addFilter(FromMatchesFilter.createFull(server));
        }
        else if (local != null && to.toLowerCase(Locale.US).equals(StringUtils.parseBareAddress(local))) {
            fromFilter.addFilter(FromMatchesFilter.createFull(null));
        }
    }

So what happened here?

Well, because the username has not yet been assigned in the early stages of SASL authentication, “local” is set to null. This means that when in the same constructor a few lines down the fromFilter is built,** the local attribute is not yet set, and so it just creates a matching filter for the server jid **(in this case chat.facebook.com).

So when facebook responds** not with the bare server id but with the bare client jid in the “from” field** of the IQ packet (user.name.1@chat.facebook.com) it leads to the packet being rejected. (from filter incorrectly only matches server jid chat.facebook.com when it should also match client jid user.name.1@chat.facebook.com)

Possible fix:

for the time being I use my own subclass of IQReplyFilter which overwrites the accept method to not check the fromFilter and so basically let all the packets through regardless of the from attribute. This is working for testing purposes but obviously not satisfactory for security purposes.

A possible fix might include to somehow add the bare and full client jid to the fromFilter in later stages of the SASL auth when the username has been aquired? Sorry I don’t know enough about xmpp to really say…

P.S. Oh yeah, I hope this is the right place to report this issue? If not feel free to move it to the right place. I was also searching for an email adress to report this but couldn’t find any.

Could you show use the full stanza that is beeing rejected?

Flow schrieb:

Could you show use the full stanza that is beeing rejected?

Sure here is the full xml handshake from logcat (with SmackConfiguration.DEBUG_ENABLED = TRUE)

(sorry the format highlight button is missing on the reply form, so it’s hard to read…)

07-30 15:01:09.009: D/SMACK(9888): SENT (1): <stream:stream to=“” xmlns=“jabber:client” xmlns:stream=“http://etherx.jabber.org/streams” version=“1.0”>

07-30 15:01:09.939: D/SMACK(9888): RCV (1): <?xml version='1.0' ?><stream:stream from=‘chat.facebook.com’ id=‘1’ version=‘1.0’ xmlns:stream=‘http://etherx.jabber.org/streams’ xmlns=‘jabber:client’ xml:lang=‘en’>stream:featuresX-FACEBOOK-PLATFORM</mechan ism>PLAIN</stream:features>

07-30 15:01:09.939: D/SMACK(9888): SENT (1):

07-30 15:01:10.189: D/SMACK(9888): RCV (1):

07-30 15:01:11.399: D/SMACK(9888): SENT (1): <stream:stream to=“chat.facebook.com” xmlns=“jabber:client” xmlns:stream=“http://etherx.jabber.org/streams” version=“1.0”>

07-30 15:01:11.609: D/SMACK(9888): RCV (1): <?xml version='1.0' ?><stream:stream from=‘chat.facebook.com’ id=‘1’ version=‘1.0’ xmlns:stream=‘http://etherx.jabber.org/streams’ xmlns=‘jabber:client’ xml:lang=‘en’>stream:featuresX-FACEBOOK-PLATFORM</mechan ism>PLAIN</stream:features>

07-30 15:01:11.639: D/SMACK(9888): SENT (1): XXXXXXXXXXXXXXXXXXXXXXXXX

07-30 15:01:15.099: D/SMACK(9888): RCV (1):

07-30 15:01:15.099: D/SMACK(9888): SENT (1): <stream:stream to=“chat.facebook.com” xmlns=“jabber:client” xmlns:stream=“http://etherx.jabber.org/streams” version=“1.0”>

07-30 15:01:15.929: D/SMACK(9888): RCV (1): <?xml version='1.0' ?><stream:stream from=‘chat.facebook.com’ id=‘1’ version=‘1.0’ xmlns:stream=‘http://etherx.jabber.org/streams’ xmlns=‘jabber:client’ xml:lang=‘en’>stream:features</stream:features>

07-30 15:01:15.949: D/SMACK(9888): SENT (1): Smack

07-30 15:01:16.189: D/SMACK(9888): RCV (1): user.name.1@chat.facebook.com/Smack

07-30 15:01:16.189: W/IQReplyFilter(9888): Rejected potentially spoofed reply to IQ-packet. Filter settings: packetId=sqvTK-0, to=null, local=null, server=chat.facebook.com. Received packet with from=user.name.1@chat.facebook.com

07-30 15:01:16.199: W/IQReplyFilter(9888): Rejected potentially spoofed reply to IQ-packet. Filter settings: packetId=sqvTK-1, to=null, local=null, server=chat.facebook.com. Received packet with from=user.name.1@chat.facebook.com

Is this what you’re looking for?*
*

Yes, exactly. Thanks for the detailed report.

I once got a PR on github that addressed the same issue ( https://github.com/igniterealtime/Smack/pull/8 ), but sadly in an unacceptable way. Your report helped me to understand the problem and it appears that it’s easily fixable without sacraficing the security that IQReplyFilter provides: I think we can use a simpler filter for the bind stanza. But I will have to gather some more opinions about that. Logged as SMACK-590.

07-30 15:01:16.199: W/IQReplyFilter(9888): Rejected potentially spoofed reply to IQ-packet. Filter settings: packetId=sqvTK-1, to=null, local=null, server=chat.facebook.com. Received packet with from=user.name.1@chat.facebook.com

Out of curiosity: Where is the packet with the id sqvTK-1?

Glad I could help!

Flow schrieb:

07-30 15:01:16.199: W/IQReplyFilter(9888): Rejected potentially spoofed reply to IQ-packet. Filter settings: packetId=sqvTK-1, to=null, local=null, server=chat.facebook.com. Received packet with from=user.name.1@chat.facebook.com

Out of curiosity: Where is the packet with the id sqvTK-1?

Uhm, not sure I understand? Do you mean because there are two rejected packets sqvTK-1 and sqvTK-0? I had actually multiple connection requests, and then when I finally stepped through the last part in the code in IQReplyFilter, one packet probably was still in the queue from an earlier request or sth. like that. Can’t really remember

I had actually multiple connection requests
Allright, that would be an explaination we see two rejected packets.

I’ve uploaded https://github.com/Flowdalic/Smack/tree/smack590

Are you able to build the branch, test and report back if it works as expected?

You are using Gradle I see, I don’t really have experience with that. Would it be possible to send me a precompiled JAR for convenience (e.g. like asmack-android-8-4.0.2.jar)) or alternatively give me instructions on how to build it?

Nevermind, I figured it out. BUT I need asmack and not smack as I am deploying on android… I looked up the repo at https://github.com/Flowdalic/asmack but the new changes are not in there and there is only one branch (master). Would it be possible port the changes to asmack, maybe in a temporary branch asmack-590 or sth. like that? Then I could check it out and build it with gradle…

Thanks

You don’t need aSmack no more. Checkout the smack590 branch, run “gradle assemble” and collect all required jar’s and place them in your Android Project (e.g. by putting them into the libs/ folder). Note that you also need the minidns library.

Hi!

The problem still exists in the 4.1.0-alpha1-SNAPSHOT build.

Edit: I built the smack590 branch and now I’m getting NullPointerException.

Full stack trace:

Exception in thread “main” java.lang.NullPointerException

at org.jivesoftware.smack.filter.IQReplyFilter.(IQReplyFilter.java:89)

at org.jivesoftware.smack.AbstractXMPPConnection.createPacketCollectorAndSend(Abst ractXMPPConnection.java:642)

at org.jivesoftware.smack.AbstractXMPPConnection.bindResourceAndEstablishSession(A bstractXMPPConnection.java:446)

at org.jivesoftware.smack.tcp.XMPPTCPConnection.login(XMPPTCPConnection.java:278)

at org.jivesoftware.smack.AbstractXMPPConnection.login(AbstractXMPPConnection.java :352)

at com.test.Main.main(Main.java:29)

It seems in the IQReplyFilter constructor, there is a call:

local = conn.getUser().toLowerCase(Locale.US);

But the conn.getUser() call returns null because the user is not authenticated yet.

The problem still exists in the 4.1.0-alpha1-SNAPSHOT build.

As you have already discovered, the patch is not currently in the master branch and therefore not available as snapshot.

Edit: I built the smack590 branch and now I’m getting NullPointerException.
Thanks for reporting. I’ve pushed a new version of the patch. Please update and report back if it works now.

It seems it is working now. Thanks!