From 0b10440b61f3328d7478b93fbf27e879a0dbac91 Mon Sep 17 00:00:00 2001 From: Andrew Purtell Date: Mon, 14 Feb 2022 15:00:03 -0800 Subject: [PATCH] CVE-2021-22569: Improve performance of parsing unknown fields in Java An issue in protobuf-java allowed the interleaving of UnknownFieldSet fields in such a way that would be processed out of order. A small malicious payload can occupy the parser for several minutes by creating large numbers of short- lived objects that cause frequent, repeated pauses. Port fix from https://github.com/protocolbuffers/protobuf/pull/9371. It uses a TreeMap to process fields in a well ordered way. Reason: Security Test Plan: Unit tests --- Makefile.am | 1 + Makefile.in | 1 + .../com/google/protobuf/UnknownFieldSet.java | 179 +++++++++--------- .../UnknownFieldSetPerformanceTest.java | 76 ++++++++ 4 files changed, 171 insertions(+), 86 deletions(-) create mode 100644 java/src/test/java/com/google/protobuf/UnknownFieldSetPerformanceTest.java diff --git a/Makefile.am b/Makefile.am index c9286132b..aedd021c3 100644 --- a/Makefile.am +++ b/Makefile.am @@ -140,6 +140,7 @@ EXTRA_DIST = \ java/src/test/java/com/google/protobuf/TestUtil.java \ java/src/test/java/com/google/protobuf/TextFormatTest.java \ java/src/test/java/com/google/protobuf/UnknownFieldSetTest.java \ + java/src/test/java/com/google/protobuf/UnknownFieldSetPerformanceTest.java \ java/src/test/java/com/google/protobuf/UnmodifiableLazyStringListTest.java \ java/src/test/java/com/google/protobuf/WireFormatTest.java \ java/src/test/java/com/google/protobuf/multiple_files_test.proto \ diff --git a/Makefile.in b/Makefile.in index e87df3b5a..8c3f4e51b 100644 --- a/Makefile.in +++ b/Makefile.in @@ -489,6 +489,7 @@ EXTRA_DIST = \ java/src/test/java/com/google/protobuf/TestUtil.java \ java/src/test/java/com/google/protobuf/TextFormatTest.java \ java/src/test/java/com/google/protobuf/UnknownFieldSetTest.java \ + java/src/test/java/com/google/protobuf/UnknownFieldSetPerformanceTest.java \ java/src/test/java/com/google/protobuf/UnmodifiableLazyStringListTest.java \ java/src/test/java/com/google/protobuf/WireFormatTest.java \ java/src/test/java/com/google/protobuf/multiple_files_test.proto \ diff --git a/java/src/main/java/com/google/protobuf/UnknownFieldSet.java b/java/src/main/java/com/google/protobuf/UnknownFieldSet.java index 45e2e6e40..f02377cf0 100644 --- a/java/src/main/java/com/google/protobuf/UnknownFieldSet.java +++ b/java/src/main/java/com/google/protobuf/UnknownFieldSet.java @@ -57,7 +57,6 @@ import java.util.TreeMap; * @author kenton@google.com Kenton Varda */ public final class UnknownFieldSet implements MessageLite { - private UnknownFieldSet() {} /** Create a new {@link Builder}. */ public static Builder newBuilder() { @@ -80,16 +79,20 @@ public final class UnknownFieldSet implements MessageLite { return defaultInstance; } private static final UnknownFieldSet defaultInstance = - new UnknownFieldSet(Collections.emptyMap()); + new UnknownFieldSet(new TreeMap()); + + private UnknownFieldSet() { + fields = new TreeMap(); + } /** * Construct an {@code UnknownFieldSet} around the given map. The map is * expected to be immutable. */ - private UnknownFieldSet(final Map fields) { + private UnknownFieldSet(final TreeMap fields) { this.fields = fields; } - private Map fields; + private final TreeMap fields; @Override public boolean equals(final Object other) { @@ -196,8 +199,10 @@ public final class UnknownFieldSet implements MessageLite { /** Get the number of bytes required to encode this set. */ public int getSerializedSize() { int result = 0; - for (final Map.Entry entry : fields.entrySet()) { - result += entry.getValue().getSerializedSize(entry.getKey()); + if (!fields.isEmpty()) { + for (final Map.Entry entry : fields.entrySet()) { + result += entry.getValue().getSerializedSize(entry.getKey()); + } } return result; } @@ -281,62 +286,44 @@ public final class UnknownFieldSet implements MessageLite { // This constructor should never be called directly (except from 'create'). private Builder() {} - private Map fields; - - // Optimization: We keep around a builder for the last field that was - // modified so that we can efficiently add to it multiple times in a - // row (important when parsing an unknown repeated field). - private int lastFieldNumber; - private Field.Builder lastField; + private TreeMap fieldBuilders = + new TreeMap(); private static Builder create() { - Builder builder = new Builder(); - builder.reinitialize(); - return builder; + return new Builder(); } /** * Get a field builder for the given field number which includes any * values that already exist. */ - private Field.Builder getFieldBuilder(final int number) { - if (lastField != null) { - if (number == lastFieldNumber) { - return lastField; - } - // Note: addField() will reset lastField and lastFieldNumber. - addField(lastFieldNumber, lastField.build()); - } + private Field.Builder getFieldBuilder(int number) { if (number == 0) { return null; } else { - final Field existing = fields.get(number); - lastFieldNumber = number; - lastField = Field.newBuilder(); - if (existing != null) { - lastField.mergeFrom(existing); + Field.Builder builder = fieldBuilders.get(number); + if (builder == null) { + builder = Field.newBuilder(); + fieldBuilders.put(number, builder); } - return lastField; + return builder; } } /** * Build the {@link UnknownFieldSet} and return it. - * - *

