Harden Range.readObject() to reject bad cached hash code (#1633)

* A serialized Range can't store a bad cached hashCode.

Author: garydgregory

src/main/java/org/apache/commons/lang3/Range.java

@@ -16,6 +16,9 @@
  */
 package org.apache.commons.lang3;
 
+import java.io.IOException;
+import java.io.InvalidObjectException;
+import java.io.ObjectInputStream;
 import java.io.Serializable;
 import java.util.Comparator;
 import java.util.Objects;
@@ -106,6 +109,10 @@ public static <T> Range<T> between(final T fromInclusive, final T toInclusive, f
         return new Range<>(fromInclusive, toInclusive, comparator);
     }
 
+    private static int hash(final Object value1, final Object value2) {
+        return Objects.hash(value1, value2);
+    }
+
     /**
      * Creates a range using the specified element as both the minimum
      * and maximum in this range.
@@ -235,7 +242,7 @@ public static <T> Range<T> of(final T fromInclusive, final T toInclusive, final
             this.minimum = element2;
             this.maximum = element1;
         }
-        this.hashCode = Objects.hash(minimum, maximum);
+        this.hashCode = hash(minimum, maximum);
     }
 
     /**
@@ -527,6 +534,22 @@ public boolean isStartedBy(final T element) {
         return comparator.compare(element, minimum) == 0;
     }
 
+    /**
+     * See {@link Serializable}.
+     *
+     * @param in See {@link Serializable}.
+     * @throws IOException See {@link Serializable}.
+     * @throws ClassNotFoundException See {@link Serializable}.
+     */
+    private void readObject(final ObjectInputStream in) throws IOException, ClassNotFoundException {
+        in.defaultReadObject();
+        // Reject streams whose cached hashCode does not match the canonical hash of the deserialized minimum/maximum: a crafted stream cannot supply a forged
+        // value.
+        if (hashCode != hash(minimum, maximum)) {
+            throw new InvalidObjectException("Range hashCode does not match minimum/maximum.");
+        }
+    }
+
     /**
      * Gets the range as a {@link String}.
      *

src/test/java/org/apache/commons/lang3/RangeReadObjectTest.java

@@ -0,0 +1,54 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *      https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.commons.lang3;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertInstanceOf;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+import java.io.InvalidObjectException;
+
+import org.apache.commons.lang3.reflect.FieldUtils;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Tests that a serialized Range can't store a bad cached hashCode.
+ */
+class RangeReadObjectTest {
+
+    @Test
+    void testBadHashCodeRejected() throws Exception {
+        final Range<Integer> range = Range.of(1, 100);
+        final byte[] bytes = SerializationUtils.serialize(range);
+        // Locate the legitimate hashCode int in the serialized stream and overwrite it.
+        final int hashCode = (Integer) FieldUtils.readDeclaredField(range, "hashCode", true);
+        final byte[] edited = SerializationUtilsTest.replaceLastInt(bytes, hashCode, 0xDEADBEEF);
+        final SerializationException ex = assertThrows(SerializationException.class, () -> SerializationUtils.deserialize(edited),
+                "Bad hashCode in stream must be rejected with InvalidObjectException");
+        assertInstanceOf(InvalidObjectException.class, ex.getCause());
+        assertEquals("java.io.InvalidObjectException: Range hashCode does not match minimum/maximum.", ex.getMessage());
+    }
+
+    @Test
+    void testRoundTripPreservesCorrectHashCode() throws Exception {
+        final Range<String> range = Range.of("apple", "mango");
+        final Range<String> roundtrip = SerializationUtils.roundtrip(range);
+        assertEquals(range.hashCode(), roundtrip.hashCode(), "Round-trip serialization must preserve the correct hashCode");
+        assertEquals(range, roundtrip);
+    }
+}

src/test/java/org/apache/commons/lang3/SerializationUtilsTest.java

@@ -27,6 +27,7 @@
 import static org.junit.jupiter.api.Assertions.assertSame;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assertions.fail;
 
 import java.io.ByteArrayInputStream;
 import java.io.ByteArrayOutputStream;
@@ -65,8 +66,29 @@ class SerializationUtilsTest extends AbstractLangTest {
     static final String CLASS_NOT_FOUND_MESSAGE = "ClassNotFoundSerialization.readObject fake exception";
     protected static final String SERIALIZE_IO_EXCEPTION_MESSAGE = "Anonymous OutputStream I/O exception";
 
+    static byte[] intToBytes(final int v) {
+        return new byte[] { (byte) (v >>> 24), (byte) (v >>> 16), (byte) (v >>> 8), (byte) v };
+    }
+    static byte[] replaceLastInt(final byte[] src, final int from, final int to) {
+        final byte[] fromB = intToBytes(from);
+        final byte[] toB = intToBytes(to);
+        final byte[] out = src.clone();
+        for (int i = out.length - 4; i >= 0; i--) {
+            if (out[i] == fromB[0] && out[i + 1] == fromB[1] && out[i + 2] == fromB[2] && out[i + 3] == fromB[3]) {
+                out[i] = toB[0];
+                out[i + 1] = toB[1];
+                out[i + 2] = toB[2];
+                out[i + 3] = toB[3];
+                return out;
+            }
+        }
+        fail("No legitimate int in stream, serialization must keep hashCode in default field set");
+        return null;
+    }
     private String iString;
+
     private Integer iInteger;
+
     private HashMap<Object, Object> iMap;
 
     @BeforeEach
@@ -112,6 +134,7 @@ void testCloneUnserializable() {
         assertThrows(SerializationException.class, () -> SerializationUtils.clone(iMap));
     }
 
+    @SuppressWarnings("deprecation")
     @Test
     void testConstructor() {
         assertNotNull(new SerializationUtils());