Apache Avro Java SDK: Code injection on Java generated code

@@ -17,27 +17,7 @@
  */
 package org.apache.avro.compiler.specific;
 
-import java.io.File;
-import java.io.FileOutputStream;
-import java.io.IOException;
-import java.io.OutputStreamWriter;
-import java.io.StringWriter;
-import java.io.Writer;
-import java.lang.reflect.InvocationTargetException;
-import java.nio.file.Files;
-import java.util.ArrayList;
-import java.util.Collection;
-import java.util.Collections;
-import java.util.Comparator;
-import java.util.HashMap;
-import java.util.HashSet;
-import java.util.LinkedHashMap;
-import java.util.LinkedHashSet;
-import java.util.List;
-import java.util.Map;
-import java.util.Set;
-import java.util.stream.Collectors;
-import java.util.stream.Stream;
+import static java.nio.charset.StandardCharsets.UTF_8;
 
 import org.apache.avro.Conversion;
 import org.apache.avro.Conversions;
@@ -60,7 +40,28 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-import static java.nio.charset.StandardCharsets.UTF_8;
+import java.io.File;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.io.OutputStreamWriter;
+import java.io.StringWriter;
+import java.io.Writer;
+import java.lang.reflect.InvocationTargetException;
+import java.nio.file.Files;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.LinkedHashMap;
+import java.util.LinkedHashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.regex.Pattern;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
 
 /**
  * Generate specific Java interfaces and classes for protocols and schemas.
@@ -1004,27 +1005,43 @@ public String conversionInstance(Schema schema) {
    */
   public String[] javaAnnotations(JsonProperties props) {
     final Object value = props.getObjectProp("javaAnnotation");
-    if (value == null)
-      return new String[0];
-    if (value instanceof String)
+    if (value instanceof String && isValidAsAnnotation((String) value))
       return new String[] { value.toString() };
     if (value instanceof List) {
       final List<?> list = (List<?>) value;
       final List<String> annots = new ArrayList<>(list.size());
       for (Object o : list) {
-        annots.add(o.toString());
+        if (isValidAsAnnotation(o.toString()))
+          annots.add(o.toString());
       }
       return annots.toArray(new String[0]);
     }
     return new String[0];
   }
 
+  private static final String PATTERN_IDENTIFIER_PART = "\\p{javaJavaIdentifierStart}\\p{javaJavaIdentifierPart}*";
+  private static final String PATTERN_IDENTIFIER = String.format("(?:%s(?:\\.%s)*)", PATTERN_IDENTIFIER_PART,
+      PATTERN_IDENTIFIER_PART);
+  private static final String PATTERN_STRING = "\"(?:\\\\[\\\\\"ntfb]|(?<!\\\\).)*\"";
+  private static final String PATTERN_NUMBER = "(?:\\((?:byte|char|short|int|long|float|double)\\))?[x0-9_.]*[fl]?";
+  private static final String PATTERN_LITERAL_VALUE = String.format("(?:%s|%s|true|false)", PATTERN_STRING,
+      PATTERN_NUMBER);
+  private static final String PATTERN_PARAMETER_LIST = String.format(
+      "\\(\\s*(?:%s|%s\\s*=\\s*%s(?:\\s*,\\s*%s\\s*=\\s*%s)*)?\\s*\\)", PATTERN_LITERAL_VALUE, PATTERN_IDENTIFIER,
+      PATTERN_LITERAL_VALUE, PATTERN_IDENTIFIER, PATTERN_LITERAL_VALUE);
+  private static final Pattern VALID_AS_ANNOTATION = Pattern
+      .compile(String.format("%s(?:%s)?", PATTERN_IDENTIFIER, PATTERN_PARAMETER_LIST));
+
+  private boolean isValidAsAnnotation(String value) {
+    return VALID_AS_ANNOTATION.matcher(value.strip()).matches();
+  }
+
   // maximum size for string constants, to avoid javac limits
   int maxStringChars = 8192;
 
   /**
    * Utility for template use. Takes a (potentially overly long) string and splits
-   * it into a quoted, comma-separted sequence of escaped strings.
+   * it into a quoted, comma-separated sequence of escaped strings.
    *
    * @param s The string to split
    * @return A sequence of quoted, comma-separated, escaped strings
@@ -1036,7 +1053,7 @@ public String javaSplit(String s) throws IOException {
       if (i != 0)
         b.append("\",\""); // insert quote-comma-quote
       String chunk = s.substring(i, Math.min(s.length(), i + maxStringChars));
-      b.append(javaEscape(chunk)); // escape chunks
+      b.append(escapeForJavaString(chunk)); // escape chunks
     }
     b.append("\""); // final quote
     return b.toString();
@@ -1045,10 +1062,20 @@ public String javaSplit(String s) throws IOException {
   /**
    * Utility for template use. Escapes quotes and backslashes.
    */
