Bug 65853

Sorry, my suggested implementation was a little rushed. Here is a corrected version.

####################
protected boolean getSkipNonceCheck(HttpServletRequest request) {
    if (!Constants.METHOD_GET.equals(request.getMethod()))
        return false;

    String reqPath = getRequestedPath(request);

    if (!entryPoints.contains(reqPath))
        return false;

    if (log.isTraceEnabled())
        log.trace("Skipping CSRF nonce-check for GET request to entry point " + reqPath);

    return true;
}
####################

doFilter()
####################
boolean skipNonceCheck = getSkipNonceCheck(req);
####################

I further suggest to either add HttpServletRequest argument to the generateNonce() method or add another method like getSkipNonceGeneration(HttpServletRequest request) or getGenerateNonce(HttpServletRequest request), what ever you prefer.

Background is, that you may need to override behavior, when to actually generate a nonce for a given request.

The call would look like this.

#######################
String newNonce = generateNonce();

if (newNonce != null) {
    nonceCache.add(newNonce);

    // Take this request's nonce and put it into the request
    // attributes so pages can make direct use of it, rather than
    // requiring the use of response.encodeURL.
    request.setAttribute(Constants.CSRF_NONCE_REQUEST_ATTR_NAME, newNonce);

    wResponse = new CsrfResponseWrapper(res, nonceRequestParameterName, newNonce);
} else {
    wResponse = response;
}
#######################

Sorry, of course like this.

String newNonce = generateNonce(req);
...

I have further suggestions for this Filter. To add a little flexibility for the Nonce cache and for the storage of it I suggest construction methods like this:

#################################
protected static interface NonceCache<T> extends Serializable {
    void add(T nonce);

    boolean contains(T nonce);
}

