You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by parthchandra <gi...@git.apache.org> on 2017/11/23 03:03:52 UTC
[GitHub] drill pull request #1050: DRILL-5694: Do not allow queries to access paths o...
GitHub user parthchandra opened a pull request:
https://github.com/apache/drill/pull/1050
DRILL-5694: Do not allow queries to access paths outside the current …
Added check to prevent users from accessing a path outside the current workspace. For backward compatibility this introduces a new parameter 'allowAccessOutsideWorkspace' in the dfs storage plugin configuration that allows the user to override the check and allow access outside the workspace. The default value for the parameter is false. Any existing storage plugin configurations which do not have the parameter specified will no longer be able to access paths outside the workspace.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/parthchandra/drill DRILL-5964
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/1050.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 #1050
----
commit 6fa5ac7eb1520af6def3fb9f6ebd35ee7f982b01
Author: Parth Chandra <pa...@apache.org>
Date: 2017-11-14T23:25:33Z
DRILL-5694: Do not allow queries to access paths outside the current workspace root
----
---
[GitHub] drill pull request #1050: DRILL-5964: Do not allow queries to access paths o...
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/1050#discussion_r152774087
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSelection.java ---
@@ -359,15 +363,30 @@ private static Path handleWildCard(final String root) {
}
}
- private static String removeLeadingSlash(String path) {
- if (path.charAt(0) == '/') {
+ public static String removeLeadingSlash(String path) {
+ if (!path.isEmpty() && path.charAt(0) == '/') {
String newPath = path.substring(1);
return removeLeadingSlash(newPath);
} else {
return path;
}
}
+ // Check if the path is a valid sub path under the parent after removing backpaths. Throw an exception if
+ // it is not
+ // We pass subpath in as a parameter only for the error message
+ public static boolean checkBackPaths(String parent, String combinedPath, String subpath) {
+ Preconditions.checkArgument(!parent.isEmpty());
+ Preconditions.checkArgument(!combinedPath.isEmpty());
--- End diff --
Please add message for pre-conditions so error message will be more clear.
---
[GitHub] drill pull request #1050: DRILL-5964: Do not allow queries to access paths o...
Posted by parthchandra <gi...@git.apache.org>.
Github user parthchandra commented on a diff in the pull request:
https://github.com/apache/drill/pull/1050#discussion_r152862636
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSelection.java ---
@@ -252,11 +252,15 @@ private static String buildPath(final String[] path, final int folderIndex) {
return builder.toString();
}
- public static FileSelection create(final DrillFileSystem fs, final String parent, final String path) throws IOException {
+ public static FileSelection create(final DrillFileSystem fs, final String parent, final String path,
+ final boolean allowAccessOutsideWorkspace) throws IOException {
Stopwatch timer = Stopwatch.createStarted();
boolean hasWildcard = path.contains(WILD_CARD);
final Path combined = new Path(parent, removeLeadingSlash(path));
+ if (!allowAccessOutsideWorkspace) {
+ checkBackPaths(parent, combined.toString(), path);
--- End diff --
Done
---
[GitHub] drill pull request #1050: DRILL-5964: Do not allow queries to access paths o...
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/1050#discussion_r152774820
--- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/store/dfs/TestFileSelection.java ---
@@ -59,4 +61,53 @@ public void testEmptyFolderThrowsTableNotFound() throws Exception {
throw ex;
}
}
+
+ @Test
+ public void testBackPath() throws Exception {
--- End diff --
Please split into two tests.
---
[GitHub] drill issue #1050: DRILL-5964: Do not allow queries to access paths outside ...
Posted by parthchandra <gi...@git.apache.org>.
Github user parthchandra commented on the issue:
https://github.com/apache/drill/pull/1050
Changed the type of allowAccessOutsideWorkspace to boolean. This changes behavior. All existing dfs workspaces will also disallow access outside the workspace. Ideally, we should deprecate this parameter in a future release, but for the moment it allows existing users to access paths outside the workspace if they want to.
---
[GitHub] drill pull request #1050: DRILL-5964: Do not allow queries to access paths o...
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/1050#discussion_r153436976
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/WorkspaceConfig.java ---
@@ -30,18 +30,24 @@
public class WorkspaceConfig {
/** Default workspace is a root directory which supports read, but not write. */
- public static final WorkspaceConfig DEFAULT = new WorkspaceConfig("/", false, null);
+ public static final WorkspaceConfig DEFAULT = new WorkspaceConfig("/", false, null, false);
private final String location;
private final boolean writable;
private final String defaultInputFormat;
-
+ private final Boolean allowAccessOutsideWorkspace; // allow access outside the workspace by default. This
--- End diff --
1. Can we adjust the variable to be false by default, i.e. rename it to `disallowAccessOutsideWorkspace`? Thus we'll be able to use primitive, right?
2. In the below code you always set `this.allowAccessOutsideWorkspace = true;`, block with `this.allowAccessOutsideWorkspace = allowAccessOutsideWorkspace != null ? allowAccessOutsideWorkspace : false ;` is commented. I guess this is a mistake.
---
[GitHub] drill issue #1050: DRILL-5964: Do not allow queries to access paths outside ...
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on the issue:
https://github.com/apache/drill/pull/1050
+1, LGTM.
---
[GitHub] drill pull request #1050: DRILL-5964: Do not allow queries to access paths o...
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/1050#discussion_r152772486
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSelection.java ---
@@ -359,15 +363,30 @@ private static Path handleWildCard(final String root) {
}
}
- private static String removeLeadingSlash(String path) {
- if (path.charAt(0) == '/') {
+ public static String removeLeadingSlash(String path) {
+ if (!path.isEmpty() && path.charAt(0) == '/') {
String newPath = path.substring(1);
return removeLeadingSlash(newPath);
} else {
return path;
}
}
+ // Check if the path is a valid sub path under the parent after removing backpaths. Throw an exception if
+ // it is not
+ // We pass subpath in as a parameter only for the error message
+ public static boolean checkBackPaths(String parent, String combinedPath, String subpath) {
--- End diff --
This method can return void, since it never will return false. I see you use this during testing but it can be tested with void type as well.
---
[GitHub] drill pull request #1050: DRILL-5964: Do not allow queries to access paths o...
Posted by parthchandra <gi...@git.apache.org>.
Github user parthchandra commented on a diff in the pull request:
https://github.com/apache/drill/pull/1050#discussion_r152862693
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSelection.java ---
@@ -359,15 +363,30 @@ private static Path handleWildCard(final String root) {
}
}
- private static String removeLeadingSlash(String path) {
- if (path.charAt(0) == '/') {
+ public static String removeLeadingSlash(String path) {
+ if (!path.isEmpty() && path.charAt(0) == '/') {
String newPath = path.substring(1);
return removeLeadingSlash(newPath);
} else {
return path;
}
}
+ // Check if the path is a valid sub path under the parent after removing backpaths. Throw an exception if
+ // it is not
+ // We pass subpath in as a parameter only for the error message
+ public static boolean checkBackPaths(String parent, String combinedPath, String subpath) {
--- End diff --
Done
---
[GitHub] drill pull request #1050: DRILL-5964: Do not allow queries to access paths o...
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/1050#discussion_r152773573
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/WorkspaceConfig.java ---
@@ -30,18 +30,24 @@
public class WorkspaceConfig {
/** Default workspace is a root directory which supports read, but not write. */
- public static final WorkspaceConfig DEFAULT = new WorkspaceConfig("/", false, null);
+ public static final WorkspaceConfig DEFAULT = new WorkspaceConfig("/", false, null, false);
private final String location;
private final boolean writable;
private final String defaultInputFormat;
-
+ private final Boolean allowAccessOutsideWorkspace; // allow access outside the workspace by default. This
--- End diff --
Consider using primitive, we don't want unnecessary boxing / unboxing.
---
[GitHub] drill pull request #1050: DRILL-5964: Do not allow queries to access paths o...
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/1050#discussion_r153337275
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/WorkspaceConfig.java ---
@@ -30,18 +30,25 @@
public class WorkspaceConfig {
/** Default workspace is a root directory which supports read, but not write. */
- public static final WorkspaceConfig DEFAULT = new WorkspaceConfig("/", false, null);
+ public static final WorkspaceConfig DEFAULT = new WorkspaceConfig("/", false, null, false);
private final String location;
private final boolean writable;
private final String defaultInputFormat;
-
+ private final Boolean allowAccessOutsideWorkspace; // allow access outside the workspace by default. This
+ // field is a Boolean (not boolean) so that we can
+ // assign a default value if it is not defined in a
+ // storage plugin config
public WorkspaceConfig(@JsonProperty("location") String location,
@JsonProperty("writable") boolean writable,
- @JsonProperty("defaultInputFormat") String defaultInputFormat) {
+ @JsonProperty("defaultInputFormat") String defaultInputFormat,
+ @JsonProperty("allowAccessOutsideWorkspace") Boolean allowAccessOutsideWorkspace
+ ) {
this.location = location;
this.writable = writable;
this.defaultInputFormat = defaultInputFormat;
+ //this.allowAccessOutsideWorkspace = allowAccessOutsideWorkspace != null ? allowAccessOutsideWorkspace : false ;
+ this.allowAccessOutsideWorkspace = true;
--- End diff --
It seems we should not always set true...
---
[GitHub] drill pull request #1050: DRILL-5964: Do not allow queries to access paths o...
Posted by parthchandra <gi...@git.apache.org>.
Github user parthchandra commented on a diff in the pull request:
https://github.com/apache/drill/pull/1050#discussion_r153388800
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/WorkspaceConfig.java ---
@@ -30,18 +30,24 @@
public class WorkspaceConfig {
/** Default workspace is a root directory which supports read, but not write. */
- public static final WorkspaceConfig DEFAULT = new WorkspaceConfig("/", false, null);
+ public static final WorkspaceConfig DEFAULT = new WorkspaceConfig("/", false, null, false);
private final String location;
private final boolean writable;
private final String defaultInputFormat;
-
+ private final Boolean allowAccessOutsideWorkspace; // allow access outside the workspace by default. This
--- End diff --
Yes it would, I believe. But we want the value to be `true` for backward compatibility. (This also addresses your next comment). So we need to know if the value is missing. Can only do that with a non primitive AFAIK.
---
[GitHub] drill pull request #1050: DRILL-5964: Do not allow queries to access paths o...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/1050
---
[GitHub] drill pull request #1050: DRILL-5964: Do not allow queries to access paths o...
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/1050#discussion_r153336979
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/WorkspaceConfig.java ---
@@ -30,18 +30,24 @@
public class WorkspaceConfig {
/** Default workspace is a root directory which supports read, but not write. */
- public static final WorkspaceConfig DEFAULT = new WorkspaceConfig("/", false, null);
+ public static final WorkspaceConfig DEFAULT = new WorkspaceConfig("/", false, null, false);
private final String location;
private final boolean writable;
private final String defaultInputFormat;
-
+ private final Boolean allowAccessOutsideWorkspace; // allow access outside the workspace by default. This
--- End diff --
As far I remember `@JsonProperty("allowAccessOutsideWorkspace") boolean allowAccessOutsideWorkspace` will set false by default if value is not present during deserialization. Could you please check?
---
[GitHub] drill pull request #1050: DRILL-5964: Do not allow queries to access paths o...
Posted by parthchandra <gi...@git.apache.org>.
Github user parthchandra commented on a diff in the pull request:
https://github.com/apache/drill/pull/1050#discussion_r153318818
--- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/store/dfs/TestFileSelection.java ---
@@ -59,4 +61,53 @@ public void testEmptyFolderThrowsTableNotFound() throws Exception {
throw ex;
}
}
+
+ @Test
+ public void testBackPath() throws Exception {
--- End diff --
Done
---
[GitHub] drill pull request #1050: DRILL-5964: Do not allow queries to access paths o...
Posted by parthchandra <gi...@git.apache.org>.
Github user parthchandra commented on a diff in the pull request:
https://github.com/apache/drill/pull/1050#discussion_r152862954
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/WorkspaceConfig.java ---
@@ -30,18 +30,24 @@
public class WorkspaceConfig {
/** Default workspace is a root directory which supports read, but not write. */
- public static final WorkspaceConfig DEFAULT = new WorkspaceConfig("/", false, null);
+ public static final WorkspaceConfig DEFAULT = new WorkspaceConfig("/", false, null, false);
private final String location;
private final boolean writable;
private final String defaultInputFormat;
-
+ private final Boolean allowAccessOutsideWorkspace; // allow access outside the workspace by default. This
--- End diff --
I need to check if the value is not present (i.e. null). That will be the case with all storage plugin configurations that have already been created.
---
[GitHub] drill pull request #1050: DRILL-5964: Do not allow queries to access paths o...
Posted by parthchandra <gi...@git.apache.org>.
Github user parthchandra commented on a diff in the pull request:
https://github.com/apache/drill/pull/1050#discussion_r152862722
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSelection.java ---
@@ -359,15 +363,30 @@ private static Path handleWildCard(final String root) {
}
}
- private static String removeLeadingSlash(String path) {
- if (path.charAt(0) == '/') {
+ public static String removeLeadingSlash(String path) {
+ if (!path.isEmpty() && path.charAt(0) == '/') {
String newPath = path.substring(1);
return removeLeadingSlash(newPath);
} else {
return path;
}
}
+ // Check if the path is a valid sub path under the parent after removing backpaths. Throw an exception if
+ // it is not
+ // We pass subpath in as a parameter only for the error message
+ public static boolean checkBackPaths(String parent, String combinedPath, String subpath) {
+ Preconditions.checkArgument(!parent.isEmpty());
+ Preconditions.checkArgument(!combinedPath.isEmpty());
--- End diff --
Done
---
[GitHub] drill pull request #1050: DRILL-5964: Do not allow queries to access paths o...
Posted by parthchandra <gi...@git.apache.org>.
Github user parthchandra commented on a diff in the pull request:
https://github.com/apache/drill/pull/1050#discussion_r152862665
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSelection.java ---
@@ -359,15 +363,30 @@ private static Path handleWildCard(final String root) {
}
}
- private static String removeLeadingSlash(String path) {
- if (path.charAt(0) == '/') {
+ public static String removeLeadingSlash(String path) {
+ if (!path.isEmpty() && path.charAt(0) == '/') {
String newPath = path.substring(1);
return removeLeadingSlash(newPath);
} else {
return path;
}
}
+ // Check if the path is a valid sub path under the parent after removing backpaths. Throw an exception if
--- End diff --
Done
---
[GitHub] drill pull request #1050: DRILL-5964: Do not allow queries to access paths o...
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/1050#discussion_r152773919
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSelection.java ---
@@ -252,11 +252,15 @@ private static String buildPath(final String[] path, final int folderIndex) {
return builder.toString();
}
- public static FileSelection create(final DrillFileSystem fs, final String parent, final String path) throws IOException {
+ public static FileSelection create(final DrillFileSystem fs, final String parent, final String path,
+ final boolean allowAccessOutsideWorkspace) throws IOException {
Stopwatch timer = Stopwatch.createStarted();
boolean hasWildcard = path.contains(WILD_CARD);
final Path combined = new Path(parent, removeLeadingSlash(path));
+ if (!allowAccessOutsideWorkspace) {
+ checkBackPaths(parent, combined.toString(), path);
--- End diff --
I usually void using `toString` for `Path`, consider using `combined.toUri().getPath()`.
---
[GitHub] drill pull request #1050: DRILL-5964: Do not allow queries to access paths o...
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/1050#discussion_r152772350
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSelection.java ---
@@ -359,15 +363,30 @@ private static Path handleWildCard(final String root) {
}
}
- private static String removeLeadingSlash(String path) {
- if (path.charAt(0) == '/') {
+ public static String removeLeadingSlash(String path) {
+ if (!path.isEmpty() && path.charAt(0) == '/') {
String newPath = path.substring(1);
return removeLeadingSlash(newPath);
} else {
return path;
}
}
+ // Check if the path is a valid sub path under the parent after removing backpaths. Throw an exception if
--- End diff --
Please use java doc.
---