-  public static String javaEscape(String o) {
+  public static String escapeForJavaString(String o) {
     return o.replace("\\", "\\\\").replace("\"", "\\\"");
   }
 
+  /**
+   * Utility for template use (previous name). Escapes quotes and backslashes.
+   *
+   * @deprecated Use {@link #escapeForJavaString(String)} instead
+   */
+  @Deprecated(since = "1.12.1", forRemoval = true)
+  public static String javaEscape(String o) {
+    return escapeForJavaString(o);
+  }
+
   /**
    * Utility for template use. Escapes comment end with HTML entities.
    */

@@ -19,7 +19,7 @@
 package $this.mangle($schema.getNamespace());
 #end
 #if ($schema.getDoc())
-/** $schema.getDoc() */
+/** $this.escapeForJavadoc($schema.getDoc()) */
 #end
 #foreach ($annotation in $this.javaAnnotations($schema))
 @$annotation
@@ -28,7 +28,7 @@ package $this.mangle($schema.getNamespace());
 public enum ${this.mangleTypeIdentifier($schema.getName())} implements org.apache.avro.generic.GenericEnumSymbol<${this.mangleTypeIdentifier($schema.getName())}> {
   #foreach ($symbol in ${schema.getEnumSymbols()})${this.mangle($symbol)}#if ($foreach.hasNext), #end#end
   ;
-  public static final org.apache.avro.Schema SCHEMA$ = new org.apache.avro.Schema.Parser().parse("${this.javaEscape($schema.toString())}");
+  public static final org.apache.avro.Schema SCHEMA$ = new org.apache.avro.Schema.Parser().parse("${this.escapeForJavaString($schema.toString())}");
   public static org.apache.avro.Schema getClassSchema() { return SCHEMA$; }
 
   @Override

@@ -19,7 +19,7 @@
 package $this.mangle($schema.getNamespace());
 #end
 #if ($schema.getDoc())
-/** $schema.getDoc() */
+/** $this.escapeForJavadoc($schema.getDoc()) */
 #end
 #foreach ($annotation in $this.javaAnnotations($schema))
 @$annotation
@@ -28,7 +28,7 @@ package $this.mangle($schema.getNamespace());
 @org.apache.avro.specific.AvroGenerated
 public class ${this.mangleTypeIdentifier($schema.getName())} extends org.apache.avro.specific.SpecificFixed {
   private static final long serialVersionUID = ${this.fingerprint64($schema)}L;
-  public static final org.apache.avro.Schema SCHEMA$ = new org.apache.avro.Schema.Parser().parse("${this.javaEscape($schema.toString())}");
+  public static final org.apache.avro.Schema SCHEMA$ = new org.apache.avro.Schema.Parser().parse("${this.escapeForJavaString($schema.toString())}");
   public static org.apache.avro.Schema getClassSchema() { return SCHEMA$; }
   public org.apache.avro.Schema getSchema() { return SCHEMA$; }
 

@@ -20,7 +20,7 @@ package $this.mangle($protocol.getNamespace());
 #end
 
 #if ($protocol.getDoc())
-/** $protocol.getDoc() */
+/** $this.escapeForJavadoc($protocol.getDoc()) */
 #end
 #foreach ($annotation in $this.javaAnnotations($protocol))
 @$annotation
@@ -37,7 +37,7 @@ public interface $this.mangleTypeIdentifier($protocol.getName()) {
    * $this.escapeForJavadoc($message.getDoc())
 #end
 #foreach ($p in $message.getRequest().getFields())##
-#if ($p.doc())   * @param ${this.mangle($p.name())} $p.doc()
+#if ($p.doc())   * @param ${this.mangle($p.name())} $this.escapeForJavadoc($p.doc())
 #end
 #end
    */
@@ -62,7 +62,7 @@ ${this.mangle($error.getFullName())}##
 
 ## Generate nested callback API
 #if ($protocol.getDoc())
-  /** $protocol.getDoc() */
+  /** $this.escapeForJavadoc($protocol.getDoc()) */
 #end
   @org.apache.avro.specific.AvroGenerated
   public interface Callback extends $this.mangleTypeIdentifier($protocol.getName()) {
@@ -78,7 +78,7 @@ ${this.mangle($error.getFullName())}##
      * $this.escapeForJavadoc($message.getDoc())
 #end
 #foreach ($p in $message.getRequest().getFields())##
-#if ($p.doc())     * @param ${this.mangle($p.name())} $p.doc()
+#if ($p.doc())     * @param ${this.mangle($p.name())} $this.escapeForJavadoc($p.doc())
 #end
 #end
      * @throws java.io.IOException The async call could not be completed.

@@ -29,7 +29,7 @@ import org.apache.avro.message.SchemaStore;
 #if (${this.gettersReturnOptional} || ${this.createOptionalGetters})import java.util.Optional;#end
 
 #if ($schema.getDoc())
-/** $schema.getDoc() */
+/** $this.escapeForJavadoc($schema.getDoc()) */
 #end
 #foreach ($annotation in $this.javaAnnotations($schema))
 @$annotation
@@ -116,7 +116,7 @@ public class ${this.mangleTypeIdentifier($schema.getName())} extends ${this.getS
 
 #foreach ($field in $schema.getFields())
 #if ($field.doc())
-  /** $field.doc() */
+  /** $this.escapeForJavadoc($field.doc()) */
 #end
 #foreach ($annotation in $this.javaAnnotations($field))
   @$annotation
@@ -155,7 +155,7 @@ public class ${this.mangleTypeIdentifier($schema.getName())} extends ${this.getS
   /**
    * All-args constructor.
 #foreach ($field in $schema.getFields())
-#if ($field.doc())   * @param ${this.mangle($field.name())} $field.doc()
+#if ($field.doc())   * @param ${this.mangle($field.name())} $this.escapeForJavadoc($field.doc())
 #else   * @param ${this.mangle($field.name())} The new value for ${field.name()}
 #end
 #end
@@ -228,7 +228,7 @@ public class ${this.mangleTypeIdentifier($schema.getName())} extends ${this.getS
 #if (${this.gettersReturnOptional} && (!${this.optionalGettersForNullableFieldsOnly} || ${field.schema().isNullable()}))
   /**
    * Gets the value of the '${this.mangle($field.name(), $schema.isError())}' field as an Optional<${this.escapeForJavadoc(${this.javaType($field.schema())})}>.
-#if ($field.doc())   * $field.doc()
+#if ($field.doc())   * $this.escapeForJavadoc($field.doc())
 #end
    * @return The value wrapped in an Optional<${this.escapeForJavadoc(${this.javaType($field.schema())})}>.
    */
@@ -238,7 +238,7 @@ public class ${this.mangleTypeIdentifier($schema.getName())} extends ${this.getS
 #else
   /**
    * Gets the value of the '${this.mangle($field.name(), $schema.isError())}' field.
-#if ($field.doc())   * @return $field.doc()
+#if ($field.doc())   * @return $this.escapeForJavadoc($field.doc())
 #else   * @return The value of the '${this.mangle($field.name(), $schema.isError())}' field.
 #end
    */
@@ -257,7 +257,7 @@ public class ${this.mangleTypeIdentifier($schema.getName())} extends ${this.getS
 #if (${this.createOptionalGetters})
   /**
    * Gets the value of the '${this.mangle($field.name(), $schema.isError())}' field as an Optional<${this.escapeForJavadoc(${this.javaType($field.schema())})}>.
-#if ($field.doc())   * $field.doc()
+#if ($field.doc())   * $this.escapeForJavadoc($field.doc())
 #end
    * @return The value wrapped in an Optional<${this.escapeForJavadoc(${this.javaType($field.schema())})}>.
    */
@@ -269,7 +269,7 @@ public class ${this.mangleTypeIdentifier($schema.getName())} extends ${this.getS
 #if ($this.createSetters)
   /**
    * Sets the value of the '${this.mangle($field.name(), $schema.isError())}' field.
-#if ($field.doc())   * $field.doc()
+#if ($field.doc())   * $this.escapeForJavadoc($field.doc())
 #end
    * @param value the value to set.
    */
@@ -323,7 +323,7 @@ public class ${this.mangleTypeIdentifier($schema.getName())} extends ${this.getS
 
 #foreach ($field in $schema.getFields())
 #if ($field.doc())
-    /** $field.doc() */
+    /** $this.escapeForJavadoc($field.doc()) */
 #end
     private ${this.javaUnbox($field.schema(), false)} ${this.mangle($field.name(), $schema.isError())};
 #if (${this.hasBuilder($field.schema())})
@@ -402,7 +402,7 @@ public class ${this.mangleTypeIdentifier($schema.getName())} extends ${this.getS
 #foreach ($field in $schema.getFields())
     /**
       * Gets the value of the '${this.mangle($field.name(), $schema.isError())}' field.
-#if ($field.doc())      * $field.doc()
+#if ($field.doc())      * $this.escapeForJavadoc($field.doc())
 #end
       * @return The value.
       */
@@ -413,7 +413,7 @@ public class ${this.mangleTypeIdentifier($schema.getName())} extends ${this.getS
 #if (${this.createOptionalGetters})
     /**
       * Gets the value of the '${this.mangle($field.name(), $schema.isError())}' field as an Optional<${this.escapeForJavadoc(${this.javaType($field.schema())})}>.
-#if ($field.doc())      * $field.doc()
+#if ($field.doc())      * $this.escapeForJavadoc($field.doc())
 #end
       * @return The value wrapped in an Optional<${this.escapeForJavadoc(${this.javaType($field.schema())})}>.
       */
@@ -424,7 +424,7 @@ public class ${this.mangleTypeIdentifier($schema.getName())} extends ${this.getS
 
     /**
       * Sets the value of the '${this.mangle($field.name(), $schema.isError())}' field.
-#if ($field.doc())      * $field.doc()
+#if ($field.doc())      * $this.escapeForJavadoc($field.doc())
 #end
       * @param value The value of '${this.mangle($field.name(), $schema.isError())}'.
       * @return This builder.
@@ -441,7 +441,7 @@ public class ${this.mangleTypeIdentifier($schema.getName())} extends ${this.getS
 
     /**
       * Checks whether the '${this.mangle($field.name(), $schema.isError())}' field has been set.
-#if ($field.doc())      * $field.doc()
+#if ($field.doc())      * $this.escapeForJavadoc($field.doc())
 #end
       * @return True if the '${this.mangle($field.name(), $schema.isError())}' field has been set, false otherwise.
       */
@@ -452,7 +452,7 @@ public class ${this.mangleTypeIdentifier($schema.getName())} extends ${this.getS
 #if (${this.hasBuilder($field.schema())})
     /**
      * Gets the Builder instance for the '${this.mangle($field.name(), $schema.isError())}' field and creates one if it doesn't exist yet.
-#if ($field.doc())     * $field.doc()
+#if ($field.doc())     * $this.escapeForJavadoc($field.doc())
 #end
      * @return This builder.
      */
@@ -469,7 +469,7 @@ public class ${this.mangleTypeIdentifier($schema.getName())} extends ${this.getS
 
     /**
      * Sets the Builder instance for the '${this.mangle($field.name(), $schema.isError())}' field
-#if ($field.doc())     * $field.doc()
+#if ($field.doc())     * $this.escapeForJavadoc($field.doc())
 #end
      * @param value The builder instance that must be set.
      * @return This builder.
@@ -483,7 +483,7 @@ public class ${this.mangleTypeIdentifier($schema.getName())} extends ${this.getS
 
     /**
      * Checks whether the '${this.mangle($field.name(), $schema.isError())}' field has an active Builder instance
-#if ($field.doc())     * $field.doc()
+#if ($field.doc())     * $this.escapeForJavadoc($field.doc())
 #end
      * @return True if the '${this.mangle($field.name(), $schema.isError())}' field has an active Builder instance
      */
@@ -494,7 +494,7 @@ public class ${this.mangleTypeIdentifier($schema.getName())} extends ${this.getS
 
     /**
       * Clears the value of the '${this.mangle($field.name(), $schema.isError())}' field.
-#if ($field.doc())      * $field.doc()
+#if ($field.doc())      * $this.escapeForJavadoc($field.doc())
 #end
       * @return This builder.
       */

@@ -26,6 +26,20 @@
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
+import org.apache.avro.AvroTypeException;
+import org.apache.avro.LogicalType;
+import org.apache.avro.LogicalTypes;
+import org.apache.avro.Protocol;
+import org.apache.avro.Schema;
+import org.apache.avro.SchemaBuilder;
+import org.apache.avro.generic.GenericData.StringType;
+import org.apache.avro.specific.SpecificData;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
 import java.io.BufferedReader;
 import java.io.File;
 import java.io.FileInputStream;
@@ -37,29 +51,17 @@
 import java.util.Collections;
 import java.util.HashSet;
 import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Set;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
 import javax.tools.Diagnostic;
 import javax.tools.DiagnosticListener;
 import javax.tools.JavaCompiler;
 import javax.tools.JavaFileObject;
 import javax.tools.StandardJavaFileManager;
 import javax.tools.ToolProvider;
-import org.apache.avro.AvroTypeException;
-
-import java.util.Locale;
-import java.util.Map;
-import java.util.Set;
-
-import org.apache.avro.LogicalType;
-import org.apache.avro.LogicalTypes;
-import org.apache.avro.Schema;
-import org.apache.avro.SchemaBuilder;
-import org.apache.avro.generic.GenericData.StringType;
-import org.apache.avro.specific.SpecificData;
-import org.junit.jupiter.api.BeforeEach;
-import org.junit.jupiter.api.Test;
-import org.junit.jupiter.api.io.TempDir;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
 
 public class TestSpecificCompiler {
   private static final Logger LOG = LoggerFactory.getLogger(TestSpecificCompiler.class);
@@ -990,4 +992,49 @@ void fieldWithUnderscore_avro3826() {
     assertFalse(outputFile4.contents.contains("$3"));
   }
 
+  @Test
+  void docsAreEscaped_avro4053() {
+    String jsonSchema = "{\n" + "  \"protocol\": \"DummyProtocol\",\n"
+        + "  \"doc\": \"*/\\nTest escaping <threats>\\n/*\",\n" + "  \"types\" : [\n"
+        + "    {\"type\": \"fixed\", \"name\": \"Hash\", \"size\": 16, \"doc\": \"*/\\nTest escaping <threats>\\n/*\""
+        + "},\n"
+        + "    {\"type\": \"enum\", \"name\": \"Status\", \"symbols\": [\"ON\", \"OFF\"], \"doc\": \"*/\\nTest escaping <threats>\\n/*\"},\n"
+        + "   " + " {\"type\": \"record\", \"name\": \"Message\", \"fields\" : [\n"
+        + "      {\"name\": \"content\", \"type\": \"string\", \"doc\":  \"*/\\nTest escaping <threats>\\n/*\"},\n"
+        + "      {\"name\": \"status\", \"type\": \"Status\", \"doc\":  \"*/\\nTest escaping <threats>\\n/*\"},\n"
+        + "      {\"name\": \"signature\", \"type\": \"Hash\", \"doc\":  \"*/\\nTest escaping <threats>\\n/*\"}\n"
+        + "    ]}\n" + "  ],\n" + "  \"messages\" : {\n" + "    \"echo\": {\"request\": ["
+        + "{\"name\": \"msg\", \"type\": \"Message\"}"
+        + "], \"response\": \"Message\", \"doc\": \"*/\\nTest escaping <threats>\\n/*\"}\n" + "  },\n"
+        + "  \"javaAnnotation\": [\n" + "    \"Deprecated(forRemoval = true, since = \\\"forever\\\")\",\n"
+        + "    \"SuppressWarnings(\\\"ALL\\\")\",\n" + "    \"SuppressWarnings(\\\"CodeInjection\\\")/*\",\n"
+        + "    \" This is inside a comment as each line is prefixed with @\",\n"
+        + "    \" and the next bit is really dangerous... */ static { System.exit(); }\"\n" + "  ]\n" + "}";
+    Collection<SpecificCompiler.OutputFile> outputs = new SpecificCompiler(Protocol.parse(jsonSchema)).compile();
+    for (SpecificCompiler.OutputFile outputFile : outputs) {
+      assertFalse(outputFile.contents.contains("*/\\nTest escaping <threats>\\n/*"), "Threats present?");
+
+      int expectedEscapeCount = countOccurrences(Pattern.compile("Test escaping", Pattern.LITERAL),
+          outputFile.contents);
+      int escapedJavaDocCount = countOccurrences(Pattern.compile("\\*&#47;\\s*Test escaping &lt;threats&gt;\\s*/\\*"),
+          outputFile.contents);
+      // noinspection RegExpRedundantEscape
+      int escapedDocStringCount = countOccurrences(
+          Pattern.compile("\\\"doc\\\":\\\"*/\\\\nTest escaping <threats>\\\\n/*\\\"", Pattern.LITERAL),
+          outputFile.contents);
+      assertEquals(expectedEscapeCount, escapedJavaDocCount + escapedDocStringCount,
+          "Escaped threats in " + outputFile.path);
+
+      assertFalse(Pattern.compile("\\{ System.exit\\(\\); }(?!\\\\\")").matcher(outputFile.contents).find(),
+          "Code injection present? " + outputFile.contents);
+    }
+  }
+
+  private int countOccurrences(Pattern pattern, String textToSearch) {
+    int count = 0;
+    for (Matcher matcher = pattern.matcher(textToSearch); matcher.find();) {
+      count++;
+    }
+    return count;
+  }
 }

@@ -1,8 +1,9 @@
 {
-  "type": "record", 
+  "type": "record",
   "name": "SimpleRecord",
+  "doc": ",*/\n hoping the compiler won't barf on strange comments\n/*",
   "fields" : [
     {"name": "value", "type": "int"},
     {"name": "nullableValue", "type": ["null","int"], "doc" : "doc"}
   ]
-}
\ No newline at end of file
+}

@@ -70,7 +70,7 @@ public class TestSpecificCompiler {
 
   @Test
   void esc() {
-    assertEquals("\\\"", SpecificCompiler.javaEscape("\""));
+    assertEquals("\\\"", SpecificCompiler.escapeForJavaString("\""));
   }
 
   @Test