diff --git a/geode-core/src/main/java/org/apache/geode/internal/InternalDataSerializer.java b/geode-core/src/main/java/org/apache/geode/internal/InternalDataSerializer.java index 5b785e87b43..c2d8e64abc6 100644 --- a/geode-core/src/main/java/org/apache/geode/internal/InternalDataSerializer.java +++ b/geode-core/src/main/java/org/apache/geode/internal/InternalDataSerializer.java @@ -2492,6 +2492,60 @@ public static String readString(DataInput in, byte header) throws IOException { return StaticSerialization.readString(in, header); } + private static final boolean READ_ALL_PROPERTY_VALUE_TYPES = + Boolean.getBoolean(GeodeGlossary.GEMFIRE_PREFIX + "readAllPropertyValueTypes"); + + /** + * Reads a {@link Properties} written by {@link DataSerializer#writeProperties} whose keys and + * values use only the simple encodings expected for connection credentials: strings, byte + * arrays and nulls. An entry encoded in any other form is rejected with an {@link IOException}. + * Set {@code gemfire.readAllPropertyValueTypes} to read value types other than strings and byte + * arrays, for deployments whose {@code AuthInitialize} stores such values. + */ + public static Properties readStringProperties(DataInput in) + throws IOException, ClassNotFoundException { + if (READ_ALL_PROPERTY_VALUE_TYPES) { + return DataSerializer.readProperties(in); + } + checkIn(in); + int size = readArrayLength(in); + if (size == -1) { + return null; + } + Properties props = new Properties(); + for (int index = 0; index < size; index++) { + Object key = readSimplePropertyValue(in); + Object value = readSimplePropertyValue(in); + props.put(key, value); + } + return props; + } + + private static Object readSimplePropertyValue(DataInput in) throws IOException { + checkIn(in); + DSCODE dscode = DscodeHelper.toDSCODE(in.readByte()); + if (dscode == null) { + throw new IOException("Unexpected wire format while reading properties"); + } + switch (dscode) { + case NULL: + case NULL_STRING: + return null; + case STRING: + return readStringUTFFromDataInput(in); + case HUGE_STRING: + return readHugeStringFromDataInput(in); + case STRING_BYTES: + return readStringBytesFromDataInput(in, in.readUnsignedShort()); + case HUGE_STRING_BYTES: + return readStringBytesFromDataInput(in, in.readInt()); + case BYTE_ARRAY: + return readByteArray(in); + default: + throw new IOException("Unexpected wire format while reading properties"); + } + } + /** * Just like readObject but make sure and pdx deserialized is not a PdxInstance. * diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ClientProxyMembershipID.java b/geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ClientProxyMembershipID.java index f50a346ca88..bae225f7872 100644 --- a/geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ClientProxyMembershipID.java +++ b/geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ClientProxyMembershipID.java @@ -37,8 +37,10 @@ import org.apache.geode.distributed.DurableClientAttributes; import org.apache.geode.distributed.internal.membership.InternalDistributedMember; import org.apache.geode.internal.HeapDataOutputStream; +import org.apache.geode.internal.InternalDataSerializer; import org.apache.geode.internal.net.SocketCreator; import org.apache.geode.internal.serialization.ByteArrayDataInput; +import org.apache.geode.internal.serialization.DSCODE; import org.apache.geode.internal.serialization.DataSerializableFixedID; import org.apache.geode.internal.serialization.DeserializationContext; import org.apache.geode.internal.serialization.KnownVersion; @@ -547,10 +549,22 @@ public KnownVersion[] getSerializationVersions() { public static ClientProxyMembershipID readCanonicalized(DataInput in) throws IOException, ClassNotFoundException { - ClientProxyMembershipID result = DataSerializer.readObject(in); + // The field is always written with DataSerializer.writeObject of a + // ClientProxyMembershipID (a fixed-id type) or a null, so the only shapes that appear + // on the wire are a NULL marker or a byte fixed-id header for this type. Read those two + // shapes directly instead of going through the general object reader. + final byte header = in.readByte(); + if (header == DSCODE.NULL.toByte()) { + return null; + } + if (header != DSCODE.DS_FIXED_ID_BYTE.toByte() || in.readByte() != CLIENT_PROXY_MEMBERSHIPID) { + throw new IOException("Unexpected wire format for ClientProxyMembershipID"); + } + final ClientProxyMembershipID result = new ClientProxyMembershipID(); + InternalDataSerializer.invokeFromData(result, in); // We can't canonicalize if we have no identity. // I only saw this happen in unit tests that serialize "new ClientProxyMembershipID()". - if (result == null || result.identity == null) { + if (result.identity == null) { return result; } return result.canonicalReference(); diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/EncryptorImpl.java b/geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/EncryptorImpl.java index e5ad190c48a..83cbef2d52a 100644 --- a/geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/EncryptorImpl.java +++ b/geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/EncryptorImpl.java @@ -66,6 +66,7 @@ import org.apache.geode.distributed.DistributedSystem; import org.apache.geode.distributed.internal.DistributionConfig; import org.apache.geode.internal.HeapDataOutputStream; +import org.apache.geode.internal.InternalDataSerializer; import org.apache.geode.internal.cache.tier.Encryptor; import org.apache.geode.internal.logging.InternalLogWriter; import org.apache.geode.internal.serialization.ByteArrayDataInput; @@ -687,7 +688,7 @@ static Properties getDecryptedCredentials(DataInputStream dis, DataOutputStream byte[] credentialBytes = decrypt.doFinal(encBytes); ByteArrayDataInput dinp = new ByteArrayDataInput(credentialBytes); - credentials = DataSerializer.readProperties(dinp); + credentials = InternalDataSerializer.readStringProperties(dinp); byte[] challengeRes = DataSerializer.readByteArray(dinp); // Check the challenge string if (!Arrays.equals(challenge, challengeRes)) { diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/Handshake.java b/geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/Handshake.java index b7083e69850..64ef7ae48bb 100644 --- a/geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/Handshake.java +++ b/geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/Handshake.java @@ -38,6 +38,7 @@ import org.apache.geode.distributed.DistributedSystem; import org.apache.geode.internal.ClassLoadUtils; import org.apache.geode.internal.HeapDataOutputStream; +import org.apache.geode.internal.InternalDataSerializer; import org.apache.geode.internal.cache.tier.ConnectionProxy; import org.apache.geode.internal.logging.InternalLogWriter; import org.apache.geode.internal.security.CallbackInstantiator; @@ -445,9 +446,9 @@ public static Properties readCredentials(DataInputStream dis, DataOutputStream d throwIfMissingRequiredCredentials(requireAuthentication, secureMode != CREDENTIALS_NONE); if (secureMode == CREDENTIALS_NORMAL) { if (requireAuthentication) { - credentials = DataSerializer.readProperties(dis); + credentials = InternalDataSerializer.readStringProperties(dis); } else { - DataSerializer.readProperties(dis); // ignore the credentials + InternalDataSerializer.readStringProperties(dis); // ignore the credentials } } else if (secureMode == CREDENTIALS_DHENCRYPT) { credentials = EncryptorImpl.getDecryptedCredentials(dis, dos, system, requireAuthentication, diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ServerConnection.java b/geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ServerConnection.java index 80ea9d37a4a..34fcc558bb5 100644 --- a/geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ServerConnection.java +++ b/geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ServerConnection.java @@ -48,7 +48,6 @@ import org.jetbrains.annotations.TestOnly; import org.apache.geode.CancelException; -import org.apache.geode.DataSerializer; import org.apache.geode.SystemFailure; import org.apache.geode.annotations.VisibleForTesting; import org.apache.geode.annotations.internal.MakeNotStatic; @@ -59,6 +58,7 @@ import org.apache.geode.distributed.DistributedSystem; import org.apache.geode.internal.Assert; import org.apache.geode.internal.HeapDataOutputStream; +import org.apache.geode.internal.InternalDataSerializer; import org.apache.geode.internal.cache.EventID; import org.apache.geode.internal.cache.InternalCache; import org.apache.geode.internal.cache.tier.Acceptor; @@ -1202,7 +1202,7 @@ long getUniqueId(Message message, long existingUniqueId) throws Exception { Properties credentials; try (ByteArrayDataInput dinp = new ByteArrayDataInput(credBytes)) { - credentials = DataSerializer.readProperties(dinp); + credentials = InternalDataSerializer.readStringProperties(dinp); } // When here, security is enforced on server, if login returns a subject, then it's the newly diff --git a/geode-core/src/test/java/org/apache/geode/internal/InternalDataSerializerPropertiesTest.java b/geode-core/src/test/java/org/apache/geode/internal/InternalDataSerializerPropertiesTest.java new file mode 100644 index 00000000000..6e4a776734b --- /dev/null +++ b/geode-core/src/test/java/org/apache/geode/internal/InternalDataSerializerPropertiesTest.java @@ -0,0 +1,72 @@ +/* + * 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 + * + * http://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.geode.internal; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import java.io.ByteArrayOutputStream; +import java.io.DataOutputStream; +import java.io.IOException; +import java.io.Serializable; +import java.util.Properties; + +import org.junit.Test; + +import org.apache.geode.DataSerializer; +import org.apache.geode.internal.serialization.ByteArrayDataInput; + +public class InternalDataSerializerPropertiesTest { + + @Test + public void readStringPropertiesReadsStringAndByteArrayValues() throws Exception { + final Properties written = new Properties(); + written.put("user", "alice"); + written.put("token", new byte[] {1, 2, 3}); + final ByteArrayOutputStream bytes = new ByteArrayOutputStream(); + DataSerializer.writeProperties(written, new DataOutputStream(bytes)); + + final Properties read = + InternalDataSerializer.readStringProperties(new ByteArrayDataInput(bytes.toByteArray())); + + assertThat(read.get("user")).isEqualTo("alice"); + assertThat((byte[]) read.get("token")).containsExactly(1, 2, 3); + } + + @Test + public void readStringPropertiesRejectsOtherValueTypes() throws Exception { + ValueProbe.deserialized = false; + final Properties written = new Properties(); + written.put("user", new ValueProbe()); + final ByteArrayOutputStream bytes = new ByteArrayOutputStream(); + DataSerializer.writeProperties(written, new DataOutputStream(bytes)); + + assertThatThrownBy(() -> InternalDataSerializer + .readStringProperties(new ByteArrayDataInput(bytes.toByteArray()))) + .isInstanceOf(IOException.class); + assertThat(ValueProbe.deserialized).isFalse(); + } + + public static class ValueProbe implements Serializable { + static boolean deserialized; + + private void readObject(java.io.ObjectInputStream in) + throws IOException, ClassNotFoundException { + in.defaultReadObject(); + deserialized = true; + } + } +} diff --git a/geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/ClientProxyMembershipIDTest.java b/geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/ClientProxyMembershipIDTest.java index 0eb4e93dfe3..15662d0c4a0 100644 --- a/geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/ClientProxyMembershipIDTest.java +++ b/geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/ClientProxyMembershipIDTest.java @@ -16,19 +16,24 @@ package org.apache.geode.internal.cache.tier.sockets; +import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; import java.io.ByteArrayOutputStream; +import java.io.DataOutputStream; import java.io.IOException; import java.io.ObjectInput; import java.io.ObjectInputStream; import java.io.ObjectOutput; import java.io.ObjectOutputStream; +import java.io.Serializable; import org.junit.Test; +import org.apache.geode.DataSerializer; import org.apache.geode.internal.NullDataOutputStream; import org.apache.geode.internal.serialization.ByteArrayDataInput; +import org.apache.geode.internal.serialization.DSCODE; public class ClientProxyMembershipIDTest { @@ -61,4 +66,49 @@ public void readExternalThrowsIfIdentityIsTooShort() throws IOException { .hasMessageContaining("identity length is too small"); } + + @Test + public void readCanonicalizedRejectsUnexpectedHeader() throws Exception { + ProbeObject.deserialized = false; + + final ByteArrayOutputStream bytes = new ByteArrayOutputStream(); + bytes.write(DSCODE.SERIALIZABLE.toByte()); + final ObjectOutputStream objectOut = new ObjectOutputStream(bytes); + objectOut.writeObject(new ProbeObject()); + objectOut.close(); + final ByteArrayDataInput in = new ByteArrayDataInput(bytes.toByteArray()); + + assertThatThrownBy(() -> ClientProxyMembershipID.readCanonicalized(in)) + .isInstanceOf(IOException.class); + assertThat(ProbeObject.deserialized).isFalse(); + } + + @Test + public void readCanonicalizedReadsNull() throws Exception { + final ByteArrayOutputStream bytes = new ByteArrayOutputStream(); + DataSerializer.writeObject(null, new DataOutputStream(bytes)); + final ByteArrayDataInput in = new ByteArrayDataInput(bytes.toByteArray()); + + assertThat(ClientProxyMembershipID.readCanonicalized(in)).isNull(); + } + + @Test + public void readCanonicalizedReadsFixedIdForm() throws Exception { + final ClientProxyMembershipID written = new ClientProxyMembershipID(); + final ByteArrayOutputStream bytes = new ByteArrayOutputStream(); + DataSerializer.writeObject(written, new DataOutputStream(bytes)); + final ByteArrayDataInput in = new ByteArrayDataInput(bytes.toByteArray()); + + assertThat(ClientProxyMembershipID.readCanonicalized(in)).isNotNull(); + } + + public static class ProbeObject implements Serializable { + static boolean deserialized; + + private void readObject(java.io.ObjectInputStream in) + throws IOException, ClassNotFoundException { + in.defaultReadObject(); + deserialized = true; + } + } }