From 948fd365064648052aecf861483d82be259d0e06 Mon Sep 17 00:00:00 2001 From: Sokwhan Huh Date: Thu, 25 Jan 2024 16:58:23 -0800 Subject: [PATCH] Add SafeStringFormatter. Address fuzzing issue around CelValidationException. PiperOrigin-RevId: 601609078 --- common/internal/BUILD.bazel | 5 ++ .../src/main/java/dev/cel/common/BUILD.bazel | 1 + .../main/java/dev/cel/common/CelIssue.java | 3 +- .../cel/common/CelValidationException.java | 22 ++++++++- .../java/dev/cel/common/internal/BUILD.bazel | 11 +++++ .../common/internal/SafeStringFormatter.java | 48 +++++++++++++++++++ .../src/test/java/dev/cel/common/BUILD.bazel | 1 + .../common/CelValidationExceptionTest.java | 42 ++++++++++++++++ .../src/main/java/dev/cel/runtime/BUILD.bazel | 1 + .../dev/cel/runtime/InterpreterException.java | 17 ++----- 10 files changed, 134 insertions(+), 17 deletions(-) create mode 100644 common/src/main/java/dev/cel/common/internal/SafeStringFormatter.java create mode 100644 common/src/test/java/dev/cel/common/CelValidationExceptionTest.java diff --git a/common/internal/BUILD.bazel b/common/internal/BUILD.bazel index 95b36768..6293804f 100644 --- a/common/internal/BUILD.bazel +++ b/common/internal/BUILD.bazel @@ -67,3 +67,8 @@ java_library( name = "cel_descriptor_pools", exports = ["//common/src/main/java/dev/cel/common/internal:cel_descriptor_pools"], ) + +java_library( + name = "safe_string_formatter", + exports = ["//common/src/main/java/dev/cel/common/internal:safe_string_formatter"], +) diff --git a/common/src/main/java/dev/cel/common/BUILD.bazel b/common/src/main/java/dev/cel/common/BUILD.bazel index dd39523f..31b59370 100644 --- a/common/src/main/java/dev/cel/common/BUILD.bazel +++ b/common/src/main/java/dev/cel/common/BUILD.bazel @@ -64,6 +64,7 @@ java_library( ":common", "//:auto_value", "//common/annotations", + "//common/internal:safe_string_formatter", "//common/types:cel_types", "//common/types:type_providers", "@cel_spec//proto/cel/expr:expr_java_proto", diff --git a/common/src/main/java/dev/cel/common/CelIssue.java b/common/src/main/java/dev/cel/common/CelIssue.java index 4ad48cea..7f7417a5 100644 --- a/common/src/main/java/dev/cel/common/CelIssue.java +++ b/common/src/main/java/dev/cel/common/CelIssue.java @@ -17,6 +17,7 @@ import com.google.auto.value.AutoValue; import com.google.errorprone.annotations.CheckReturnValue; import com.google.errorprone.annotations.Immutable; +import dev.cel.common.internal.SafeStringFormatter; import java.util.Optional; import java.util.PrimitiveIterator; @@ -76,7 +77,7 @@ public static CelIssue formatError(int line, int column, String message) { public String toDisplayString(CelSource source) { // Based onhttps://github.com/google/cel-go/blob/v0.5.1/common/error.go#L42. String result = - String.format( + SafeStringFormatter.format( "%s: %s:%d:%d: %s", getSeverity(), source.getDescription(), diff --git a/common/src/main/java/dev/cel/common/CelValidationException.java b/common/src/main/java/dev/cel/common/CelValidationException.java index 84f5bcbf..ef136f8a 100644 --- a/common/src/main/java/dev/cel/common/CelValidationException.java +++ b/common/src/main/java/dev/cel/common/CelValidationException.java @@ -18,18 +18,21 @@ import com.google.common.base.Joiner; import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; +import java.util.List; /** Base class for all checked exceptions explicitly thrown by the library during parsing. */ public final class CelValidationException extends CelException { private static final Joiner JOINER = Joiner.on('\n'); + // Truncates all errors beyond this limit in the message. + private static final int MAX_ERRORS_TO_REPORT = 1000; private final CelSource source; private final ImmutableList errors; @VisibleForTesting - public CelValidationException(CelSource source, Iterable errors) { - super(JOINER.join(Iterables.transform(errors, error -> error.toDisplayString(source)))); + public CelValidationException(CelSource source, List errors) { + super(safeJoinErrorMessage(source, errors)); this.source = source; this.errors = ImmutableList.copyOf(errors); } @@ -41,6 +44,21 @@ public CelValidationException(CelSource source, Iterable errors) { this.errors = ImmutableList.copyOf(errors); } + private static String safeJoinErrorMessage(CelSource source, List errors) { + if (errors.size() <= MAX_ERRORS_TO_REPORT) { + return JOINER.join(Iterables.transform(errors, error -> error.toDisplayString(source))); + } + + List truncatedErrors = errors.subList(0, MAX_ERRORS_TO_REPORT); + StringBuilder sb = new StringBuilder(); + JOINER.appendTo( + sb, Iterables.transform(truncatedErrors, error -> error.toDisplayString(source))); + sb.append( + String.format("%n...and %d more errors (truncated)", errors.size() - MAX_ERRORS_TO_REPORT)); + + return sb.toString(); + } + /** Returns the {@link CelSource} that was being validated. */ public CelSource getSource() { return source; diff --git a/common/src/main/java/dev/cel/common/internal/BUILD.bazel b/common/src/main/java/dev/cel/common/internal/BUILD.bazel index 88d7dd51..e373f442 100644 --- a/common/src/main/java/dev/cel/common/internal/BUILD.bazel +++ b/common/src/main/java/dev/cel/common/internal/BUILD.bazel @@ -227,3 +227,14 @@ java_library( "@maven//:com_google_protobuf_protobuf_java", ], ) + +java_library( + name = "safe_string_formatter", + srcs = ["SafeStringFormatter.java"], + tags = [ + ], + deps = [ + "//common/annotations", + "@maven//:com_google_re2j_re2j", + ], +) diff --git a/common/src/main/java/dev/cel/common/internal/SafeStringFormatter.java b/common/src/main/java/dev/cel/common/internal/SafeStringFormatter.java new file mode 100644 index 00000000..7c22e834 --- /dev/null +++ b/common/src/main/java/dev/cel/common/internal/SafeStringFormatter.java @@ -0,0 +1,48 @@ +// Copyright 2024 Google LLC +// +// Licensed 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 dev.cel.common.internal; + +import com.google.re2j.Pattern; +import dev.cel.common.annotations.Internal; + +/** + * {@link SafeStringFormatter} is a wrapper around JDK's {@link String#format}. It prevents any + * unsafe string.format calls by only allowing known formatting specifiers to be provided. + * + *

CEL Library Internals. Do Not Use. + */ +@Internal +public final class SafeStringFormatter { + // Allow format specifiers of %d, %f, %s and %n only. + private static final Pattern FORBIDDEN_FORMAT_SPECIFIERS = Pattern.compile("%[^dfsn]"); + + /** + * Performs a safe {@link String#format}. + * + * @param format A format string. Only %d, %f, %s and %n are allowed as formatting specifiers. All + * other formatting specifiers will be stripped out. + * @return A formatted string + */ + public static String format(String format, Object... args) { + if (args.length == 0) { + return format; + } + + String sanitizedMessage = FORBIDDEN_FORMAT_SPECIFIERS.matcher(format).replaceAll(""); + return String.format(sanitizedMessage, args); + } + + private SafeStringFormatter() {} +} diff --git a/common/src/test/java/dev/cel/common/BUILD.bazel b/common/src/test/java/dev/cel/common/BUILD.bazel index 6001ed1b..d22f00e7 100644 --- a/common/src/test/java/dev/cel/common/BUILD.bazel +++ b/common/src/test/java/dev/cel/common/BUILD.bazel @@ -11,6 +11,7 @@ java_library( deps = [ "//:java_truth", "//common", + "//common:compiler_common", "//common:features", "//common:options", "//common:proto_v1alpha1_ast", diff --git a/common/src/test/java/dev/cel/common/CelValidationExceptionTest.java b/common/src/test/java/dev/cel/common/CelValidationExceptionTest.java new file mode 100644 index 00000000..be428a64 --- /dev/null +++ b/common/src/test/java/dev/cel/common/CelValidationExceptionTest.java @@ -0,0 +1,42 @@ +// Copyright 2024 Google LLC +// +// Licensed 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 dev.cel.common; + +import static com.google.common.truth.Truth.assertThat; + +import com.google.common.collect.ImmutableList; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public class CelValidationExceptionTest { + + @Test + public void construct_withLargeErrorCount() { + ImmutableList.Builder issueBuilder = ImmutableList.builder(); + for (int i = 0; i < 1500; i++) { + issueBuilder.add(CelIssue.formatError(i + 1, i + 1, "generic error")); + } + + CelValidationException celValidationException = + new CelValidationException(CelSource.newBuilder().build(), issueBuilder.build()); + + assertThat(celValidationException.getErrors()).hasSize(1500); + assertThat(celValidationException) + .hasMessageThat() + .endsWith("...and 500 more errors (truncated)"); + } +} diff --git a/runtime/src/main/java/dev/cel/runtime/BUILD.bazel b/runtime/src/main/java/dev/cel/runtime/BUILD.bazel index d713b511..f9d3dc6a 100644 --- a/runtime/src/main/java/dev/cel/runtime/BUILD.bazel +++ b/runtime/src/main/java/dev/cel/runtime/BUILD.bazel @@ -54,6 +54,7 @@ java_library( "//common/internal:comparison_functions", "//common/internal:default_message_factory", "//common/internal:dynamic_proto", + "//common/internal:safe_string_formatter", "//common/types", "//common/types:type_providers", "@cel_spec//proto/cel/expr:expr_java_proto", diff --git a/runtime/src/main/java/dev/cel/runtime/InterpreterException.java b/runtime/src/main/java/dev/cel/runtime/InterpreterException.java index 2802726a..98595ee8 100644 --- a/runtime/src/main/java/dev/cel/runtime/InterpreterException.java +++ b/runtime/src/main/java/dev/cel/runtime/InterpreterException.java @@ -16,10 +16,10 @@ import com.google.errorprone.annotations.CanIgnoreReturnValue; import com.google.errorprone.annotations.CheckReturnValue; -import com.google.re2j.Pattern; import dev.cel.common.CelErrorCode; import dev.cel.common.CelRuntimeException; import dev.cel.common.annotations.Internal; +import dev.cel.common.internal.SafeStringFormatter; import org.jspecify.nullness.Nullable; /** @@ -32,8 +32,6 @@ */ @Internal public class InterpreterException extends Exception { - // Allow format specifiers of %d, %f, %s and %n only. - private static final Pattern ALLOWED_FORMAT_SPECIFIERS = Pattern.compile("%[^dfsn]"); private final CelErrorCode errorCode; public CelErrorCode getErrorCode() { @@ -50,7 +48,7 @@ public static class Builder { @SuppressWarnings({"AnnotateFormatMethod"}) // Format strings are optional. public Builder(String message, Object... args) { - this.message = safeFormat(message, args); + this.message = SafeStringFormatter.format(message, args); } @SuppressWarnings({"AnnotateFormatMethod"}) // Format strings are optional. @@ -67,7 +65,7 @@ public Builder(RuntimeException e, String message, Object... args) { this.cause = e; } - this.message = safeFormat(message, args); + this.message = SafeStringFormatter.format(message, args); } @CanIgnoreReturnValue @@ -100,15 +98,6 @@ public InterpreterException build() { cause, errorCode); } - - private static String safeFormat(String message, Object[] args) { - if (args.length == 0) { - return message; - } - - String sanitizedMessage = ALLOWED_FORMAT_SPECIFIERS.matcher(message).replaceAll(""); - return String.format(sanitizedMessage, args); - } } private InterpreterException(String message, Throwable cause, CelErrorCode errorCode) {