CVE-2020-8929: Other (NVD-CWE-Other)
value: A mis-handling of invalid unicode characters in the Java implementation of Tink versions prior to 1.5 allows an attacker to change the ID part of a ciphertext, which result in the creation of a second ciphertext that can decrypt to the same plaintext. This can be a problem with encrypting deterministic AEAD with a single key, and rely on a unique ciphertext-per-plaintext.
CWE description: NVD is only using a subset of CWE for mapping instead of the entire CWE, and the weakness type is not covered by that subset.
CVSS v3 base score: 5.3
Published: 2020-10-19T13:15Z
Repository: https://github.com/google/tink
Language: Other
Files changed: java_src/src/main/java/com/google/crypto/tink/BUILD.bazel, java_src/src/main/java/com/google/crypto/tink/PrimitiveSet.java, java_src/src/test/java/com/google/crypto/tink/BUILD.bazel, java_src/src/test/java/com/google/crypto/tink/PrimitiveSetTest.java
Commit date: 2020-10-12 16:03:15 -0700
Commit message: Fixing ciphertext malleability issue in Java caused by storing the ciphertext prefix in a hashmap keyed by UTF8 encoded strings, instead of byte arrays, leading to the ability to retrieve keys with IDs that happen to be invalid Unicode strings with a changed ID.

PiperOrigin-RevId: 336763863

