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