find-sec-bugs & redirect warnings
A colleague asked me why is SonarQube complaining about HttpServletResponse redirects so much. It seems to come from FindSecBugs plugin for FindBugs. In the description Unvalidated Redirect) the explanation is the following:
Unvalidated redirects occur when an application redirects a user to a destination URL specified by a user supplied parameter that is not validated. Such vulnerabilities can be used to facilitate phishing attacks.
This is the famous “Unvalidated Redirects and Forwards” OWASP Top 10 vulnerability.
But the issue is that the code in mention was making a simple redirect to a path defined in class level a constant – no variables were ever added to the URL. Definitely a false-positive. The question was: how to re-write the code for Sonar to stop complaining?
|
1 |
response.sendRedirect(LOGIN_URL); |
First thing to do: check the proposed solutions.
Solution/Countermeasures:
– Don’t accept redirection destinations from users
– Accept a destination key, and use it to look up the target (legal) destination
– Accept only relative paths
– White list URLs (if possible)
– Validate that the beginning of the URL is part of a white list
I realized some of the solutions (like white-listing) are a bit too much for a static analyzer to check. The following crossed my mind: security issues are too important so maybe the creators of find-sec-bugs have decided to mark ALL redirects as vulnerabilities in order for the code author to check all of them with care and mark the false positives.
Next step: Check the implementation, which in our case is class find-sec-bugs/plugin/src/main/java/com/h3xstream/findsecbugs/injection/redirect/RedirectionSource.java
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 |
public class RedirectionSource implements InjectionSource { private static final String UNVALIDATED_REDIRECT_TYPE = "UNVALIDATED_REDIRECT"; @Override public InjectionPoint getInjectableParameters(InvokeInstruction ins, ConstantPoolGen cpg, InstructionHandle insHandle) { if (ins instanceof INVOKEINTERFACE) { String methodName = ins.getMethodName(cpg); String className = ins.getReferenceType(cpg).toString(); if (className.equals("javax.servlet.http.HttpServletResponse") || className.equals("javax.servlet.http.HttpServletResponseWrapper")) { if (methodName.equals("sendRedirect")) { InjectionPoint ip = new InjectionPoint(new int[]{0}, UNVALIDATED_REDIRECT_TYPE); ip.setInjectableMethod(className.concat(".sendRedirect(...)")); return ip; } else if (methodName.equals("addHeader") || methodName.equals("setHeader")) { LDC ldc = ByteCode.getPrevInstruction(insHandle, LDC.class); if (ldc != null) { Object value = ldc.getValue(cpg); if (value != null && "Location".equalsIgnoreCase((String) value)) { InjectionPoint ip = new InjectionPoint(new int[]{0}, UNVALIDATED_REDIRECT_TYPE); ip.setInjectableMethod(className + "." + methodName + "(\"Location\", ...)"); return ip; } } } } } return InjectionPoint.NONE; } } |
My guess was correct! All redirects are marked as vulnerabilities, as well as all additions of “Location” headers.
So only option for now – manual ignore with comment.
It seems there is a long-going discussion on the topic in their issue tracker: “findsecbugs:UNVALIDATED_REDIRECT and context path”
