Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Escape quotes in string literals #2545

Merged
merged 3 commits into from Mar 12, 2019

Conversation

@rodesai
Copy link
Contributor

commented Mar 11, 2019

Description

This patch changes SqlToJavaVisitor to escape quotes in string literals. This
protects code gen against java code injection attacks. The patch also includes
a query-validation-test that demonstrates how one might inject java code

Testing done

unit test + qvt

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • Ensure relevant issues are linked (description should include text like "Fixes #")
Escape quotes in string literals
This patch changes SqlToJavaVisitor to escape quotes in string literals. This
protects code gen against java code injection attacks. The patch also includes
a query-validation-test that demonstrates how one might inject java code

@rodesai rodesai requested a review from confluentinc/ksql as a code owner Mar 11, 2019

@@ -17,6 +17,7 @@

import io.confluent.ksql.util.KsqlConfig;
import org.apache.kafka.common.serialization.Serde;
import org.apache.kafka.streams.kstream.JoinWindows;

This comment has been minimized.

Copy link
@rodesai

rodesai Mar 11, 2019

Author Contributor

not sure what happened here...

{"topic": "test_topic", "key": 0, "value": "0"}
],
"outputs": [
{"topic": "S1", "key": 0, "value": "\"\"\" + new java.util.function.Supplier<String>(){public String get() {return \"\"boom\"\";}}.get() + \"\"\""}

This comment has been minimized.

Copy link
@rodesai

rodesai Mar 11, 2019

Author Contributor

A malicious user could put arbitrary java code into the expression as long as it returns a string. For example, as in this test, by constructing a supplier (with some malicious behavior in its get() call before returning some string) and getting its value.

@agavra
Copy link
Contributor

left a comment

This is particularly malicious 😈! Thanks for the fix @rodesai

@@ -145,7 +145,9 @@ private String formatExpression(final Expression expression) {
final StringLiteral node,
final Void context
) {
return new Pair<>("\"" + node.getValue() + "\"", Schema.OPTIONAL_STRING_SCHEMA);
return new Pair<>(
"\"" + node.getValue().replace("\"", "\\\"") + "\"",

This comment has been minimized.

Copy link
@agavra

agavra Mar 11, 2019

Contributor

I feel like at some point we need to unescape the values, especially if we are then passing the string literals into UDFs that expect certain semantics. If I have:

SELECT CONCAT(CONCAT('\"', colA), '\"'))

I expect \"colA\" as the result.

This comment has been minimized.

Copy link
@rodesai

rodesai Mar 11, 2019

Author Contributor

I don't think we need to un-escape the values. We need to convert the sql string literal into a java string literal, which should have the quotes escaped. That said, I think your example raises a valid issue - currently this patch will generate an invalid java string literal: "\""

This comment has been minimized.

Copy link
@rodesai

rodesai Mar 11, 2019

Author Contributor

Using StringEscapeUtils addresses this. I've updated accordingly and added your example as a test case.

@@ -145,7 +145,9 @@ private String formatExpression(final Expression expression) {
final StringLiteral node,
final Void context
) {
return new Pair<>("\"" + node.getValue() + "\"", Schema.OPTIONAL_STRING_SCHEMA);
return new Pair<>(
"\"" + node.getValue().replace("\"", "\\\"") + "\"",

This comment has been minimized.

Copy link
@agavra

agavra Mar 11, 2019

Contributor

You might want to look at StringEscapeUtils which handles all the weird escape sequences you might need.

This comment has been minimized.

Copy link
@spena

spena Mar 12, 2019

Member

+1 on StringEscaleUtils. It has a escapeSql which is useful in this kind of situations.

@agavra agavra requested a review from confluentinc/ksql Mar 11, 2019

@agavra

agavra approved these changes Mar 11, 2019

Copy link
Contributor

left a comment

\"LGTM\"!

"SELECT '\\\"' FROM test1;", metaStore);
final String javaExpression = sqlToJavaVisitor
.process(analysis.getSelectExpressions().get(0));
assertThat(javaExpression, equalTo("\"\\\\\\\"\""));

This comment has been minimized.

Copy link
@agavra

agavra Mar 11, 2019

Contributor

🙄 my eyes just hit stack overflow trying to parse the number of \s

@agavra agavra requested a review from confluentinc/ksql Mar 11, 2019

@spena

spena approved these changes Mar 12, 2019

@@ -145,7 +145,9 @@ private String formatExpression(final Expression expression) {
final StringLiteral node,
final Void context
) {
return new Pair<>("\"" + node.getValue() + "\"", Schema.OPTIONAL_STRING_SCHEMA);
return new Pair<>(
"\"" + node.getValue().replace("\"", "\\\"") + "\"",

This comment has been minimized.

Copy link
@spena

spena Mar 12, 2019

Member

+1 on StringEscaleUtils. It has a escapeSql which is useful in this kind of situations.

@rodesai rodesai merged commit 82709eb into confluentinc:master Mar 12, 2019

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.