Details
-
Type:
Improvement
-
Status: Closed
-
Priority:
Major
-
Resolution: Fixed
-
Affects Version/s: 3.2
-
Fix Version/s: EE-3.2.0.GA, 3.3
-
Component/s: Framework, ICE-Components
-
Labels:None
-
Environment:Security
-
Assignee Priority:P2
Description
This is a specific case opened up as part of a detailed analysis (ICE-8771) of a Veracode security report submitted by a customer.
The reported issue was: "External Control of File Name or Path"
The details provided by Veracode were:
This call to java.lang.ClassLoader.getResourceAsStream() contains a path manipulation flaw. The argument to the function is a filename constructed using user-supplied input. If an attacker is allowed to specify all or part of the filename, it may be possible to gain unauthorized access to files on the server, including those outside the webroot, that would be normally be inaccessible to end users. The level of exposure depends on the effectiveness of input validation routines, if any. The first argument to getResourceAsStream() contains tainted data from the variable path. The tainted data originated from an earlier call to javax.servlet.http.HttpServletRequest.getPathInfo. Validate all user-supplied input to ensure that it conforms to the expected format, using centralized data validation routines when possible. When using black lists, be sure that the sanitizing routine performs a sufficient number of iterations to remove all instances of disallowed characters.
The relevant class is:
com.icesoft.faces.webapp.CompatResourceServlet
void service(javax.servlet.http.HttpServletRequest, javax.servlet.http.HttpServletResponse)
The task is to review the and attempt to ensure, if possible, that input from the user is sanitized before being used to generate path names.
The reported issue was: "External Control of File Name or Path"
The details provided by Veracode were:
This call to java.lang.ClassLoader.getResourceAsStream() contains a path manipulation flaw. The argument to the function is a filename constructed using user-supplied input. If an attacker is allowed to specify all or part of the filename, it may be possible to gain unauthorized access to files on the server, including those outside the webroot, that would be normally be inaccessible to end users. The level of exposure depends on the effectiveness of input validation routines, if any. The first argument to getResourceAsStream() contains tainted data from the variable path. The tainted data originated from an earlier call to javax.servlet.http.HttpServletRequest.getPathInfo. Validate all user-supplied input to ensure that it conforms to the expected format, using centralized data validation routines when possible. When using black lists, be sure that the sanitizing routine performs a sufficient number of iterations to remove all instances of disallowed characters.
The relevant class is:
com.icesoft.faces.webapp.CompatResourceServlet
void service(javax.servlet.http.HttpServletRequest, javax.servlet.http.HttpServletResponse)
The task is to review the and attempt to ensure, if possible, that input from the user is sanitized before being used to generate path names.
Issue Links
- blocks
-
ICE-8771
SECURITY: Potential security improvements related to findings from Veracode security scan
-
- Closed
-
Activity
| Status | Resolved [ 5 ] | Closed [ 6 ] |
Hello Deryk,
This fix produce Null pointer exception on URL's like "http://host/url-pattern", "http://host/url-pattern/". On this url's getPathInfo() method return null,
and throw null pointer exception here "(path.contains("..")) {".
Thanks.
Show
Kairat Rakhimov
added a comment - - edited Hello Deryk,
This fix produce Null pointer exception on URL's like "http://host/url-pattern", "http://host/url-pattern/". On this url's getPathInfo() method return null,
and throw null pointer exception here "(path.contains("..")) {".
Thanks.
| Security | Private [ 10001 ] |
| Repository | Revision | Date | User | Message |
| ICEsoft Public SVN Repository | #33061 | Tue Jan 15 10:18:00 MST 2013 | deryk.sinotte | |
| Files Changed | ||||
MODIFY
/icefaces3/trunk/icefaces/core/src/main/java/org/icefaces/util/EnvUtils.java
MODIFY
/icefaces3/trunk/icefaces/compat/components/src/main/java/com/icesoft/faces/component/util/CustomComponentUtils.java
|
| Status | Open [ 1 ] | Resolved [ 5 ] |
| Resolution | Fixed [ 1 ] |
The CompatResourceServlet has this code which the Verascan picks up a potentially dangerous:
...
private static final String BASE_PATH = "com/icesoft/faces/resources";
...
String path = httpServletRequest.getPathInfo();
final InputStream in = loader.getResourceAsStream(BASE_PATH + path);
...
The challenge is to "sanitize" the path variable so that getResourceAsStream isn't allowed to return anything it isn't supposed to. The JavaDoc for getPathInfo():
"Returns any extra path information associated with the URL the client sent when it made this request. The extra path information follows the servlet path but precedes the query string and will start with a "/" character."
All I can think of being a problem here is someone crafting the request so that the path ends up containing some directory manipulation (e.g. "/../../../..") that might allow access to something outside of what the BASE_PATH sets. The method isn't going to execute anything like a script.
Consulted with Ted and this appears to be very low risk. We could just reject any strings containing ".." and log a security warning. I suspect that there is no security hole here, but rejecting ".." would be the safest thing to do.
So I've added a check to the path value that will check for paths with ".." in them and if it detects one:
- log a security warning
- return a 404
Not sure whether this will satisfy the Veracode scanning or not but should satisfy the spirit of the issue.
Show
Deryk Sinotte
added a comment - The CompatResourceServlet has this code which the Verascan picks up a potentially dangerous:
...
private static final String BASE_PATH = "com/icesoft/faces/resources" ;
...
String path = httpServletRequest.getPathInfo();
final InputStream in = loader.getResourceAsStream(BASE_PATH + path);
...
The challenge is to "sanitize" the path variable so that getResourceAsStream isn't allowed to return anything it isn't supposed to. The JavaDoc for getPathInfo():
"Returns any extra path information associated with the URL the client sent when it made this request. The extra path information follows the servlet path but precedes the query string and will start with a "/" character."
All I can think of being a problem here is someone crafting the request so that the path ends up containing some directory manipulation (e.g. "/../../../..") that might allow access to something outside of what the BASE_PATH sets. The method isn't going to execute anything like a script.
Consulted with Ted and this appears to be very low risk. We could just reject any strings containing ".." and log a security warning. I suspect that there is no security hole here, but rejecting ".." would be the safest thing to do.
So I've added a check to the path value that will check for paths with ".." in them and if it detects one:
log a security warning
return a 404
Not sure whether this will satisfy the Veracode scanning or not but should satisfy the spirit of the issue.
| Repository | Revision | Date | User | Message |
| ICEsoft Public SVN Repository | #33054 | Mon Jan 14 12:59:31 MST 2013 | deryk.sinotte | |
| Files Changed | ||||
|
MODIFY
/icefaces3/trunk/icefaces/compat/core/src/main/java/com/icesoft/faces/webapp/CompatResourceServlet.java
|
| Assignee Priority | P2 [ 10011 ] |
| Summary | External Control of File Name or Path | SECURITY: External Control of File Name or Path |
| Field | Original Value | New Value |
|---|---|---|
| Summary | Placeholder issue | External Control of File Name or Path |
| Issue Type | Bug [ 1 ] | Improvement [ 4 ] |
| Assignee | Deryk Sinotte [ deryk.sinotte ] | |
| Fix Version/s | EE-3.2.0.GA [ 10332 ] | |
| Fix Version/s | 3.3 [ 10370 ] | |
| Reporter | Migration [ remote ] | Deryk Sinotte [ deryk.sinotte ] |
| Affects Version/s | 3.2 [ 10338 ] | |
| Environment | Test | Security |
| Description | Placeholder issue |
This is a specific case opened up as part of a detailed analysis ( The reported issue was: "External Control of File Name or Path" The details provided by Veracode were: This call to java.lang.ClassLoader.getResourceAsStream() contains a path manipulation flaw. The argument to the function is a filename constructed using user-supplied input. If an attacker is allowed to specify all or part of the filename, it may be possible to gain unauthorized access to files on the server, including those outside the webroot, that would be normally be inaccessible to end users. The level of exposure depends on the effectiveness of input validation routines, if any. The first argument to getResourceAsStream() contains tainted data from the variable path. The tainted data originated from an earlier call to javax.servlet.http.HttpServletRequest.getPathInfo. Validate all user-supplied input to ensure that it conforms to the expected format, using centralized data validation routines when possible. When using black lists, be sure that the sanitizing routine performs a sufficient number of iterations to remove all instances of disallowed characters. The relevant class is: com.icesoft.faces.webapp.CompatResourceServlet void service(javax.servlet.http.HttpServletRequest, javax.servlet.http.HttpServletResponse) The task is to review the and attempt to ensure, if possible, that input from the user is sanitized before being used to generate path names. |
| Component/s | Components [ 10012 ] | |
| Component/s | Framework [ 10013 ] |
Thanks for catching this. Since 3.2 is no longer under active development, I've opened a new JIRA (
ICE-9942) targeted for the next release and applied a patch.