You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2021/05/04 12:23:21 UTC
[GitHub] [accumulo] slackwinner opened a new pull request #2075: Add fromClassPath Method
slackwinner opened a new pull request #2075:
URL: https://github.com/apache/accumulo/pull/2075
This commit contains introduces a fourth from() that accepts URL paths to accumulo-client.properties. This additional method would allow users to build an Accumulo client from the properties file embedded in the classpath.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a change in pull request #2075: Add fromClassPath Method
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2075:
URL: https://github.com/apache/accumulo/pull/2075#discussion_r627559708
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/ClientInfoImpl.java
##########
@@ -115,6 +120,18 @@ public static Properties toProperties(Path propertiesFile) {
return properties;
}
+ @SuppressFBWarnings(value = "URLCONNECTION_SSRF_FD",
+ justification = "code runs in same security context as user who provided propertiesURL")
+ public static Properties toProperties(URL propertiesURL) {
+ Properties properties = new Properties();
+ try (InputStream is = propertiesURL.openStream()) {
Review comment:
You could add some additional validation to make sure this is just a properties file and not something malicious.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a change in pull request #2075: Add fromClassPath Method
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2075:
URL: https://github.com/apache/accumulo/pull/2075#discussion_r628265358
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/ClientInfoImpl.java
##########
@@ -115,6 +120,18 @@ public static Properties toProperties(Path propertiesFile) {
return properties;
}
+ @SuppressFBWarnings(value = "URLCONNECTION_SSRF_FD",
+ justification = "code runs in same security context as user who provided propertiesURL")
+ public static Properties toProperties(URL propertiesURL) {
+ Properties properties = new Properties();
+ try (InputStream is = propertiesURL.openStream()) {
Review comment:
> Or, are you thinking that parsing the URL itself in order to locate the stream to read from could itself cause code to be executed?
Yes. But a URL _could_ be a lot of things and there is nothing preventing the user from opening a stream to "http://www.badguys.com/maliciousFile.exe". This is what I thought the sec-bugs error was highlighting.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a change in pull request #2075: Add fromClassPath Method
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2075:
URL: https://github.com/apache/accumulo/pull/2075#discussion_r627703570
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/ClientInfoImpl.java
##########
@@ -115,6 +120,18 @@ public static Properties toProperties(Path propertiesFile) {
return properties;
}
+ @SuppressFBWarnings(value = "URLCONNECTION_SSRF_FD",
+ justification = "code runs in same security context as user who provided propertiesURL")
+ public static Properties toProperties(URL propertiesURL) {
+ Properties properties = new Properties();
+ try (InputStream is = propertiesURL.openStream()) {
Review comment:
That's a good point. I was just thinking he could do something simple, like check to see if the URL ends with a valid extension but that will probably get vetted on `load()`. I told @slackwinner that it would be a good exercise to figure out how to sanitize a user parameter to prevent malcious code execution.
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/ClientInfoImpl.java
##########
@@ -115,6 +120,18 @@ public static Properties toProperties(Path propertiesFile) {
return properties;
}
+ @SuppressFBWarnings(value = "URLCONNECTION_SSRF_FD",
+ justification = "code runs in same security context as user who provided propertiesURL")
+ public static Properties toProperties(URL propertiesURL) {
+ Properties properties = new Properties();
+ try (InputStream is = propertiesURL.openStream()) {
Review comment:
That's a good point. I was just thinking he could do something simple, like check to see if the URL ends with a valid extension but that will probably get vetted on `load()`. I told @slackwinner that it would be a good exercise to figure out how to sanitize a user parameter to prevent malicious code execution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2075: Add fromClassPath Method
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2075:
URL: https://github.com/apache/accumulo/pull/2075#discussion_r627691242
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/ClientInfoImpl.java
##########
@@ -115,6 +120,18 @@ public static Properties toProperties(Path propertiesFile) {
return properties;
}
+ @SuppressFBWarnings(value = "URLCONNECTION_SSRF_FD",
+ justification = "code runs in same security context as user who provided propertiesURL")
+ public static Properties toProperties(URL propertiesURL) {
+ Properties properties = new Properties();
+ try (InputStream is = propertiesURL.openStream()) {
Review comment:
@milleruntime What kind of validation were you thinking? It's just getting the data from the data source. One would think that the Properties object would validate that the contents are appropriate when calling `load()`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a change in pull request #2075: Add fromClassPath Method
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2075:
URL: https://github.com/apache/accumulo/pull/2075#discussion_r628265358
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/ClientInfoImpl.java
##########
@@ -115,6 +120,18 @@ public static Properties toProperties(Path propertiesFile) {
return properties;
}
+ @SuppressFBWarnings(value = "URLCONNECTION_SSRF_FD",
+ justification = "code runs in same security context as user who provided propertiesURL")
+ public static Properties toProperties(URL propertiesURL) {
+ Properties properties = new Properties();
+ try (InputStream is = propertiesURL.openStream()) {
Review comment:
> Or, are you thinking that parsing the URL itself in order to locate the stream to read from could itself cause code to be executed?
Yes. But a URL _could_ be a lot of things and there is nothing preventing the user from opening a stream to "http://www.badguys.com/maliciousFile.exe"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii merged pull request #2075: Add fromClassPath Method
Posted by GitBox <gi...@apache.org>.
ctubbsii merged pull request #2075:
URL: https://github.com/apache/accumulo/pull/2075
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] slackwinner commented on a change in pull request #2075: Add fromClassPath Method
Posted by GitBox <gi...@apache.org>.
slackwinner commented on a change in pull request #2075:
URL: https://github.com/apache/accumulo/pull/2075#discussion_r628151713
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/ClientInfoImpl.java
##########
@@ -115,6 +120,18 @@ public static Properties toProperties(Path propertiesFile) {
return properties;
}
+ @SuppressFBWarnings(value = "URLCONNECTION_SSRF_FD",
+ justification = "code runs in same security context as user who provided propertiesURL")
+ public static Properties toProperties(URL propertiesURL) {
+ Properties properties = new Properties();
+ try (InputStream is = propertiesURL.openStream()) {
Review comment:
Well I was thinking we can ensure the URL contains a properties extension before executing load() to achieve fast failure in case a user accidentally passes in the wrong URL (e.g. The user sends in a URL that leads to a text file). However if load() already provides the fast failure feature, we could forgo the parameter sanitation all together.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2075: Add fromClassPath Method
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2075:
URL: https://github.com/apache/accumulo/pull/2075#discussion_r628412748
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/ClientInfoImpl.java
##########
@@ -115,6 +120,18 @@ public static Properties toProperties(Path propertiesFile) {
return properties;
}
+ @SuppressFBWarnings(value = "URLCONNECTION_SSRF_FD",
+ justification = "code runs in same security context as user who provided propertiesURL")
+ public static Properties toProperties(URL propertiesURL) {
+ Properties properties = new Properties();
+ try (InputStream is = propertiesURL.openStream()) {
Review comment:
@slackwinner wrote:
> Well I was thinking we can ensure the URL contains a properties extension before executing load() to achieve fast failure in case a user accidentally passes in the wrong URL (e.g. The user sends in a URL that leads to a text file). However if load() already provides the fast failure feature, we could forgo the parameter sanitation all together.
We have no requirement on the file's name. It could be `*.conf`, `*.properties`, `*.props`, `*.properties.backup`, `nofileextension`, or anything else. Such a check would only restrict user flexibility, but not actually achieve anything in terms of security.
@milleruntime wrote:
> Yes. But a URL _could_ be a lot of things and there is nothing preventing the user from opening a stream to "http://www.badguys.com/maliciousFile.exe".
Yeah, it could open a stream to that. But, a method that takes a Path or File could also open `/home/user/maliciousFile.exe`. It matters what we do with it. Merely reading from the stream won't automatically execute it. A well-crafted file could be used to exploit a parser vulnerability and execute code, if there were such a vulnerability, but that would apply whether it's a File or a URL, and would be the responsibility of the parser code to prevent. However, since this is client code, it executes in the same security context as the calling client user who provided the URL or File name. So, it would be nonsensical for such a user to employ Accumulo to execute this code, since the user could execute the malicious file directly if they wanted.
> This is what I thought the sec-bugs error was highlighting.
It wasn't. https://find-sec-bugs.github.io/bugs.htm#URLCONNECTION_SSRF_FD
This class of bugs requires the user and the code to be operating in different security contexts. Commonly, user input on a web form or REST endpoint causes a web server to read or execute code on the server-side that it shouldn't, or in some other security context that is separate from the user (such as within a "same origin" sandbox in a browser). None of those apply here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] slackwinner commented on a change in pull request #2075: Add fromClassPath Method
Posted by GitBox <gi...@apache.org>.
slackwinner commented on a change in pull request #2075:
URL: https://github.com/apache/accumulo/pull/2075#discussion_r627572041
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/ClientInfoImpl.java
##########
@@ -115,6 +120,18 @@ public static Properties toProperties(Path propertiesFile) {
return properties;
}
+ @SuppressFBWarnings(value = "URLCONNECTION_SSRF_FD",
+ justification = "code runs in same security context as user who provided propertiesURL")
+ public static Properties toProperties(URL propertiesURL) {
+ Properties properties = new Properties();
+ try (InputStream is = propertiesURL.openStream()) {
Review comment:
Good idea. I'll include a validation mechanism in my next commit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2075: Add fromClassPath Method
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2075:
URL: https://github.com/apache/accumulo/pull/2075#discussion_r627813996
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/ClientInfoImpl.java
##########
@@ -115,6 +120,18 @@ public static Properties toProperties(Path propertiesFile) {
return properties;
}
+ @SuppressFBWarnings(value = "URLCONNECTION_SSRF_FD",
+ justification = "code runs in same security context as user who provided propertiesURL")
+ public static Properties toProperties(URL propertiesURL) {
+ Properties properties = new Properties();
+ try (InputStream is = propertiesURL.openStream()) {
Review comment:
@milleruntime wrote:
> That's a good point. I was just thinking he could do something simple, like check to see if the URL ends with a valid extension but that will probably get vetted on `load()`.
There's no requirement that the file or URL have any particular naming convention.
> I told @slackwinner that it would be a good exercise to figure out how to sanitize a user parameter to prevent malicious code execution.
In general, that's good advice, but I don't think it applies here. What's the potential attack vector?
Are you thinking the file itself could contain something malicious? I don't think the properties parser executes any code it finds in the file... if it does, then that vulnerability would be in the parser, and would apply no matter where the file is read from, whether from a URL, a Path, a File, or even a String object.
Or, are you thinking that parsing the URL itself in order to locate the stream to read from could itself cause code to be executed? Because, then the vulnerability would be in Java's URL parsing code, and in any case that doesn't seem like a thing that can happen.
I think the spotbugs sec-bugs error highlights the biggest risk: since the user decides the URL to read from, they could instruct the code to read a file that the user themselves don't have permission to read, and then leak that data back to the user. However, that doesn't apply here, since the code executes in the same security context as the user. So, the code can only read files the user already has permission to read. This would be a valid concern if the URL was coming from some outside process or remote user, but the responsibility to sanitize that would be with the service that is built around Accumulo's API and receives that remote input. It's not a concern for our API.
I don't think there's anything to sanitize. What could the URL contain or link to that would necessitate sanitization?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org