Diff: diff --git a/java_src/src/main/java/com/google/crypto/tink/BUILD.bazel b/java_src/src/main/java/com/google/crypto/tink/BUILD.bazel
index 712b4ef1..59f10d24 100644
--- a/java_src/src/main/java/com/google/crypto/tink/BUILD.bazel
+++ b/java_src/src/main/java/com/google/crypto/tink/BUILD.bazel
@@ -476,19 +476,21 @@ java_library(
     name = "primitive_set",
     srcs = ["PrimitiveSet.java"],
     deps = [
         ":crypto_format",
         "//proto:tink_java_proto",
+        "//src/main/java/com/google/crypto/tink/subtle:hex",
     ],
 )
 
 android_library(
     name = "primitive_set-android",
     srcs = ["PrimitiveSet.java"],
     deps = [
         ":crypto_format-android",
         "//proto:tink_java_proto_lite",
+        "//src/main/java/com/google/crypto/tink/subtle:hex",
     ],
 )
 
 java_library(
     name = "registry_cluster",
diff --git a/java_src/src/main/java/com/google/crypto/tink/PrimitiveSet.java b/java_src/src/main/java/com/google/crypto/tink/PrimitiveSet.java
index 86b59b66..e5ba18d7 100644
--- a/java_src/src/main/java/com/google/crypto/tink/PrimitiveSet.java
+++ b/java_src/src/main/java/com/google/crypto/tink/PrimitiveSet.java
@@ -17,11 +17,11 @@
 package com.google.crypto.tink;
 
 import com.google.crypto.tink.proto.KeyStatusType;
 import com.google.crypto.tink.proto.Keyset;
 import com.google.crypto.tink.proto.OutputPrefixType;
-import java.nio.charset.Charset;
+import com.google.crypto.tink.subtle.Hex;
 import java.security.GeneralSecurityException;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
@@ -47,11 +47,10 @@ import java.util.concurrent.ConcurrentMap;
  * <p>PrimitiveSet is a public class to allow its use in implementations of custom primitives.
  *
  * @since 1.0.0
  */
 public final class PrimitiveSet<P> {
-  private static final Charset UTF_8 = Charset.forName("UTF-8");
   /**
    * A single entry in the set. In addition to the actual primitive it holds also some extra
    * information about the primitive.
    */
   public static final class Entry<P> {
@@ -115,11 +114,11 @@ public final class PrimitiveSet<P> {
     return getPrimitive(CryptoFormat.RAW_PREFIX);
   }
 
   /** @return the entries with primitive identifed by {@code identifier}. */
   public List<Entry<P>> getPrimitive(final byte[] identifier) {
-    List<Entry<P>> found = primitives.get(new String(identifier, UTF_8));
+    List<Entry<P>> found = primitives.get(new Prefix(identifier));
     return found != null ? found : Collections.<Entry<P>>emptyList();
   }
 
   /** Returns the entries with primitives identified by the ciphertext prefix of {@code key}. */
   protected List<Entry<P>> getPrimitive(Keyset.Key key) throws GeneralSecurityException {
@@ -134,12 +133,12 @@ public final class PrimitiveSet<P> {
   /**
    * The primitives are stored in a hash map of (ciphertext prefix, list of primivies sharing the
    * prefix). This allows quickly retrieving the list of primitives sharing some particular prefix.
    * Because all RAW keys are using an empty prefix, this also quickly allows retrieving them.
    */
-  private ConcurrentMap<java.lang.String, List<Entry<P>>> primitives =
-      new ConcurrentHashMap<java.lang.String, List<Entry<P>>>();
+  private final ConcurrentMap<Prefix, List<Entry<P>>> primitives =
+      new ConcurrentHashMap<Prefix, List<Entry<P>>>();
 
   private Entry<P> primary;
   private final Class<P> primitiveClass;
 
   private PrimitiveSet(Class<P> primitiveClass) {
@@ -183,12 +182,12 @@ public final class PrimitiveSet<P> {
             key.getStatus(),
             key.getOutputPrefixType(),
             key.getKeyId());
     List<Entry<P>> list = new ArrayList<Entry<P>>();
     list.add(entry);
-    // Cannot use [] as keys in hash map, convert to string.
-    String identifier = new String(entry.getIdentifier(), UTF_8);
+    // Cannot use [] as keys in hash map, convert to Prefix wrapper class.
+    Prefix identifier = new Prefix(entry.getIdentifier());
     List<Entry<P>> existing = primitives.put(identifier, Collections.unmodifiableList(list));
     if (existing != null) {
       List<Entry<P>> newList = new ArrayList<Entry<P>>();
       newList.addAll(existing);
       newList.add(entry);
@@ -198,6 +197,47 @@ public final class PrimitiveSet<P> {
   }
 
   public Class<P> getPrimitiveClass() {
     return primitiveClass;
   }
+
+  private static class Prefix implements Comparable<Prefix> {
+    private final byte[] prefix;
+
+    private Prefix(byte[] prefix) {
+      this.prefix = Arrays.copyOf(prefix, prefix.length);
+    }
+
+    @Override
+    public int hashCode() {
+      return Arrays.hashCode(prefix);
+    }
+
+    @Override
+    public boolean equals(Object o) {
+      if (!(o instanceof Prefix)) {
+        return false;
+      }
+      Prefix other = (Prefix) o;
+      return Arrays.equals(prefix, other.prefix);
+    }
+
+    @Override
+    public int compareTo(Prefix o) {
+      if (prefix.length != o.prefix.length) {
+        return prefix.length - o.prefix.length;
+      }
+      for (int i = 0; i < prefix.length; i++) {
+        if (prefix[i] != o.prefix[i]) {
+          return prefix[i] - o.prefix[i];
+        }
+      }
+      return 0;
+    }
+
+    @Override
+    public String toString() {
+      return Hex.encode(prefix);
+    }
+  }
+
 }
diff --git a/java_src/src/test/java/com/google/crypto/tink/BUILD.bazel b/java_src/src/test/java/com/google/crypto/tink/BUILD.bazel
index 5b67eb2c..44f97321 100644
--- a/java_src/src/test/java/com/google/crypto/tink/BUILD.bazel
+++ b/java_src/src/test/java/com/google/crypto/tink/BUILD.bazel
@@ -170,10 +170,11 @@ java_test(
     deps = [
         "//proto:tink_java_proto",
         "//src/main/java/com/google/crypto/tink:crypto_format",
         "//src/main/java/com/google/crypto/tink:mac",
         "//src/main/java/com/google/crypto/tink:primitive_set",
+        "//src/main/java/com/google/crypto/tink/subtle:hex",
         "//src/main/java/com/google/crypto/tink/testing:test_util",
         "@maven//:com_google_truth_truth",
         "@maven//:junit_junit",
     ],
 )
diff --git a/java_src/src/test/java/com/google/crypto/tink/PrimitiveSetTest.java b/java_src/src/test/java/com/google/crypto/tink/PrimitiveSetTest.java
index 8a7040b1..479823b2 100644
--- a/java_src/src/test/java/com/google/crypto/tink/PrimitiveSetTest.java
+++ b/java_src/src/test/java/com/google/crypto/tink/PrimitiveSetTest.java
@@ -16,29 +16,29 @@
 
 package com.google.crypto.tink;
 
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.crypto.tink.testing.TestUtil.assertExceptionContains;
+import static java.nio.charset.StandardCharsets.UTF_8;
 import static org.junit.Assert.assertArrayEquals;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.fail;
 
 import com.google.crypto.tink.proto.KeyStatusType;
 import com.google.crypto.tink.proto.Keyset.Key;
 import com.google.crypto.tink.proto.OutputPrefixType;
-import java.nio.charset.Charset;
+import com.google.crypto.tink.subtle.Hex;
 import java.security.GeneralSecurityException;
 import java.util.ArrayList;
 import java.util.List;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
 
 /** Tests for PrimitiveSet. */
 @RunWith(JUnit4.class)
 public class PrimitiveSetTest {
-  private static final Charset UTF_8 = Charset.forName("UTF-8");
 
   private static class DummyMac1 implements Mac {
     public DummyMac1() {}
 
     @Override
@@ -89,49 +89,47 @@ public class PrimitiveSetTest {
             .setStatus(KeyStatusType.ENABLED)
             .setOutputPrefixType(OutputPrefixType.LEGACY)
             .build();
     pset.addPrimitive(new DummyMac1(), key3);
 
-    assertEquals(3, pset.getAll().size());
+    assertThat(pset.getAll()).hasSize(3);
 
     List<PrimitiveSet.Entry<Mac>> entries = pset.getPrimitive(key1);
-    assertEquals(1, entries.size());
+    assertThat(entries).hasSize(1);
     PrimitiveSet.Entry<Mac> entry = entries.get(0);
     assertEquals(
-        DummyMac1.class.getSimpleName(),
-        new String(entry.getPrimitive().computeMac(null), "UTF-8"));
+        DummyMac1.class.getSimpleName(), new String(entry.getPrimitive().computeMac(null), UTF_8));
     assertEquals(KeyStatusType.ENABLED, entry.getStatus());
     assertEquals(CryptoFormat.TINK_START_BYTE, entry.getIdentifier()[0]);
     assertArrayEquals(CryptoFormat.getOutputPrefix(key1), entry.getIdentifier());
     assertEquals(entry.getKeyId(), 1);
 
     entries = pset.getPrimitive(key2);
-    assertEquals(1, entries.size());
+    assertThat(entries).hasSize(1);
     entry = entries.get(0);
     assertEquals(
         DummyMac2.class.getSimpleName(),
-        new String(entry.getPrimitive().computeMac(null), "UTF-8"));
+        new String(entry.getPrimitive().computeMac(null), UTF_8));
     assertEquals(KeyStatusType.ENABLED, entry.getStatus());
-    assertEquals(0, entry.getIdentifier().length);
+    assertThat(entry.getIdentifier()).isEmpty();
     assertArrayEquals(CryptoFormat.getOutputPrefix(key2), entry.getIdentifier());
     assertEquals(2, entry.getKeyId());
 
     entries = pset.getPrimitive(key3);
-    assertEquals(1, entries.size());
+    assertThat(entries).hasSize(1);
     entry = entries.get(0);
     assertEquals(
-        DummyMac1.class.getSimpleName(),
-        new String(entry.getPrimitive().computeMac(null), "UTF-8"));
+        DummyMac1.class.getSimpleName(), new String(entry.getPrimitive().computeMac(null), UTF_8));
     assertEquals(KeyStatusType.ENABLED, entry.getStatus());
     assertEquals(CryptoFormat.LEGACY_START_BYTE, entry.getIdentifier()[0]);
     assertArrayEquals(CryptoFormat.getOutputPrefix(key3), entry.getIdentifier());
     assertEquals(entry.getKeyId(), 3);
 
     entry = pset.getPrimary();
     assertEquals(
         DummyMac2.class.getSimpleName(),
-        new String(entry.getPrimitive().computeMac(null), "UTF-8"));
+        new String(entry.getPrimitive().computeMac(null), UTF_8));
     assertEquals(KeyStatusType.ENABLED, entry.getStatus());
     assertArrayEquals(CryptoFormat.getOutputPrefix(key2), entry.getIdentifier());
     assertEquals(2, entry.getKeyId());
   }
 
@@ -274,6 +272,39 @@ public class PrimitiveSetTest {
       fail("Expected GeneralSecurityException.");
     } catch (GeneralSecurityException e) {
       assertExceptionContains(e, "only ENABLED key is allowed");
     }
   }
+
+  @Test
+  public void testPrefix_isUnique() throws Exception {
+    PrimitiveSet<Mac> pset = PrimitiveSet.newPrimitiveSet(Mac.class);
+    Key key1 =
+        Key.newBuilder()
+            .setKeyId(0xffffffff)
+            .setStatus(KeyStatusType.ENABLED)
+            .setOutputPrefixType(OutputPrefixType.TINK)
+            .build();
+    pset.addPrimitive(new DummyMac1(), key1);
+    Key key2 =
+        Key.newBuilder()
+            .setKeyId(0xffffffdf)
+            .setStatus(KeyStatusType.ENABLED)
+            .setOutputPrefixType(OutputPrefixType.RAW)
+            .build();
+    pset.setPrimary(pset.addPrimitive(new DummyMac2(), key2));
+    Key key3 =
+        Key.newBuilder()
+            .setKeyId(0xffffffef)
+            .setStatus(KeyStatusType.ENABLED)
+            .setOutputPrefixType(OutputPrefixType.LEGACY)
+            .build();
+    pset.addPrimitive(new DummyMac1(), key3);
+
+    assertThat(pset.getAll()).hasSize(3);
+
+    assertThat(pset.getPrimitive(Hex.decode("01ffffffff"))).hasSize(1);
+    assertThat(pset.getPrimitive(Hex.decode("01ffffffef"))).isEmpty();
+    assertThat(pset.getPrimitive(Hex.decode("00ffffffff"))).isEmpty();
+    assertThat(pset.getPrimitive(Hex.decode("00ffffffef"))).hasSize(1);
+  }
 }

Vulnerable code: import java.nio.charset.Charset;
  private static final Charset UTF_8 = Charset.forName("UTF-8");
    List<Entry<P>> found = primitives.get(new String(identifier, UTF_8));
  private ConcurrentMap<java.lang.String, List<Entry<P>>> primitives =
      new ConcurrentHashMap<java.lang.String, List<Entry<P>>>();
    // Cannot use [] as keys in hash map, convert to string.
    String identifier = new String(entry.getIdentifier(), UTF_8);
import java.nio.charset.Charset;
  private static final Charset UTF_8 = Charset.forName("UTF-8");
    assertEquals(3, pset.getAll().size());
    assertEquals(1, entries.size());
        DummyMac1.class.getSimpleName(),
        new String(entry.getPrimitive().computeMac(null), "UTF-8"));
    assertEquals(1, entries.size());
        new String(entry.getPrimitive().computeMac(null), "UTF-8"));
    assertEquals(0, entry.getIdentifier().length);
    assertEquals(1, entries.size());
        DummyMac1.class.getSimpleName(),
        new String(entry.getPrimitive().computeMac(null), "UTF-8"));
        new String(entry.getPrimitive().computeMac(null), "UTF-8"));
