Fixes #1038: Don't re-issue CSRF tokens when sessions are unchanged

[pmlopes]

No description provided.

[lukehutch]

I suggest eliminating code duplication:

        case GET:
            final String token;
            Session session = ctx.session();
            // if there's no session to store values, tokens are issued on every request
            boolean setCookie = true;
            if (session == null) {
                token = generateToken();
            } else {
                // get the token from the session
                String sessionToken = session.get(headerName);
                // when there's no token in the session, then we behave just like when there is no session
                // create a new token, but we also store it in the session for the next runs
                boolean setSessionVal = true;
                if (sessionToken == null) {
                    token = generateToken();
                } else {
                    // attempt to parse the value
                    int idx = sessionToken.indexOf('/');
                    if (idx != -1) {
                        String sid = sessionToken.substring(0, idx);
                        if (sid.equals(session.id())) {
                            // we're still on the same session, no need to regenerate the token
                            token = sessionToken.substring(idx + 1);
                            // in this case specifically we don't issue the token as it is unchanged
                            // the user agent still has it from the previous interaction.
                            setSessionVal = false;
                            setCookie = false;
                        } else {
                            // session has been upgraded, don't trust the token and regenerate
                            token = generateToken();
                        }
                    } else {
                        // cannot parse the value from the session
                        token = generateToken();
                    }
                }
                if (setSessionVal) {
                    // storing will include the session id too. The reason is that if a session is upgraded
                    // we don't want to allow the token to be valid anymore
                    session.put(headerName, session.id() + "/" + token);
                }
            }
            if (setCookie) {
                ctx.addCookie(Cookie.cookie(cookieName, token).setPath(cookiePath));
            }
            // put the token in the context for users who prefer to render the token directly on the HTML
            ctx.put(headerName, token);
            ctx.next();
            break;

otherwise, LGTM. Thanks for doing the work!

[pmlopes]

@lukehutch the cookies are still there, just being set on a different method, which I renamed later. Indeed I wasn't considering the session on the POST and that has been fixed.

I also, added tests with sessions and a test that shows that if you use sessions, tokens can only be used once (preventing replay attacks if the token would remain in the session).

[lukehutch]

Great job, I really appreciate your work on this!

[pmlopes]

Looks like you only checked 1 commit ;-) not the whole thing... sorry we usually don't squash (all history preserved, but hard to follow) :)

[lukehutch]

Weirdly GitHub kept showing me an old version of the code, but I clicked on the link again and can now see the latest version. Some sort of browser caching issue or something.

[runnermann]

Moved from #1038

Vulnerability review:

Viewing the master branch it appears that the PRNG is being used. With my
little knowledge in hacking other than from the recommendations from the
OWASP and NIST. I am limited to English and my only alternative language is
French and non technical. If there is a European equivalent to OWASP or
NIST it would be good to read their comments on hacking trends. PRNG from
my experience in a Mac OSX environment is not a good Random generator.
However the OWASP recommendation is untested on the same system.

Assumptions: 1) Host VERTx systems redirect their users to HTTPS. We are
assuming that attacks do not include exploiting a vulnerability in HTTPS.
2) Low latency and high efficiency are considered are valued to maintain
consistency with VERTx's history.

1. First attack approach. Log into the host system as a user as many times
   as possible and search for patterns and repeats in CSRF tokens. Then blast
   user systems with a mock set-up using the captured tokens and directives
   from any patterns detected. It would not work against all user/client
   systems, but if there is a pattern that can be detected or repeats found
   then eventually some users would become victims.

Risk: A small set of users would become victims of the attack.

Consequences: Potentially catastrophic depending on the application. IE if
a financial institution application or health care application.

Mitigation: Depending on the duration of the lifespan of the application,
shorten the lifespan of the CSRF token.

Likelihood: It seems this attack would be difficult to execute since
thousands of users would have to be accessed in order to be successful.

Opinion: Although the likelihood is low, the consequences are catastrophic
and mitigation allows VERTx to remain consistent with performance except in
extreme situations. ie 10,000 accesses per second. If VERTx is employed in
an HA environment then this should be sufficient to remove the risk that a
vertical is unable to handle the load.

-Lowell

[pmlopes]

@runnermann PRNG is a vertx auth class (that uses the jvm secure random + all recommendations for random generators from owasp), that does a couple of things:

• Generated secure random numbers (using secure random)
• Feeds the generator with entropy on given intervals (as recommend by owasp )
• Ensures that number of bits in entropy is high (owasp)
• Ensure blocking operations happen outside the eventloop for performance.

Docs here:

https://vertx.io/docs/vertx-auth-common/java/#_pseudo_random_number_generator

I still don't understand what you keep saying about owasp. Please look at the code and let me know which recommendation we are missing.

Or provide a showcase where it fails (which is the best option as it allows us to fix it and have a regression test).

[pmlopes]

The attack you described will fail. Even if you collect enough token to derive the seed of the secure random generator, you're missing the point that the PRNG object reseeds on random intervals, so you also need to guess those too, and the number of bits of entropy.

This also means that the token samples you have are useless as the seed isn't constant.

I'm not saying it's bulletproof, I'm saying that it's as safe as we can get following the safety guidelines from owasp.

Of course, it's software, there can be bugs and if that's the case I'll gladly try to fix it 👍

[runnermann]

Ok, that is good news. I looked at the PRNG class in io.vertx.ext.auth. And I am happy to say I was wrong. I cannot see how my suggestion would work. Lowell :)

…

On Wed, Nov 27, 2019 at 11:29 PM Paulo Lopes ***@***.***> wrote: The attack you described will fail. Even if you collect enough token to derive the seed of the secure random generator, you're missing the point that the PRNG object reseeds on random intervals, so you also need to guess those too, and the number of bits of entropy. This also means that the token samples you have are useless as the seed isn't constant. I'm not saying it's bulletproof, I'm saying that it's as safe as we can get following the safety guidelines from owasp. Of course, it's software, there can be bugs and if that's the case I'll gladly try to fix it 👍 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1465?email_source=notifications&email_token=AGC7W72K75CYSD4ABHSVOILQV5XMVA5CNFSM4JRWHYFKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFLWD4A#issuecomment-559374832>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGC7W7454P5EC2OPW7HGJLDQV5XMVANCNFSM4JRWHYFA> .