[activemq-dev] DefaultWireFormat vulnerable on Internet

classic Classic list List threaded Threaded
1 message Options
Reply | Threaded
Open this post in threaded view

[activemq-dev] DefaultWireFormat vulnerable on Internet

Hakan Guleryuz-2

First of all I would like to thank everyone involved in this great
effort. ActiveMQ is a great and very well designed software. We are
using and tweaking it with great pleasure and joy, and learning a lot
from it as well.

This is my first post, and I would like to point out an issue that
causes the server to stop functioning with java.lang.OutOfMemoryError.
The issue is spotted on ActiveMQ-3.0 but looking at the release source
code for ActiveMQ-3.1, it still exists in it as well.
The issue makes the server very vulnerable towards random garbage data,
port scanners, worms, and others nasty stuff on the internet.
We had our servers go down for no reason in a few days until we spotted
the reason.
(one could argue why we open the port 61616 on internet or a big LAN,
but our case requires it)

A simple demonstration is the following:
do a telnet to port 61616, and then type
 > 'SPACE' (ASCII 32) 'a' 's' 'd' 'f'

What will happen is the following:
ASCII 32 will match "Packet type"
public static final int CACHED_VALUE_COMMAND = 32;

and then
"readPacket(DataInput dataIn, PacketReader reader)"
method of "DefaultWireFormat" will be called
which in return will parse 'a' 's' 'd' 'f' as an int in line 123:
        int length = dataIn.readInt();
then will try to allocate a byte buffer of "length"
        byte[] data = new byte[length];
which will cause a java.lang.OutOfMemory exception on the server.

Our version of "readPacket" method of "DefaultWireFormat" currently
looks like this:

     protected synchronized final Packet readPacket(DataInput dataIn,
PacketReader reader) throws IOException {
         synchronized (readMutex) {
             Packet packet = reader.createPacket();
             int length = dataIn.readInt();

             //********************<START> ADDED by
             // We must validate this length is within limits for all
             // This hardens the server in case of random TCP/IP attacks
and worms
             if (length > 1024*256 /* 256k */) {
                 String errStr = "Invalid packet length: " + length + "
(PacketType=" + reader.getPacketType() + ")";
                 throw new IOException(errStr);
             //********************<END> ADDED by

             byte[] data = new byte[length];
             //then splat into the internal datainput
             reader.buildPacket(packet, internalDataIn);
             return packet;

This is not the best solution. Restricting all packets to be at most
256k in size. I know it should be more intelligent than this. The packet
type is known at this point, and a length sanity check could be done per
"Packet type". This is just a quick fix which solved our case.

We also tried to enforce "Packet ordering" with some added code on
TCPTransportChannel with a rule saying:
"all connections must first receive WireFormatInfo, and then ConnectionInfo"
but this rule does not stand correct with the ReliableTransportChannel,
because ReliableTransportChannel allows new connection continue from
where they left with new connections, so any "Packet type" can be
received with a new TCP connection. So our "order validation" code could
not be written.

Another tweak I did to make the server less vulnerable for "port
scanners", worms, etc, is to change the attitude towards unrecognized
Packet types.
In the "public Packet readPacket(int firstByte, DataInput dataIn)"
method of "AbstractDefaultWireFormat" there is a "default" case for the
big switch statement. It looked like this:

             default :
                 log.error("Could not find PacketReader for packet type: "
                         + AbstractPacket.getPacketTypeAsString(firstByte));
                 return null;

but we changed it to this:

             default :
                 //********************<START> ADDED by
                 // It is risky to continue if we are receiving
unrecognized packet types
                 // This could be a worm scan...
                 String errStr = "Could not find PacketReader for packet
type: "
                     + AbstractPacket.getPacketTypeAsString(firstByte);
                 throw new IOException(errStr);
                 //********************<END> ADDED by
//                log.error("Could not find PacketReader for packet type: "
//                        +
//                return null;

In short, we don't want unrecognized packet types to be allowed to
continue operation.

In fact, what I wanted to achieve but could not code in was to put some
sort of protocol magic to the default wire format ie. for any type of
wireformat or protocol, we would require a 4 letter magic (like "HTTP")
or "ACMQ". The aim here is to get rid of any port scanners scanning for
backdoors, or other random protocols on the 61616 port.
But, the reliable implementation stands in our way, since it allows a
new TCP connection to be used as a continuation of an old one, and there
is no control on what the client or server will send on the new channel.
The only alternative was to put the 4 letter magic to all packet types,
which would have increased the overall network footpring quite a lot.
So we had to skip this implementation.

Sorry for this long post.
I will post some other tweaks in very near future.

I can send the above modification as "diff" patches if anyone requires.
I hope our worries and modifications applies for others and we don't
have to keep a code branch of ActiveMQ ourselves.

Thanks to every one involved in ActiveMQ.
Hakan Guleryuz.

Chief Architect