Fixed code:         "//src/main/java/com/google/crypto/tink/subtle:hex",
        "//src/main/java/com/google/crypto/tink/subtle:hex",
import com.google.crypto.tink.subtle.Hex;
    List<Entry<P>> found = primitives.get(new Prefix(identifier));
  private final ConcurrentMap<Prefix, List<Entry<P>>> primitives =
      new ConcurrentHashMap<Prefix, List<Entry<P>>>();
    // Cannot use [] as keys in hash map, convert to Prefix wrapper class.
    Prefix identifier = new Prefix(entry.getIdentifier());

  private static class Prefix implements Comparable<Prefix> {
    private final byte[] prefix;

    private Prefix(byte[] prefix) {
      this.prefix = Arrays.copyOf(prefix, prefix.length);
    }

    @Override
    public int hashCode() {
      return Arrays.hashCode(prefix);
    }

    @Override
    public boolean equals(Object o) {
      if (!(o instanceof Prefix)) {
        return false;
      }
      Prefix other = (Prefix) o;
      return Arrays.equals(prefix, other.prefix);
    }

    @Override
    public int compareTo(Prefix o) {
      if (prefix.length != o.prefix.length) {
        return prefix.length - o.prefix.length;
      }
      for (int i = 0; i < prefix.length; i++) {
        if (prefix[i] != o.prefix[i]) {
          return prefix[i] - o.prefix[i];
        }
      }
      return 0;
    }

    @Override
    public String toString() {
      return Hex.encode(prefix);
    }
  }

        "//src/main/java/com/google/crypto/tink/subtle:hex",
