Skip to content

Commit

Permalink
Prevent string.format injection when interpreter exception is being b…
Browse files Browse the repository at this point in the history
…uilt

PiperOrigin-RevId: 601159713
  • Loading branch information
l46kok authored and copybara-github committed Jan 24, 2024
1 parent 7d28e89 commit b7823ba
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 2 deletions.
9 changes: 9 additions & 0 deletions bundle/src/test/java/dev/cel/bundle/CelImplTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
16 changes: 14 additions & 2 deletions runtime/src/main/java/dev/cel/runtime/InterpreterException.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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() {
Expand All @@ -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.
Expand All @@ -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
Expand Down Expand Up @@ -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) {
Expand Down

0 comments on commit b7823ba

Please sign in to comment.