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:
[email protected]
