Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add File Disclosure Injection detector #265

Merged
merged 2 commits into from Jan 13, 2017

Conversation

plr0man
Copy link
Member

@plr0man plr0man commented Jan 12, 2017

Proposed detector reports code utilizing ActionForward (Struts) or ModelAndView (Spring) to construct a server-side redirect path with user input. This may allow an attacker to view sensitive files or download application binaries.
Vulnerable code sample:

... 
String returnURL = request.getParameter("returnURL");
return new ModelAndView(returnURL); 

or

... 
String returnURL = request.getParameter("returnURL"); 
Return new ActionForward(returnURL);

@h3xstream h3xstream added the enhancement New feature or improvement to existing detector. label Jan 13, 2017
@h3xstream h3xstream self-assigned this Jan 13, 2017
@h3xstream h3xstream self-requested a review January 13, 2017 15:48
@h3xstream h3xstream removed their assignment Jan 13, 2017
@h3xstream h3xstream added this to the version-1.6.0 milestone Jan 13, 2017
@h3xstream h3xstream merged commit 4c0d12a into find-sec-bugs:master Jan 13, 2017
@plr0man plr0man deleted the fileDisclosure branch January 13, 2017 16:33
@ptamarit
Copy link
Contributor

ptamarit commented Jan 13, 2017

I think that the following Spring snippet does the same risky redirects (returning a String instead of a ModelAndView:

@GetMapping
public String index(...) {
    // ...
    return "redirect:" + request.getParameter("returnURL");
}

See:

I'm afraid that writing a detector for this case would be trickier though...

@plr0man
Copy link
Member Author

plr0man commented Jan 13, 2017

Nice one Pablo! We'll have to pave the way for more of those. Do you want to add it to Issues so we don't loose it?

@ptamarit
Copy link
Contributor

@plr0man: Done, I created #269.

<![CDATA[
<p>
Constructing a server-side redirect path with user input could allow an attacker to download application binaries (including application classes or jar files) or view arbitrary files within protected directories.<br/>
An attacker may be able to forge a request parameter to match sensitive file locations. For example, requesting "http://example.com/?returnURL=WEB-INF/applicationContext.xml" would display the application's applicationContext.xml file. The attacker would be able to locate and download the applicationContext.xml referenced in the other configuration files, and even class files or jar files, obtaining sensitive information and launching other types of attacks.
Copy link

@php-coder php-coder Apr 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example, requesting "http://example.com/?returnURL=WEB-INF/applicationContext.xml" would display the application's applicationContext.xml file.

I believe that this example is wrong and user will always get 404 or 500 error. To do a server-side redirect you need:

  1. use forward method that isn't used by default. Hence, the parameter value should start from forward:
  2. to get something in the WEB-INF dir, you need to add a slash to the beginning and only then this will be interpreted as a classpath resource.
  3. now it still won't work because this string is passed to ViewResolver that most of the time configured to search in some directory and files that have particular suffix. Most likely you will be able to get JSP/HTML files only.
  4. even if ViewResolver is configured to serve XML files, in modern Spring web apps (for example, Spring Boot) applicationContext.xml is absent at all because all the configuration is done in java classes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or improvement to existing detector.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants