You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by rick-ibm <gi...@git.apache.org> on 2015/10/21 21:10:46 UTC
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
GitHub user rick-ibm opened a pull request:
https://github.com/apache/spark/pull/9202
[SPARK-10857] [SQL] Block SQL injection vulnerabilities under DataFrameWriter.jdbc().
@marmbrus
@rxin
@joshrosen
@srowen
This patch attempts to address both https://issues.apache.org/jira/browse/SPARK-10857 and https://issues.apache.org/jira/browse/SPARK-10977. This patch is my original work which I license to the ASF. My ICLA is already on file as a Derby committer: http://people.apache.org/committer-index.html
I have abandoned the approach discussed on those issues, viz., attempting to replace the user-supplied table name with a properly escaped and quoted chain of dot-separated delimited identifiers. I believe that JDBC does not supply enough information to attempt that substitution, particularly in the case of a Microsoft SQL Server running on a case-insensitive file system. An overview of the issues involved can be found here: https://github.com/ontop/ontop/wiki/Case-sensitivity-for-SQL-identifiers
Instead, this patch simply verifies that the table name is a valid SQL identifier before inserting it into a CREATE/DROP/INSERT statement or into a statement which tests whether the table exists. That is, a SQL injection attempt will fail because the injected text does not parse as a valid SQL identifier.
The validating code may not catch some bad names. This can happen for databases which don't allow embedded, escaped quotes inside quoted identifiers. Microsoft SQL Server and Oracle don't allow embedded quotes. For those databases, the bad table name will appear as an ungrammatical sequence of quoted identifiers. E.g.:
"foo""bar"
will appear as
"foo" "bar"
Instead of a "bad identifier" error, those databases will raise some other exception. I leave further polishing of this behavior to people who are experts on the affected databases.
This patch makes the following changes:
1) Introduces a new Java class, SqlIdentifierUtil. The code was cribbed from Derby, where it has undergone almost 2 decades of commodity testing in production applications. I thought that it was safer to re-use this code rather than translate it into Scala and run the risk of introducing translation errors. However, I can try my hand at translating this class into Scala if reviewers think that that is a better approach.
The SqlIdentifierUtil class has 2 entry points:
a) quoteString() - This method escapes embedded quotes when converting a string to a quoted identifier. JdbcDialect.quoteIdentifier() did not account for this subtlety. But the patch makes JdbcDialect.quoteIdentifier() call SqlIdentifier.quoteString() now.
b) parseMultiPartSQLIdentifier() - This method takes a dot separated sequence of SQL identifiers like databaseName.schemaName.tableName and returns an array of normalized identifiers like [ DATABASENAME, SCHEMANAME, TABLENAME ].
2) Introduces some new JdbcDialect methods:
a) vetSqlIdentifier() - This method takes a table name and raises an exception if the name isn't a legal SQL identifier as determined by SqlIdentifierUtil.parseMultiPartSQLIdentifier().
b) quoteChar() - This method returns the dialect-specific character which is used to frame quoted identifiers. This information is needed by SqlIdentifierUtil.parseMultiPartSQLIdentifier().
3) Calls JdbcDialect.vetSqlIdentifier() everywhere that we plug a table name into a SQL statement. I have placed the calls to JdbcDialect.vetSqlIdentifier() as close to the statement text as possible in order to minimize the chance that a new code path will be introduced in the future which circumvents the SQL injection check.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/rick-ibm/spark b_10857_sqlInjection
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/9202.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #9202
----
commit 4e167a4e9ec8385063c4f46b6b75ae369d2e6204
Author: Rick Hillegas <rh...@us.ibm.com>
Date: 2015-10-19T20:00:32Z
SPARK-10857: Vet table names which are inserted into SQL strings to verify that they are legal SQL identifiers in the underlying database.
commit 4d9d8859294f83bff172e7e1146a2024ac3b61ec
Author: Rick Hillegas <rh...@us.ibm.com>
Date: 2015-10-21T18:46:26Z
SPARK-10857: Add and remove some comments.
commit a5c4c698705c5ed7417b405e6a0518bd7b997336
Author: Rick Hillegas <rh...@us.ibm.com>
Date: 2015-10-21T18:46:56Z
SPARK-10857: Add and remove some comments.
commit b3b845de9960e003637d26fecaf8dccaa0206f59
Author: Rick Hillegas <rh...@us.ibm.com>
Date: 2015-10-21T18:50:56Z
DERBY-10857: Fix a scalastyle problem.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/9202#issuecomment-216600951
Can one of the admins verify this patch?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/9202#issuecomment-150304899
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/44156/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/9202#issuecomment-150026844
Merged build started.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/9202#issuecomment-151610388
**[Test build #44457 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/44457/consoleFull)** for PR 9202 at commit [`7d90459`](https://github.com/apache/spark/commit/7d90459fca34e3ff1cbc1ebf62c717aebb052307).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/9202#issuecomment-150269103
Merged build started.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #9202: [SPARK-10857] [SQL] Block SQL injection vulnerabil...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/9202
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/9202#issuecomment-150269081
Merged build triggered.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/9202#issuecomment-150304893
Merged build finished. Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/9202#issuecomment-150026822
Merged build triggered.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/9202#issuecomment-150043989
Merged build triggered.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/9202#issuecomment-151642087
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/44457/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/9202#issuecomment-153806475
Merged build triggered.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/9202#issuecomment-153842920
Merged build finished. Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/9202#issuecomment-150304731
**[Test build #44156 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/44156/consoleFull)** for PR 9202 at commit [`79514f3`](https://github.com/apache/spark/commit/79514f3b538502aa3d9473cba7ae0cade73885f3).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds the following public classes _(experimental)_:\n * `public class SqlIdentifierUtil `\n
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/9202#issuecomment-150270090
**[Test build #44156 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/44156/consoleFull)** for PR 9202 at commit [`79514f3`](https://github.com/apache/spark/commit/79514f3b538502aa3d9473cba7ae0cade73885f3).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/9202#issuecomment-150020934
Merged build finished. Test FAILed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/9202#issuecomment-153842921
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/45027/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/9202#issuecomment-149997285
Can one of the admins verify this patch?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/9202#issuecomment-150064704
**[Test build #44096 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/44096/consoleFull)** for PR 9202 at commit [`e7e401f`](https://github.com/apache/spark/commit/e7e401f810af28a84b5a40300e6f5ea3f3d96fe4).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds the following public classes _(experimental)_:\n * `public class SqlIdentifierUtil `\n
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on the pull request:
https://github.com/apache/spark/pull/9202#issuecomment-152588761
@rick-ibm, thanks for working on this. With the Spark Summit just wrapping up and code freeze for 1.6 tomorrow I don't think there is going to be a lot of review bandwidth for a patch this large until after the release. While it would be nice to secure things, this doesn't seem like a particularly high priority vulnerability since any user that is creating a dataframe by querying JDBC must have the credentials already and could just open their own connection.
I haven't looked closely at the implementation, but one high level question is whether this is breaking the use case where a users gives a subquery instead of a table name (i.e. `dbtable = (SELECT ...)`.) This is an important part of the API that we can't break.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/9202#discussion_r42854130
--- Diff: sql/catalyst/src/main/java/org/apache/spark/sql/SqlIdentifierUtil.java ---
@@ -0,0 +1,322 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql;
+
+import java.io.IOException;
+import java.io.StringReader;
+import java.util.Locale;
+import java.util.Vector;
+import org.apache.spark.SparkException;
+
+/**
+ * Methods for handling SQL identifiers. These methods were cribbed
+ * from org.apache.derby.iapi.util.IdUtil and
+ * org.apache.derby.iapi.util.StringUtil.
+ */
+public class SqlIdentifierUtil {
--- End diff --
Oh right. I read right past the reasoning. It's not just inspired by Derby but copied wholesale on purpose. OK, that makes some sense, more than making a dependency on Derby. It has some 'cost' though.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/9202#issuecomment-150017407
Merged build triggered.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
Posted by rick-ibm <gi...@git.apache.org>.
Github user rick-ibm commented on the pull request:
https://github.com/apache/spark/pull/9202#issuecomment-150025478
According to the log for the build/test cycle, a git pull request succeeded...
GitHub pull request #9202 of commit b3b845de9960e003637d26fecaf8dccaa0206f59 automatically merged.
...but a git fetch failed after 15 minutes...
git fetch --tags --progress https://github.com/apache/spark.git +refs/pull/9202/*:refs/remotes/origin/pr/9202/* # timeout=15
Would appreciate some clues about what I've done wrong.
Thanks,
-Rick
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/9202#issuecomment-150027762
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/44094/
Test FAILed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
Posted by rick-ibm <gi...@git.apache.org>.
Github user rick-ibm commented on the pull request:
https://github.com/apache/spark/pull/9202#issuecomment-152638711
Thanks for that feedback, Michael. To answer your question:
MA> I haven't looked closely at the implementation, but one high level question is whether this is breaking the use case where a users gives a subquery instead of a table name (i.e. dbtable = (SELECT ...).) This is an important part of the API that we can't break.
Reynold posed the same question at https://issues.apache.org/jira/browse/SPARK-10857?focusedCommentId=14967827&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14967827 and I replied at https://issues.apache.org/jira/browse/SPARK-10857?focusedCommentId=14967930&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14967930
Can you give me an example of the syntax you have in mind which might trip this problem? I tried the following experiment...
val jdbcDF = sqlContext.read.format("jdbc").options(
Map("url" -> "jdbc:derby:/Users/rhillegas/derby/databases/derby1",
"dbtable" -> "select * from app.t")).load()
...but that statement doesn't tickle JdbcDialect.getTableExistsQuery(). Instead, the statement fails because the following ungrammatical query is sent to the database:
SELECT * FROM select * from app.t WHERE 1=0
That, in turn, is because JDBCRDD.resolveTable() has its own hard-coded query which probes for the existence of a table. I have logged https://issues.apache.org/jira/browse/SPARK-11426 to track this issue. The header comment on JDBCRDD.resolveTable() indicates that the method expects a table name and not an arbitrary query.
Thanks,
-Rick
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
Posted by rick-ibm <gi...@git.apache.org>.
Github user rick-ibm commented on the pull request:
https://github.com/apache/spark/pull/9202#issuecomment-150267546
Thanks for the review comments, Sean.
I did not polish the Java code in SqlIdentifierUtil, and I didn't translate it into Scala. This was in the interests of stability (see the header comment on my pull request above). The code came from Derby, where it has been proven by two decades of use in production applications. But I am happy to re-write it in Scala if you think that the benefits outweigh the risk of introducing bugs.
Concerning JdbcDialects.vetSqlIdentifier(), thanks for the suggestion about using a case class. That does sound like a better solution.
Thanks,
-Rick
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/9202#issuecomment-151608509
Merged build started.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
Posted by rick-ibm <gi...@git.apache.org>.
Github user rick-ibm commented on the pull request:
https://github.com/apache/spark/pull/9202#issuecomment-150043741
Thanks, Josh. My last commit should address that long line problem.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/9202#issuecomment-150043998
Merged build started.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/9202#discussion_r42743817
--- Diff: sql/catalyst/src/main/java/org/apache/spark/sql/SqlIdentifierUtil.java ---
@@ -0,0 +1,322 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql;
+
+import java.io.IOException;
+import java.io.StringReader;
+import java.util.Locale;
+import java.util.Vector;
+import org.apache.spark.SparkException;
+
+/**
+ * Methods for handling SQL identifiers. These methods were cribbed
+ * from org.apache.derby.iapi.util.IdUtil and
+ * org.apache.derby.iapi.util.StringUtil.
+ */
+public class SqlIdentifierUtil {
+
+ /**
+ * Quote a string so that it can be used as an identifier or a string
+ * literal in SQL statements. Identifiers are surrounded by double quotes
+ * and string literals are surrounded by single quotes. If the string
+ * contains quote characters, they are escaped.
+ *
+ * @param source the string to quote
+ * @param quote the character to quote the string with (' or ")
+ * @return a string quoted with the specified quote character
+ */
+ public static String quoteString(String source, char quote) {
+ // Normally, the quoted string is two characters longer than the source
+ // string (because of start quote and end quote).
+ StringBuffer quoted = new StringBuffer(source.length() + 2);
+ quoted.append(quote);
+ for (int i = 0; i < source.length(); i++) {
+ char c = source.charAt(i);
+ // if the character is a quote, escape it with an extra quote
+ if (c == quote) quoted.append(quote);
+ quoted.append(c);
+ }
+ quoted.append(quote);
+ return quoted.toString();
+ }
+
+ /**
+ * Parse a multi-part (dot separated) SQL identifier from the
+ * String provided. Raise an excepion
+ * if the string does not contain valid SQL indentifiers.
+ * The returned String array contains the normalized form of the
+ * identifiers.
+ *
+ * @param s The string to be parsed
+ * @param quoteCharacter The character which frames a delimited id (e.g., " or `)
+ * @param upperCaseIdentifiers True if SQL ids are normalized to upper case.
+ * @return An array of strings made by breaking the input string at its dots, '.'.
+ * @throws SparkException Invalid SQL identifier.
+ */
+ public static String[] parseMultiPartSQLIdentifier(
+ String s,
+ char quoteCharacter,
+ boolean upperCaseIdentifiers)
+ throws SparkException {
+ StringReader r = new StringReader(s);
+ String[] qName = parseMultiPartSQLIdentifier(
+ s,
+ r,
+ quoteCharacter,
+ upperCaseIdentifiers);
+ verifyEmpty(s, r);
+ return qName;
+ }
+
+ /**
+ * Parse a multi-part (dot separated) SQL identifier from the
+ * String provided. Raise an excepion
+ * if the string does not contain valid SQL indentifiers.
+ * The returned String array contains the normalized form of the
+ * identifiers.
+ *
+ * @param orig The full text being parsed
+ * @param r The multi-part identifier to be parsed
+ * @param quoteCharacter The character which frames a delimited id (e.g., " or `)
+ * @param upperCaseIdentifiers True if SQL ids are normalized to upper case.
+ * @return An array of strings made by breaking the input string at its dots, '.'.
+ * @throws SparkException Invalid SQL identifier.
+ */
+ private static String[] parseMultiPartSQLIdentifier(
--- End diff --
A lot of this looks accomplishable more simply with regexes. Is that not feasible?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/9202#issuecomment-153807176
**[Test build #45027 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/45027/consoleFull)** for PR 9202 at commit [`6c906bb`](https://github.com/apache/spark/commit/6c906bb21a04a99ddf992863f552ed9e0b36d574).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
Posted by rick-ibm <gi...@git.apache.org>.
Github user rick-ibm commented on the pull request:
https://github.com/apache/spark/pull/9202#issuecomment-152542748
Any further refinements which I should make to this patch? Thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
Posted by rick-ibm <gi...@git.apache.org>.
Github user rick-ibm commented on the pull request:
https://github.com/apache/spark/pull/9202#issuecomment-151608061
Thanks for the advice about how to improve this patch, Sean. Hopefully, my last commit addresses your concerns. I have made the following changes:
1) Eliminated the Java code (SqlIdentifierUtil) and replaced it with a Scala class (SqlIdUtil).
2) Revamped the id parsing to use a Scala regular expression.
3) Introduced a case class (TableId) to represent a 3-part table identfier. This replaces the ordered triple which performed the same service in the previous rev of the patch.
Thanks,
-Rick
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/9202#issuecomment-153842578
**[Test build #45027 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/45027/consoleFull)** for PR 9202 at commit [`6c906bb`](https://github.com/apache/spark/commit/6c906bb21a04a99ddf992863f552ed9e0b36d574).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds the following public classes _(experimental)_:\n * `case class ExecutorLostFailure(`\n * ` case class RegisteredExecutor(hostname: String) extends CoarseGrainedClusterMessage`\n * `public class JavaIsotonicRegressionExample `\n * `public class JavaNaiveBayesExample `\n * `case class TableId(database: String, schema: String, table: String)`\n * ` case class DecimalLit(chars: String) extends Token `\n * `case class UnresolvedStar(target: Option[Seq[String]]) extends Star with Unevaluable `\n * `case class Corr(`\n * `case class Corr(left: Expression, right: Expression)`\n * `case class Exchange(`\n * `class CoalescedPartitioner(val parent: Partitioner, val partitionStartIndices: Array[Int])`\n
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/9202#issuecomment-150064993
Merged build finished. Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on the pull request:
https://github.com/apache/spark/pull/9202#issuecomment-152648785
You have to put the subquery in parentheses so it results in valid SQL.
On Oct 30, 2015 9:06 PM, "Rick Hillegas" <no...@github.com> wrote:
> Thanks for that feedback, Michael. To answer your question:
>
> MA> I haven't looked closely at the implementation, but one high level
> question is whether this is breaking the use case where a users gives a
> subquery instead of a table name (i.e. dbtable = (SELECT ...).) This is an
> important part of the API that we can't break.
>
> Reynold posed the same question at
> https://issues.apache.org/jira/browse/SPARK-10857?focusedCommentId=14967827&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14967827
> and I replied at
> https://issues.apache.org/jira/browse/SPARK-10857?focusedCommentId=14967930&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14967930
>
> Can you give me an example of the syntax you have in mind which might trip
> this problem? I tried the following experiment...
>
> val jdbcDF = sqlContext.read.format("jdbc").options(
> Map("url" -> "jdbc:derby:/Users/rhillegas/derby/databases/derby1",
> "dbtable" -> "select * from app.t")).load()
>
> ...but that statement doesn't tickle JdbcDialect.getTableExistsQuery().
> Instead, the statement fails because the following ungrammatical query is
> sent to the database:
>
> SELECT * FROM select * from app.t WHERE 1=0
>
> That, in turn, is because JDBCRDD.resolveTable() has its own hard-coded
> query which probes for the existence of a table. I have logged
> https://issues.apache.org/jira/browse/SPARK-11426 to track this issue.
> The header comment on JDBCRDD.resolveTable() indicates that the method
> expects a table name and not an arbitrary query.
>
> Thanks,
> -Rick
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/apache/spark/pull/9202#issuecomment-152638711>.
>
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
Posted by rick-ibm <gi...@git.apache.org>.
Github user rick-ibm commented on the pull request:
https://github.com/apache/spark/pull/9202#issuecomment-152661995
Thanks for the quick response, Michael. Simply parenthesizing the query will result in non-Standard syntax which an ANSI-compliant database will reject...
// Raises the unhelpful error: Syntax error: Encountered "WHERE" at line 1, column 37.
try {
val jdbcDF = sqlContext.read.format("jdbc").options(
Map("url" -> "jdbc:derby:/Users/rhillegas/derby/databases/derby1",
"dbtable" -> "(select * from app.t)")).load()
} catch {
case t: Throwable => println(t.getMessage)
}
However, you can go one step further and give the subquery a correlation name so that it will fit nicely in a FROM list...
// succeeds
try {
val jdbcDF = sqlContext.read.format("jdbc").options(
Map("url" -> "jdbc:derby:/Users/rhillegas/derby/databases/derby1",
"dbtable" -> "(select * from app.t) correlationName")).load()
} catch {
case t: Throwable => println(t.getMessage)
}
I am inclined to close SPARK-11426 as "not a problem". It seems that JDBCRDD.resolveTable() deliberately doesn't call JdbcDialect.getTableExistsQuery() because it is not really probing for the existence of the table. It is trying to compute the row-signature of a query expression.
In any event, the changes made by this patch do not seem to affect the code path you are concerned about.
Thanks,
-Rick
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/9202#issuecomment-151641902
**[Test build #44457 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/44457/consoleFull)** for PR 9202 at commit [`7d90459`](https://github.com/apache/spark/commit/7d90459fca34e3ff1cbc1ebf62c717aebb052307).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds the following public classes _(experimental)_:\n * `case class TableId(database: String, schema: String, table: String)`\n
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/9202#issuecomment-150017438
Merged build started.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/9202#issuecomment-150032928
```
Scalastyle checks failed at following occurrences:
[error] /home/jenkins/workspace/SparkPullRequestBuilder@3/sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCWriteSuite.scala:167: File line length exceeds 100 characters
[error] (sql/test:scalastyle) errors exist
[error] Total time: 8 s, completed Oct 21, 2015 2:20:49 PM
[error] /home/jenkins/workspace/SparkPullRequestBuilder@3/sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCWriteSuite.scala:167: File line length exceeds 100 characters
[error] (sql/test:scalastyle) errors exist
[error] Total time: 8 s, completed Oct 21, 2015 2:21:32 PM
[error] running /home/jenkins/workspace/SparkPullRequestBuilder@3/dev/lint-scala ; received return code 1
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/9202#issuecomment-150027189
**[Test build #44094 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/44094/consoleFull)** for PR 9202 at commit [`b3b845d`](https://github.com/apache/spark/commit/b3b845de9960e003637d26fecaf8dccaa0206f59).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/9202#issuecomment-151642085
Merged build finished. Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/9202#discussion_r42743556
--- Diff: sql/catalyst/src/main/java/org/apache/spark/sql/SqlIdentifierUtil.java ---
@@ -0,0 +1,322 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql;
+
+import java.io.IOException;
+import java.io.StringReader;
+import java.util.Locale;
+import java.util.Vector;
+import org.apache.spark.SparkException;
+
+/**
+ * Methods for handling SQL identifiers. These methods were cribbed
+ * from org.apache.derby.iapi.util.IdUtil and
+ * org.apache.derby.iapi.util.StringUtil.
+ */
+public class SqlIdentifierUtil {
--- End diff --
As an aside, does this need to be Java? it would be simpler in Scala. I may be missing a reason it must be.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on the pull request:
https://github.com/apache/spark/pull/9202#issuecomment-150025997
Transient networking issues. test this please
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/9202#issuecomment-153806547
Merged build started.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
Posted by rick-ibm <gi...@git.apache.org>.
Github user rick-ibm commented on the pull request:
https://github.com/apache/spark/pull/9202#issuecomment-150032606
The following style tests ran cleanly for me:
build/sbt scalastyle
What other style tests should I run?
Thanks,
-Rick
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on the pull request:
https://github.com/apache/spark/pull/9202#issuecomment-150016295
ok to test
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/9202#issuecomment-150027760
Merged build finished. Test FAILed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/9202#issuecomment-150045688
**[Test build #44096 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/44096/consoleFull)** for PR 9202 at commit [`e7e401f`](https://github.com/apache/spark/commit/e7e401f810af28a84b5a40300e6f5ea3f3d96fe4).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #9202: [SPARK-10857] [SQL] Block SQL injection vulnerabilities u...
Posted by sameeragarwal <gi...@git.apache.org>.
Github user sameeragarwal commented on the issue:
https://github.com/apache/spark/pull/9202
@rick-ibm would you still have time to bring this up to date? Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/9202#issuecomment-150064995
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/44096/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/9202#issuecomment-150027756
**[Test build #44094 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/44094/consoleFull)** for PR 9202 at commit [`b3b845d`](https://github.com/apache/spark/commit/b3b845de9960e003637d26fecaf8dccaa0206f59).
* This patch **fails Scala style tests**.
* This patch merges cleanly.
* This patch adds the following public classes _(experimental)_:\n * `public class SqlIdentifierUtil `\n
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/9202#issuecomment-150020938
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/44091/
Test FAILed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/9202#discussion_r42743770
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala ---
@@ -86,19 +94,58 @@ abstract class JdbcDialect {
* name is a reserved keyword, or in case it contains characters that require quotes (e.g. space).
*/
def quoteIdentifier(colName: String): String = {
- s""""$colName""""
+ quoteString(colName, quoteChar)
+ }
+
+ /**
+ * Get the SQL query that should be used to find if the given table exists.
+ * Call this method (and not tableExistsQuery) in order to verify
+ * that the table name is properly formed.
+ * @param table The name of the table.
+ * @return The SQL query to use for checking the table.
+ * @throws org.apache.spark.SparkException On invalid table name.
+ */
+ final def getTableExistsQuery(table: String): String = {
+ vetSqlIdentifier(table)
+ tableExistsQuery(table)
}
/**
* Get the SQL query that should be used to find if the given table exists. Dialects can
* override this method to return a query that works best in a particular database.
+ * Don't expose this method outside this class and its subclasses.
+ * Other consumers should call getTableExistsQuery instead. That method
+ * verifies that the table name is properly formed.
* @param table The name of the table.
* @return The SQL query to use for checking the table.
*/
- def getTableExistsQuery(table: String): String = {
+ protected def tableExistsQuery(table: String): String = {
s"SELECT * FROM $table WHERE 1=0"
}
+ /** Vet a user-supplied object id of the form
+ * [[catalog.]schema.]objectName
+ * by parsing it into a (catalog, schema, objectName)
+ * triple. The catalog and schema names may be empty. Raises
+ * a SparkException if the user-supplied id is malformed,
+ * e.g., is a string like "foo; drop database finance;",
+ * something intended for a SQL injection attack.
+ *
+ * @param rawId The user-supplied object id (name).
+ * @throws org.apache.spark.SparkException On invalid ids.
+ */
+ def vetSqlIdentifier(rawId: String) {
--- End diff --
How about a case class rather than generic tuple?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #9202: [SPARK-10857] [SQL] Block SQL injection vulnerabilities u...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/9202
We are closing it due to inactivity. please do reopen if you want to push it forward. Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10857] [SQL] Block SQL injection vulner...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/9202#issuecomment-151608474
Merged build triggered.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org