import static java.nio.charset.StandardCharsets.UTF_8;
import com.google.crypto.tink.subtle.Hex;
    assertThat(pset.getAll()).hasSize(3);
    assertThat(entries).hasSize(1);
        DummyMac1.class.getSimpleName(), new String(entry.getPrimitive().computeMac(null), UTF_8));
    assertThat(entries).hasSize(1);
        new String(entry.getPrimitive().computeMac(null), UTF_8));
    assertThat(entry.getIdentifier()).isEmpty();
    assertThat(entries).hasSize(1);
        DummyMac1.class.getSimpleName(), new String(entry.getPrimitive().computeMac(null), UTF_8));
        new String(entry.getPrimitive().computeMac(null), UTF_8));

  @Test
  public void testPrefix_isUnique() throws Exception {
    PrimitiveSet<Mac> pset = PrimitiveSet.newPrimitiveSet(Mac.class);
    Key key1 =
        Key.newBuilder()
            .setKeyId(0xffffffff)
            .setStatus(KeyStatusType.ENABLED)
            .setOutputPrefixType(OutputPrefixType.TINK)
            .build();
    pset.addPrimitive(new DummyMac1(), key1);
    Key key2 =
        Key.newBuilder()
            .setKeyId(0xffffffdf)
            .setStatus(KeyStatusType.ENABLED)
            .setOutputPrefixType(OutputPrefixType.RAW)
            .build();
    pset.setPrimary(pset.addPrimitive(new DummyMac2(), key2));
    Key key3 =
        Key.newBuilder()
            .setKeyId(0xffffffef)
            .setStatus(KeyStatusType.ENABLED)
            .setOutputPrefixType(OutputPrefixType.LEGACY)
            .build();
    pset.addPrimitive(new DummyMac1(), key3);

    assertThat(pset.getAll()).hasSize(3);

    assertThat(pset.getPrimitive(Hex.decode("01ffffffff"))).hasSize(1);
    assertThat(pset.getPrimitive(Hex.decode("01ffffffef"))).isEmpty();
    assertThat(pset.getPrimitive(Hex.decode("00ffffffff"))).isEmpty();
    assertThat(pset.getPrimitive(Hex.decode("00ffffffef"))).hasSize(1);
  }