-
Notifications
You must be signed in to change notification settings - Fork 5k
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
Introduce logs in RestCsrfPreventionFilter to improve troubleshooting. #452
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,6 +31,10 @@ | |
import javax.servlet.http.HttpServletResponse; | ||
import javax.servlet.http.HttpSession; | ||
|
||
import org.apache.catalina.util.RequestUtil; | ||
import org.apache.juli.logging.Log; | ||
import org.apache.juli.logging.LogFactory; | ||
|
||
/** | ||
* Provides basic CSRF protection for REST APIs. The filter assumes that the | ||
* clients have adapted the transfer of the nonce through the 'X-CSRF-Token' | ||
|
@@ -78,7 +82,8 @@ public class RestCsrfPreventionFilter extends CsrfPreventionFilterBase { | |
private enum MethodType { | ||
NON_MODIFYING_METHOD, MODIFYING_METHOD | ||
} | ||
|
||
private final Log log = LogFactory.getLog(RestCsrfPreventionFilter.class); | ||
|
||
private static final Pattern NON_MODIFYING_METHODS_PATTERN = Pattern.compile("GET|HEAD|OPTIONS"); | ||
|
||
private Set<String> pathsAcceptingParams = new HashSet<>(); | ||
|
@@ -155,17 +160,29 @@ private class StateChangingRequest extends RestCsrfPreventionStrategy { | |
@Override | ||
public boolean apply(HttpServletRequest request, HttpServletResponse response) | ||
throws IOException { | ||
String nonceFromRequest = extractNonceFromRequest(request); | ||
HttpSession session = request.getSession(false); | ||
String nonceFromSession = extractNonceFromSession(session, | ||
Constants.CSRF_REST_NONCE_SESSION_ATTR_NAME); | ||
if (isValidStateChangingRequest( | ||
extractNonceFromRequest(request), | ||
extractNonceFromSession(request.getSession(false), | ||
Constants.CSRF_REST_NONCE_SESSION_ATTR_NAME))) { | ||
nonceFromRequest, | ||
nonceFromSession)) { | ||
return true; | ||
} | ||
|
||
storeNonceToResponse(response, Constants.CSRF_REST_NONCE_HEADER_NAME, | ||
Constants.CSRF_REST_NONCE_HEADER_REQUIRED_VALUE); | ||
response.sendError(getDenyStatus(), | ||
sm.getString("restCsrfPreventionFilter.invalidNonce")); | ||
if (log.isErrorEnabled()) { | ||
log.error("CSRF validation for REST failed! Request with method [" + request.getMethod() + "] and URI [" | ||
+ RequestUtil.filter(request.getRequestURI()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think there is a reason to use RequestUtil.filter() for this value; it's not going to be embedded in HTML so it's okay to use the value as-is. |
||
+ "] will be rejected. Details: request has sessionid [" | ||
+ (request.getRequestedSessionId() != null) | ||
+ "]; requested session exists [" + (session != null ) | ||
+ "]; crft nonce in request exists [" + (nonceFromRequest != null) | ||
+ "]; csrf nonce in session exists [" + (nonceFromSession != null) + "]."); | ||
} | ||
return false; | ||
} | ||
|
||
|
@@ -192,6 +209,10 @@ private String extractNonceFromRequestParams(HttpServletRequest request) { | |
String nonce = params[0]; | ||
for (String param : params) { | ||
if (!Objects.equals(param, nonce)) { | ||
if (log.isErrorEnabled()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same: use DEBUG not ERROR. |
||
log.error("Different CSRF nonces are sent as request paramaters, none of them will be used." | ||
+ "Request is with method: [" + request.getMethod() + "] and URI [" + RequestUtil.filter(request.getRequestURI()) + "]."); | ||
} | ||
return null; | ||
} | ||
} | ||
|
@@ -217,6 +238,10 @@ public boolean apply(HttpServletRequest request, HttpServletResponse response) { | |
} | ||
storeNonceToResponse(response, Constants.CSRF_REST_NONCE_HEADER_NAME, | ||
nonceFromSessionStr); | ||
if (log.isDebugEnabled()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I might use TRACE, here. |
||
log.debug("CSRF Fetch request is succesfully handled - nonce is added to the response." | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. succes |
||
+ "Request is with method: [" + request.getMethod() + "] and URI [" + RequestUtil.filter(request.getRequestURI() + "].")); | ||
} | ||
} | ||
return true; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this should be logged at ERROR level, because an attacker could fill-up your logs just by repeatedly sending-in garbage. I would recommend using DEBUG log-level for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @ChristopherSchultz !