protected static class LruCache<T> implements NonceCache<T> {
#################################

#################################
protected NonceCache<String> getNonceCache(@SuppressWarnings( "unused" ) HttpServletRequest request, HttpSession session) {
    return (NonceCache<String>) session.getAttribute(Constants.CSRF_NONCE_SESSION_ATTR_NAME);
}

protected NonceCache<String> newNonceCache(@SuppressWarnings( "unused" ) HttpServletRequest request, HttpSession session) {
        LruCache<String> nonceCache = new LruCache<>(nonceCacheSize);
    session.setAttribute(Constants.CSRF_NONCE_SESSION_ATTR_NAME, nonceCache);

    return nonceCache;
}
#################################


Called like this:

#################################
NonceCache<String> nonceCache = (session == null) ? null : getNonceCache(req, session);
#################################

#################################
if (nonceCache == null) {
    if(log.getLogger().getLevel().isAsVerboseAs(LogLevel.DEBUG)) {
        log.logDebug("Creating new CSRF nonce cache with size=" + nonceCacheSize + " for session " + (null == session ? "(will create)" : session.getId()));
    }

    if (session == null) {
        if(log.getLogger().getLevel().isAsVerboseAs(LogLevel.DEBUG)) {
             log.logDebug("Creating new session to store CSRF nonce cache");
        }

        session = req.getSession(true);
    }

    nonceCache = newNonceCache(req, session);
}
#################################

None cache creation is moved below session "creation" to be able to pass it into the call of newNonceCache(req, session).

Any thoughts here?

How about creating a PR or patch?

Fixed in:
- 10.1.x for 10.1.0-M15 onwards
- 10.0.x for 10.0.21 onwards
- 9.0.x for 9.0.63 onwards
- 8.5.x for 8.5.79 onwards

This is wonderful. Thank you very much for using my implementation.

I just have a very little annotation. Would you mind, adding a null check for the result of generatNonce(req)? If the value is null, the newNonce must not be added to the cache. With this the class is perfect.

Returning null there feels more like an error rather than something that should effectively be ignored. What is the use case for returning null?

You're right. It's easy to miss a detail in the wood of these extension suggestions. And in our current implementation we even use a base method like this

####################
protected boolean getSkipNonceGeneration(@SuppressWarnings( "unused" ) HttpServletRequest request) {
    return false;
}
####################

to decide, whether to create and handle a nonce depending on the request.

Our overriding implementation checks, if the current request goes to a page, that we consider an entry point, which must not be part of the nonce chain, well, at least nothing other than the (new) start.

In the doFilter() method this oule look as follows.

#########################
if (!getSkipNonceGeneration(req)) {
    String newNonce = generateNonce();

    nonceCache.add(newNonce);

    // Take this request's nonce and put it into the request
    // attributes so pages can make direct use of it, rather than
    // requiring the use of response.encodeURL.
    request.setAttribute(Constants.CSRF_NONCE_REQUEST_ATTR_NAME, newNonce);
    //System.out.println( "new nonce: " + newNonce );

    wResponse = new CsrfResponseWrapper(res, nonceRequestParameterName, newNonce);
} else {
    wResponse = response;
}
#########################

Are you asking for an additional extension point, skipNonceGeneration(HttpServletRequest) to be added? That looks reasonable to me.

Exactly.

Done

Wonderful. It is perfect now. Thank you so much!

I have now integrated your new version of CsrfPreventionFilter as base to our filter class. And it tuned out, that there are some minor things missing, that will prevent us from using the class as it is now.

For one there is no need to call getNonceCache(req), if both skipNonceCheck(req) and skipNonceGeneration(req) return true. For us is is actually malicious, because in these cases a new cache instance is created, that hurts later. I suggest to skip the block 166 to 180, if both extension points return true.

And much less minor: If skipNonceGeneration(req) is true, wRequest remains null and is later passed into chain.doFilter(request, wRequest). It must fallback to response in this case.

Maybe it wouldn't hurt to change line 204 like this:

chain.doFilter(request, wResponse != null ? wResponse : response);

Or, if you have some standard ifNull() of ours, use that.

Then of course current 200-201 could be dropped.

(In reply to Marvin Fröhlich from comment #15)
> For one there is no need to call getNonceCache(req), if both
> skipNonceCheck(req) and skipNonceGeneration(req) return true. For us is is
> actually malicious, because in these cases a new cache instance is created,
> that hurts later. I suggest to skip the block 166 to 180, if both extension
> points return true.

Do you mean there is no need to call createNoneCache() since that is what happens in lines 166-180?
I think it is sufficient to make the test at line 166
if (nonceCache == null && !skipNonceGeneration(req))

If skipNonceCheck(req) is false and nonceCache is null the method will have already exited so there is no need to check it at line 166.

> And much less minor: If skipNonceGeneration(req) is true, wRequest remains
> null and is later passed into chain.doFilter(request, wRequest). It must
> fallback to response in this case.
> 
> Maybe it wouldn't hurt to change line 204 like this:
> 
> chain.doFilter(request, wResponse != null ? wResponse : response);

Done.

(In reply to Mark Thomas from comment #16)
> Done.

Thanks a lot.

(In reply to Mark Thomas from comment #16)
> Do you mean there is no need to call createNoneCache() since that is what
> happens in lines 166-180?
> I think it is sufficient to make the test at line 166
> if (nonceCache == null && !skipNonceGeneration(req))
> 
> If skipNonceCheck(req) is false and nonceCache is null the method will have
> already exited so there is no need to check it at line 166.

I see, I wasn't clear enough.

I suggest this code for current 123-125:
##########################
boolean skipNonceCheck = skipNonceCheck(req);
boolean skipNonceGeneration = skipNonceGeneration(req);

NonceCache<String> nonceCache = ((session == null) || (skipNonceCheck && skipNonceGeneration)) ? null : getNonceCache(req, session);

if (!skipNonceCheck) {
##########################

This way the call to getNonceCache(req, session) is skipped, if both skipNonceCheck and skipNonceGeneration are true (or no session is available) and hence nonce handling is skipped at all for this request.

This is, what I meant by "there's no neede to call getNonceCache(req)".

You're right with your suggestion about 166.

So in your code the call to getNonceCache() will create a cache instance if none is found? That doesn't seem quite right. I'd expect that method to return null if the cache doesn't exist rather than the create a new instance.

I've refactored things a bit more so getNonceCache() is only called when necessary. Let me know what you think.

(In reply to Mark Thomas from comment #18)
> So in your code the call to getNonceCache() will create a cache instance if
> none is found? That doesn't seem quite right. I'd expect that method to
> return null if the cache doesn't exist rather than the create a new instance.

Well, the reason for many of the extensions is, that we need to distinguish between window contexts. The session is the same, but the request might come from another window (popup). Without this distinction the nonce chain will get broken once a popup is opened for a session. And this needs special treatment (separate nonce caches). Actually I think, this feature is missing in your implementation.

(In reply to Mark Thomas from comment #18)
> I've refactored things a bit more so getNonceCache() is only called when
> necessary. Let me know what you think.

Yes, this looks fine. Thanks.

The other reason for extension is to have more flexibility (wildcard support) in entry point handling.