Once {@code build()} has been called, the {@code Builder} will no - * longer be usable. Calling any method after {@code build()} will result - * in undefined behavior and can cause a {@code NullPointerException} to be - * thrown. */ public UnknownFieldSet build() { - getFieldBuilder(0); // Force lastField to be built. - final UnknownFieldSet result; - if (fields.isEmpty()) { + UnknownFieldSet result; + if (fieldBuilders.isEmpty()) { result = getDefaultInstance(); } else { - result = new UnknownFieldSet(Collections.unmodifiableMap(fields)); + TreeMap fields = new TreeMap(); + for (Map.Entry entry : fieldBuilders.entrySet()) { + fields.put(entry.getKey(), entry.getValue().build()); + } + result = new UnknownFieldSet(fields); } - fields = null; return result; } @@ -347,24 +334,22 @@ public final class UnknownFieldSet implements MessageLite { @Override public Builder clone() { - getFieldBuilder(0); // Force lastField to be built. - return UnknownFieldSet.newBuilder().mergeFrom( - new UnknownFieldSet(fields)); + Builder clone = UnknownFieldSet.newBuilder(); + for (Map.Entry entry : fieldBuilders.entrySet()) { + Integer key = entry.getKey(); + Field.Builder value = entry.getValue(); + clone.fieldBuilders.put(key, value.clone()); + } + return clone; } public UnknownFieldSet getDefaultInstanceForType() { return UnknownFieldSet.getDefaultInstance(); } - private void reinitialize() { - fields = Collections.emptyMap(); - lastFieldNumber = 0; - lastField = null; - } - /** Reset the builder to an empty set. */ public Builder clear() { - reinitialize(); + fieldBuilders = new TreeMap(); return this; } @@ -415,11 +400,8 @@ public final class UnknownFieldSet implements MessageLite { } /** Check if the given field number is present in the set. */ - public boolean hasField(final int number) { - if (number == 0) { - throw new IllegalArgumentException("Zero is not a valid field number."); - } - return number == lastFieldNumber || fields.containsKey(number); + public boolean hasField(int number) { + return fieldBuilders.containsKey(number); } /** @@ -430,15 +412,7 @@ public final class UnknownFieldSet implements MessageLite { if (number == 0) { throw new IllegalArgumentException("Zero is not a valid field number."); } - if (lastField != null && lastFieldNumber == number) { - // Discard this. - lastField = null; - lastFieldNumber = 0; - } - if (fields.isEmpty()) { - fields = new TreeMap(); - } - fields.put(number, field); + fieldBuilders.put(number, Field.newBuilder(field)); return this; } @@ -447,7 +421,10 @@ public final class UnknownFieldSet implements MessageLite { * fields are added, the changes may or may not be reflected in this map. */ public Map asMap() { - getFieldBuilder(0); // Force lastField to be built. + TreeMap fields = new TreeMap(); + for (Map.Entry entry : fieldBuilders.entrySet()) { + fields.put(entry.getKey(), entry.getValue().build()); + } return Collections.unmodifiableMap(fields); } @@ -809,54 +786,84 @@ public final class UnknownFieldSet implements MessageLite { *

Use {@link Field#newBuilder()} to construct a {@code Builder}. */ public static final class Builder { - // This constructor should never be called directly (except from 'create'). - private Builder() {} + // This constructor should only be called directly from 'create' and 'clone'. + private Builder() { + result = new Field(); + } private static Builder create() { Builder builder = new Builder(); - builder.result = new Field(); return builder; } private Field result; + @Override + public Builder clone() { + Field copy = new Field(); + if (result.varint == null) { + copy.varint = null; + } else { + copy.varint = new ArrayList(result.varint); + } + if (result.fixed32 == null) { + copy.fixed32 = null; + } else { + copy.fixed32 = new ArrayList(result.fixed32); + } + if (result.fixed64 == null) { + copy.fixed64 = null; + } else { + copy.fixed64 = new ArrayList(result.fixed64); + } + if (result.lengthDelimited == null) { + copy.lengthDelimited = null; + } else { + copy.lengthDelimited = new ArrayList(result.lengthDelimited); + } + if (result.group == null) { + copy.group = null; + } else { + copy.group = new ArrayList(result.group); + } + + Builder clone = new Builder(); + clone.result = copy; + return clone; + } + /** - * Build the field. After {@code build()} has been called, the - * {@code Builder} is no longer usable. Calling any other method will - * result in undefined behavior and can cause a - * {@code NullPointerException} to be thrown. + * Build the field. */ public Field build() { + Field built = new Field(); if (result.varint == null) { - result.varint = Collections.emptyList(); + built.varint = Collections.emptyList(); } else { - result.varint = Collections.unmodifiableList(result.varint); + built.varint = Collections.unmodifiableList(result.varint); } if (result.fixed32 == null) { - result.fixed32 = Collections.emptyList(); + built.fixed32 = Collections.emptyList(); } else { - result.fixed32 = Collections.unmodifiableList(result.fixed32); + built.fixed32 = Collections.unmodifiableList(result.fixed32); } if (result.fixed64 == null) { - result.fixed64 = Collections.emptyList(); + built.fixed64 = Collections.emptyList(); } else { - result.fixed64 = Collections.unmodifiableList(result.fixed64); + built.fixed64 = Collections.unmodifiableList(result.fixed64); } if (result.lengthDelimited == null) { - result.lengthDelimited = Collections.emptyList(); + built.lengthDelimited = Collections.emptyList(); } else { - result.lengthDelimited = + built.lengthDelimited = Collections.unmodifiableList(result.lengthDelimited); } if (result.group == null) { - result.group = Collections.emptyList(); + built.group = Collections.emptyList(); } else { - result.group = Collections.unmodifiableList(result.group); + built.group = Collections.unmodifiableList(result.group); } - - final Field returnMe = result; - result = null; - return returnMe; + return built; } /** Discard the field's contents. */ diff --git a/java/src/test/java/com/google/protobuf/UnknownFieldSetPerformanceTest.java b/java/src/test/java/com/google/protobuf/UnknownFieldSetPerformanceTest.java new file mode 100644 index 000000000..9b913b50f --- /dev/null +++ b/java/src/test/java/com/google/protobuf/UnknownFieldSetPerformanceTest.java @@ -0,0 +1,76 @@ +// Protocol Buffers - Google's data interchange format +// Copyright 2008 Google Inc. All rights reserved. +// https://developers.google.com/protocol-buffers/ +// +// Redistribution and use in source and binary forms, with or without +// modification, are permitted provided that the following conditions are +// met: +// +// * Redistributions of source code must retain the above copyright +// notice, this list of conditions and the following disclaimer. +// * Redistributions in binary form must reproduce the above +// copyright notice, this list of conditions and the following disclaimer +// in the documentation and/or other materials provided with the +// distribution. +// * Neither the name of Google Inc. nor the names of its +// contributors may be used to endorse or promote products derived from +// this software without specific prior written permission. +// +// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + +package com.google.protobuf; + +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.io.InputStream; + +import org.junit.Assert; + +import junit.framework.TestCase; + +public final class UnknownFieldSetPerformanceTest extends TestCase { + + private static byte[] generateBytes(int length) { + Assert.assertTrue((length % 4) == 0); + byte[] input = new byte[length]; + for (int i = 0; i < length; i += 4) { + input[i] = (byte) 0x08; // field 1, wiretype 0 + input[i + 1] = (byte) 0x08; // field 1, payload 8 + input[i + 2] = (byte) 0x20; // field 4, wiretype 0 + input[i + 3] = (byte) 0x20; // field 4, payload 32 + } + return input; + } + + // This is a performance test. Failure here is a timeout. + public void testAlternatingFieldNumbers() throws IOException { + byte[] input = generateBytes(800000); + InputStream in = new ByteArrayInputStream(input); + UnknownFieldSet.Builder builder = UnknownFieldSet.newBuilder(); + CodedInputStream codedInput = CodedInputStream.newInstance(in); + builder.mergeFrom(codedInput); + } + + // This is a performance test. Failure here is a timeout. + public void testAddField() { + UnknownFieldSet.Builder builder = UnknownFieldSet.newBuilder(); + for (int i = 1; i <= 100000; i++) { + UnknownFieldSet.Field field = UnknownFieldSet.Field.newBuilder().addFixed32(i).build(); + builder.addField(i, field); + } + UnknownFieldSet fieldSet = builder.build(); + assertTrue(fieldSet.getField(100000).getFixed32List().get(0) == 100000); + } + +} + -- 2.33.0