From b7823ba8467783d9f0b728de6903b462ba0f9fca Mon Sep 17 00:00:00 2001
From: Sokwhan Huh <sokwhan@google.com>
Date: Wed, 24 Jan 2024 10:18:58 -0800
Subject: [PATCH] Prevent string.format injection when interpreter exception is
 being built

PiperOrigin-RevId: 601159713
---
 .../test/java/dev/cel/bundle/CelImplTest.java    |  9 +++++++++
 .../dev/cel/runtime/InterpreterException.java    | 16 ++++++++++++++--
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/bundle/src/test/java/dev/cel/bundle/CelImplTest.java b/bundle/src/test/java/dev/cel/bundle/CelImplTest.java
index 4cb3a75e..f844246c 100644
--- a/bundle/src/test/java/dev/cel/bundle/CelImplTest.java
+++ b/bundle/src/test/java/dev/cel/bundle/CelImplTest.java
@@ -1045,6 +1045,15 @@ public void program_wrongTypeComprehensionThrows() throws Exception {
     assertThat(e).hasMessageThat().contains("expected a list or a map");
   }
 
+  @Test
+  public void program_stringFormatInjection_throwsEvaluationException() throws Exception {
+    Cel cel = standardCelBuilderWithMacros().build();
+    CelRuntime.Program program = cel.createProgram(cel.compile("{}['%2000222222s']").getAst());
+
+    CelEvaluationException e = assertThrows(CelEvaluationException.class, program::eval);
+    assertThat(e).hasMessageThat().contains("evaluation error");
+  }
+
   @Test
   public void program_emptyTypeProviderConfig() throws Exception {
     Cel cel = standardCelBuilderWithMacros().build();
diff --git a/runtime/src/main/java/dev/cel/runtime/InterpreterException.java b/runtime/src/main/java/dev/cel/runtime/InterpreterException.java
index 04b0968d..2802726a 100644
--- a/runtime/src/main/java/dev/cel/runtime/InterpreterException.java
+++ b/runtime/src/main/java/dev/cel/runtime/InterpreterException.java
@@ -16,6 +16,7 @@
 
 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;
@@ -31,6 +32,8 @@
  */
 @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() {
@@ -47,7 +50,7 @@ public static class Builder {
 
     @SuppressWarnings({"AnnotateFormatMethod"}) // Format strings are optional.
     public Builder(String message, Object... args) {
-      this.message = args.length > 0 ? String.format(message, args) : message;
+      this.message = safeFormat(message, args);
     }
 
     @SuppressWarnings({"AnnotateFormatMethod"}) // Format strings are optional.
@@ -64,7 +67,7 @@ public Builder(RuntimeException e, String message, Object... args) {
         this.cause = e;
       }
 
-      this.message = args.length > 0 ? String.format(message, args) : message;
+      this.message = safeFormat(message, args);
     }
 
     @CanIgnoreReturnValue
@@ -97,6 +100,15 @@ 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) {