Merge pull request #10 from fourbites/deserialization-fixes

Deserialization fixes

core/src/main/java/com/threerings/io/BasicStreamers.java

@@ -27,6 +27,29 @@ import com.google.common.collect.Sets;
  */
 public class BasicStreamers
 {
+    /**
+     * The maximum number of elements allowed in a deserialized array, collection, or map. This
+     * prevents a malicious client from sending a huge size value to trigger an OutOfMemoryError.
+     */
+    public static final int MAX_CONTAINER_SIZE = 65536;
+
+    /**
+     * Validates that a container size read from the stream is non-negative and within bounds.
+     *
+     * @param size the size value read from the stream
+     * @return the validated size
+     * @throws IOException if the size is negative or exceeds {@link #MAX_CONTAINER_SIZE}
+     */
+    public static int validateSize (int size)
+        throws IOException
+    {
+        if (size < 0 || size > MAX_CONTAINER_SIZE) {
+            throw new IOException(
+                "Invalid container size: " + size + " (max " + MAX_CONTAINER_SIZE + ")");
+        }
+        return size;
+    }
+
     public static final Map<Class<?>, Streamer> BSTREAMERS =
         ImmutableMap.<Class<?>, Streamer>builder()
         .put(Boolean.class, new BooleanStreamer())
@@ -445,7 +468,7 @@ public class BasicStreamers
         public Object createObject (ObjectInputStream in)
             throws IOException, ClassNotFoundException
         {
-            int size = in.readInt();
+            int size = validateSize(in.readInt());
             Collection<Object> coll = createCollection(size);
             for (int ii = 0; ii < size; ii++) {
                 coll.add(in.readObject());
@@ -514,7 +537,7 @@ public class BasicStreamers
         public Object createObject (ObjectInputStream in)
             throws IOException, ClassNotFoundException
         {
-            int size = in.readInt();
+            int size = validateSize(in.readInt());
             Map<Object, Object> map = createMap(size);
             for (int ii = 0; ii < size; ii++) {
                 map.put(in.readObject(), in.readObject());
@@ -550,7 +573,7 @@ public class BasicStreamers
         public Object createObject (ObjectInputStream in)
             throws IOException, ClassNotFoundException
         {
-            int size = in.readInt();
+            int size = validateSize(in.readInt());
             Multiset<Object> set = createMultiset(size);
             for (int ii = 0; ii < size; ii++) {
                 set.add(in.readObject(), in.readInt());
@@ -586,7 +609,7 @@ public class BasicStreamers
     public static boolean[] readBooleanArray (ObjectInputStream ins)
         throws IOException
     {
-        boolean[] value = new boolean[ins.readInt()];
+        boolean[] value = new boolean[validateSize(ins.readInt())];
         int ecount = value.length;
         for (int ii = 0; ii < ecount; ii++) {
             value[ii] = ins.readBoolean();
@@ -597,7 +620,7 @@ public class BasicStreamers
     public static byte[] readByteArray (ObjectInputStream ins)
         throws IOException
     {
-        byte[] value = new byte[ins.readInt()];
+        byte[] value = new byte[validateSize(ins.readInt())];
         int remain = value.length, offset = 0, read;
         while (remain > 0) {
             if ((read = ins.read(value, offset, remain)) > 0) {
@@ -613,7 +636,7 @@ public class BasicStreamers
     public static short[] readShortArray (ObjectInputStream ins)
         throws IOException
     {
-        short[] value = new short[ins.readInt()];
+        short[] value = new short[validateSize(ins.readInt())];
         int ecount = value.length;
         for (int ii = 0; ii < ecount; ii++) {
             value[ii] = ins.readShort();
@@ -624,7 +647,7 @@ public class BasicStreamers
     public static char[] readCharArray (ObjectInputStream ins)
         throws IOException
     {
-        char[] value = new char[ins.readInt()];
+        char[] value = new char[validateSize(ins.readInt())];
         int ecount = value.length;
         for (int ii = 0; ii < ecount; ii++) {
             value[ii] = ins.readChar();
@@ -635,7 +658,7 @@ public class BasicStreamers
     public static int[] readIntArray (ObjectInputStream ins)
         throws IOException
     {
-        int[] value = new int[ins.readInt()];
+        int[] value = new int[validateSize(ins.readInt())];
         int ecount = value.length;
         for (int ii = 0; ii < ecount; ii++) {
             value[ii] = ins.readInt();
@@ -646,7 +669,7 @@ public class BasicStreamers
     public static long[] readLongArray (ObjectInputStream ins)
         throws IOException
     {
-        long[] value = new long[ins.readInt()];
+        long[] value = new long[validateSize(ins.readInt())];
         int ecount = value.length;
         for (int ii = 0; ii < ecount; ii++) {
             value[ii] = ins.readLong();
@@ -657,7 +680,7 @@ public class BasicStreamers
     public static float[] readFloatArray (ObjectInputStream ins)
         throws IOException
     {
-        float[] value = new float[ins.readInt()];
+        float[] value = new float[validateSize(ins.readInt())];
         int ecount = value.length;
         for (int ii = 0; ii < ecount; ii++) {
             value[ii] = ins.readFloat();
@@ -668,7 +691,7 @@ public class BasicStreamers
     public static double[] readDoubleArray (ObjectInputStream ins)
         throws IOException
     {
-        double[] value = new double[ins.readInt()];
+        double[] value = new double[validateSize(ins.readInt())];
         int ecount = value.length;
         for (int ii = 0; ii < ecount; ii++) {
             value[ii] = ins.readDouble();
@@ -679,7 +702,7 @@ public class BasicStreamers
     public static Object[] readObjectArray (ObjectInputStream ins)
         throws IOException, ClassNotFoundException
     {
-        Object[] value = new Object[ins.readInt()];
+        Object[] value = new Object[validateSize(ins.readInt())];
         int ecount = value.length;
         for (int ii = 0; ii < ecount; ii++) {
             value[ii] = ins.readObject();

core/src/main/java/com/threerings/io/ObjectInputStream.java

@@ -7,11 +7,13 @@ package com.threerings.io;
 
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 
 import java.io.DataInputStream;
 import java.io.IOException;
 import java.io.InputStream;
 
+import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Maps;
 
@@ -27,6 +29,21 @@ import static com.threerings.NaryaLog.log;
  */
 public class ObjectInputStream extends DataInputStream
 {
+    /** The maximum allowed recursion depth when reading nested objects. */
+    public static final int MAX_OBJECT_DEPTH = 32;
+
+    /**
+     * Configures the set of allowed package prefixes for class deserialization. Only classes whose
+     * fully qualified names start with one of the specified prefixes will be allowed to load. This
+     * should be called once at server startup before any client connections are accepted.
+     *
+     * @param prefixes the allowed package prefixes (e.g., "com.threerings.", "com.samskivert.")
+     */
+    public static void setAllowedClassPrefixes (Set<String> prefixes)
+    {
+        _allowedPrefixes = ImmutableSet.copyOf(prefixes);
+    }
+
     /**
      * Constructs an object input stream which will read its data from the supplied source stream.
      */
@@ -63,6 +80,11 @@ public class ObjectInputStream extends DataInputStream
     public Object readObject ()
         throws IOException, ClassNotFoundException
     {
+        if (_depth >= MAX_OBJECT_DEPTH) {
+            throw new IOException("Maximum object nesting depth exceeded (" +
+                MAX_OBJECT_DEPTH + "); possible malicious input");
+        }
+        _depth++;
         try {
             // read the class mapping
             ClassMapping cmap = readClassMapping();
@@ -84,6 +106,8 @@ public class ObjectInputStream extends DataInputStream
 
         } catch (OutOfMemoryError oome) {
             throw (IOException)new IOException("Malformed object data").initCause(oome);
+        } finally {
+            _depth--;
         }
     }
 
@@ -222,6 +246,10 @@ public class ObjectInputStream extends DataInputStream
     protected ClassMapping createClassMapping (short code, String cname)
         throws IOException, ClassNotFoundException
     {
+        if (_allowedPrefixes != null) {
+            validateClassPrefix(code, cname);
+        }
+
         // resolve the class and streamer
         ClassLoader loader = (_loader != null) ? _loader :
             Thread.currentThread().getContextClassLoader();
@@ -241,6 +269,40 @@ public class ObjectInputStream extends DataInputStream
         return new ClassMapping(code, sclass, streamer);
     }
 
+    /**
+     * Validates that a class name is allowed by the configured prefix whitelist. Array prefixes
+     * ({@code [}) are stripped first. Primitive type descriptors ({@code I}, {@code Z}, etc.) are
+     * always allowed. Object type descriptors ({@code Lcom.threerings.Foo;}) are checked against
+     * the whitelist.
+     */
+    protected void validateClassPrefix (short code, String cname)
+        throws IOException
+    {
+        // strip array prefixes to get the component type descriptor
+        int start = 0;
+        while (start < cname.length() && cname.charAt(start) == '[') {
+            start++;
+        }
+        String desc = cname.substring(start);
+
+        // primitive descriptors (I, Z, B, S, C, J, F, D) are always safe
+        if (!desc.startsWith("L") || !desc.endsWith(";")) {
+            return;
+        }
+
+        // extract the class name from "Lcom.threerings.Foo;"
+        String className = desc.substring(1, desc.length() - 1);
+        for (String prefix : _allowedPrefixes) {
+            if (className.startsWith(prefix)) {
+                return;
+            }
+        }
+
+        log.warning("Blocked deserialization of non-whitelisted class",
+            "code", code, "class", cname);
+        throw new IOException("Deserialization of class not allowed: " + cname);
+    }
+
     /**
      * Reads an object from the input stream that was previously written with {@link
      * ObjectOutputStream#writeBareObject(Object)}.
@@ -331,6 +393,12 @@ public class ObjectInputStream extends DataInputStream
     /** An optional set of class name translations to use when unserializing objects. */
     protected Map<String, String> _translations;
 
+    /** Tracks the current object nesting depth to prevent stack overflow from malicious input. */
+    protected int _depth;
+
+    /** If set, only classes with names starting with one of these prefixes may be deserialized. */
+    protected static volatile Set<String> _allowedPrefixes;
+
     /** Used to activate verbose debug logging. */
     protected static final boolean STREAM_DEBUG = false;
 }

core/src/main/java/com/threerings/io/Streamer.java

@@ -639,7 +639,7 @@ public abstract class Streamer
         public Object createObject (ObjectInputStream in)
             throws IOException, ClassNotFoundException
         {
-            int length = in.readInt();
+            int length = BasicStreamers.validateSize(in.readInt());
             if (ObjectInputStream.STREAM_DEBUG) {
                 log.info(in.hashCode() + ": Creating array '" +
                     _componentType.getName() + "[" + length + "]'.");

core/src/main/java/com/threerings/presents/client/BlockingCommunicator.java

@@ -660,6 +660,7 @@ public class BlockingCommunicator extends Communicator
             InetAddress host = InetAddress.getByName(_client.getHostname());
             openChannel(host);
             _channel.configureBlocking(true);
+            _channel.socket().setKeepAlive(true);
 
             // our messages are framed (preceded by their length), so we use these helper streams
             // to manage the framing

core/src/main/java/com/threerings/presents/net/PingRequest.java

@@ -17,7 +17,7 @@ public class PingRequest extends UpstreamMessage
 {
     /** The number of milliseconds of idle upstream that are allowed to elapse before the client
      * sends a ping message to the server to let it know that we're still alive. */
-    public static final long PING_INTERVAL = 60 * 1000L;
+    public static final long PING_INTERVAL = 30 * 1000L;
 
     /**
      * Zero argument constructor used when unserializing an instance.