[doutatsu]

A flush does not always end a session. If a refresh of the same session runs at the same time as the flush, the refresh writes part of the refresh token again.

Why this is a problem

Apps flush a session to end it: on sign-out, on "sign out of all devices", or after a password change. They do this when they think that someone else has the token.

• The attacker keeps access. An attacker with a stolen token can refresh again and again. If one of these refreshes runs during the flush, it returns a new access token. The app thinks that the session is gone, but the attacker can use this access token until it expires (1 hour by default).
• With the memory store, the flush has no effect. The refresh stores the refresh token again, without an expiration. Thus the flushed session can refresh again.
• With the Redis store, keys leak. Each race leaves a key that never expires, and no flush can find it. On Redis with noeviction, these keys use memory until Redis is full. With an eviction policy, Redis can remove keys that are still in use to make space.
• The leak does not need an attacker. A normal sign-out in one tab while another tab refreshes is sufficient. Thus the number of leaked keys increases in normal use.

How it happens

Session#refresh and #refresh_by_access_payload do these steps:

1. Read the refresh token.
2. Create a new access token.
3. Update the refresh token with RefreshToken#update.

If a flush removes the refresh token between steps 1 and 3:

• Redis store: update_refresh runs HMSET on the removed key. HMSET creates the key again with only csrf, access_expiration and access_uid, and with no TTL. fetch_refresh ignores a key without expiration, so flush_namespaced and flush_all do not see this key.
• Memory store: update_refresh_fields merges the fields into {} and stores the record again, without an expiration.

Reproduction against 3.2.4 with the Redis store:

tokens = JWTSessions::Session.new(namespace: "ns", payload: { user_id: 1 }, refresh_by_access_allowed: true).login
payload = JWTSessions::Token.decode(tokens[:access]).first

# A flush that runs between the read and the update
JWTSessions::RefreshToken.prepend(Module.new do
  def update(*args)
    JWTSessions::Session.new(namespace: "ns").flush_namespaced
    super
  end
end)

JWTSessions::Session.new(namespace: "ns", payload: payload, refresh_by_access_allowed: true).refresh_by_access_payload
# => returns a new access token, and "jwt__ns_refresh_<uid>" has 3 fields and a TTL of -1

Fix

• update_refresh writes only while the refresh token exists. Otherwise, it returns false. The Redis store does the check and the write in one Lua script, so no other command can run between them.
• persist_refresh now writes all four fields itself. Before, it used update_refresh.
• If update_refresh returns false, Session removes the access token that it just created and raises Unauthorized ("Refresh token not found").
• This change also stops flush_namespaced_access_tokens from writing a token again after another flush removed it.

Only an explicit false stops the refresh. A custom store adapter whose update_refresh returns nil or "OK" continues to work.

Tests

• New tests: update_refresh after destroy_refresh for both stores, and a refresh that races a flush in TestSession. All three fail without the fix.
• I ran the CI steps from main.yml in Docker (Ruby 3.3, Rails 7.0). The unit tests and the Sinatra specs pass on both stores.

The CI on this PR fails for reasons that also apply to main:

• TestRefreshToken depends on the test order. Its setup calls Session.flush_all before it sets a signing key. It fails when earlier tests left refresh tokens and also removed the key. On main, 12 of 60 seeds fail with the same 4 errors.
• The Rails 6.1 job stops when Rails loads, because Rails 6.1 does not work with the current concurrent-ruby.
• The Rails dummy specs stop at config.fixture_path, because rspec-rails 7 removed it. With fixture_paths, they pass on both stores, with and without this change.

[tuwukee]

Thank you! I need to look into CI, it needs some maintenance, will merge once it's green....