iemejia commented on code in PR #3892:
URL: https://github.com/apache/avro/pull/3892#discussion_r3616456736
##########
lang/java/compiler/src/main/java/org/apache/avro/compiler/specific/SpecificCompiler.java:
##########
@@ -1054,7 +1054,11 @@ public String[] javaAnnotations(JsonProperties props) {
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]|(?<!\\\\).)*\"";
+ // A string literal is a quote, a body of escape sequences or characters that
+ // are not a quote, backslash or line terminator, and a closing quote. The
body
+ // must not be able to contain an unescaped quote, otherwise a single literal
+ // could span past the intended closing quote and swallow surrounding tokens.
+ private static final String PATTERN_STRING =
"\"(?:\\\\[\\\\\"ntfb]|[^\"\\\\\\r\\n])*\"";
Review Comment:
Good catch — fixed in 2a0f2ad. The character class now also excludes NEL
(`\x85`), LS (`\x{2028}`) and PS (`\x{2029}`) in addition to CR and LF, so no
line terminator can appear unescaped inside the string literal. Verified that
annotation values containing these characters are now rejected while legitimate
annotations still validate.
##########
lang/java/compiler/src/test/java/org/apache/avro/compiler/specific/TestSpecificCompiler.java:
##########
@@ -1031,6 +1031,31 @@ void docsAreEscaped_avro4053() {
}
}
+ @Test
+ void annotationCannotBreakOutViaStringLiteral() {
+ // A crafted javaAnnotation value tries to terminate the first annotation,
+ // inject arbitrary declarations plus a static initializer, then reopen a
+ // second valid annotation. It relies on a string literal spanning past its
+ // intended closing quote. Such values must be rejected, not emitted
verbatim.
+ String jsonSchema = "{\n" + " \"type\": \"record\",\n" + " \"name\":
\"Injected\",\n"
+ + " \"javaAnnotation\": [\n"
+ + " \"java.lang.SuppressWarnings(\\\"x\\\") static {
System.exit(1); } @java.lang.SuppressWarnings(\\\"y\\\")\",\n"
+ + " \"SuppressWarnings(\\\"unchecked\\\")\"\n" + " ],\n" + "
\"fields\": [\n"
+ + " {\"name\": \"value\", \"type\": \"string\"}\n" + " ]\n" + "}";
+ Collection<SpecificCompiler.OutputFile> outputs = new
SpecificCompiler(SchemaParser.parseSingle(jsonSchema))
+ .compile();
+ for (SpecificCompiler.OutputFile outputFile : outputs) {
+ // The payload is echoed (safely escaped) inside the SCHEMA$ string
constant,
+ // so we must distinguish that from a verbatim emission as code. Real
injected
+ // code would carry unescaped quotes; the schema literal escapes them as
\".
+ assertFalse(outputFile.contents.contains("SuppressWarnings(\"x\") static
{ System.exit(1); }"),
+ "Code injection present? " + outputFile.contents);
+ // The legitimate annotation in the same list must still be emitted.
+
assertTrue(outputFile.contents.contains("@SuppressWarnings(\"unchecked\")"),
+ "Valid annotation missing? " + outputFile.contents);
+ }
Review Comment:
Addressed in 90299cc. The loop now asserts the injection payload is absent
from *every* generated file, and separately asserts the valid annotation is
emitted in *at least one* output (via an accumulator checked after the loop),
so the test no longer depends on the number of output files.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
