[keycloak-dev] Issue with concurrent SSO login to same client in more browser tabs

[Stian Thorgersen]

On Mon, 26 Nov 2018, 09:04 Marek Posolda <mposolda at redhat.com wrote:

> On 26/11/2018 08:54, Stian Thorgersen wrote:
>
> The ideal may be to create a code token. If it's less than 2000 characters
> just send it as is. If it's more than we put it in the actionTokens cache
> and just send a reference to it in the code param.
>
> I see. So it will be something like "hybrid" between both approaches. Was
> also a bit thinking about it, but didn't in the end. ATM it is always
> adding the item to actionTokens cache. Maybe it's possible to optimize it
> and add the "hybrid" possibility, but not sure about it at this stage?
> Maybe after new year?
>

+1 for the future. I guess one issue would be the single use aspect.

Not sure if the reference should just be a uuid, or if it should be
> something more to make sure it can't be guessed. The last point is also
> relevant to reference tokens once/if we add support for that.
>
> ATM it is <code-uuid>.<user-session-id>.<client-uuid>
>
> The userSessionId and clientUUID are needed, so if code was already used
> (and is removed from actionTokens cache), it is still possible to lookup
> userSession and remove clientSession from it as OIDC specs suggests (in
> case that attempt of duplicate code use was detected).
>
> Not sure how big probability is to guess UUID for the very short-lived
> code. It seems to me like kind of impossible thing. And if it's really
> guessed, then when valid user will try to exchange the code, which attacker
> already exchanged, the "code" will be expired (client removed from
> userSession as pointed above).
>
uuid should be sufficiently secure, but there's a potential here for a
brute force attack of sorts though.

Marek
>
>
> On Mon, 19 Nov 2018 at 11:49, Marek Posolda <mposolda at redhat.com> wrote:
>
>> Hi,
>>
>> I've sent PR https://github.com/keycloak/keycloak/pull/5736 to fix the
>> issue in subject (corresponding JIRAs are KEYCLOAK-7774 KEYCLOAK-8438).
>> The cause is that once the user is authenticated in the browser and has
>> userSession, there is just 1 clientSession per the userSession+client.
>> So in case of concurrent SSO login in more browser tabs, there can be an
>> issue that each tab is writing it's state to the same clientSession,
>> which causes conflicts (especially attributes like nonce, scope or
>> redirectUri can be possibly different for each tab and shouldn't be
>> shared).
>>
>> The possibility can be to revert back when we had more clientSessions
>> per userSession+client. I think this is step back and has lots of other
>> issues (memory consumption, more cluster and cross-dc writes, worse
>> performance in general...). For the future, it will be rather ideal to
>> remove clientSession entirely or reduce their size even more, as they
>> will be needed for logout, but hopefully for nothing more.
>>
>> So the other possibility is to move most of the state to the "code"
>> parameter and to the refreshToken. Each browser tab has it's own "code"
>> and it's own refreshToken. Problem with the "code" is, that it's used as
>> query parameter in the URL and hence has limits for it's size. It seems
>> ideal is still to stick with 2000 characters per URL. And when
>> redirectUri itself will be part of the code, it can be problematic as we
>> don't know how the redirectUri provided by users will be big....
>> Fortunately refreshToken doesn't have this issue as it needs to be
>> always sent in the POST request.
>>
>> So to solve the issue for "code", I've added an entry to the infinispan
>> cache "actionTokens", which encapsulates needed state and it's supposed
>> to be valid just for the very short time (between the
>> AuthenticationResponse to the application and the time when application
>> sends the code-to-token request) and it's lifespan corresponds to realm
>> code lifespan (1 minute by default). In the code-to-token request, there
>> is a call to "cache.remove" to remove the code. Infinispan caches
>> (local, distributed and hotrod caches) fortunately guarantee that
>> "remove(123)" is always successful just in 1 thread (Successful means
>> that it returns previous state). So we still have guarantee for the
>> single-use code.
>>
>> Marek
>>
>> _______________________________________________
>> keycloak-dev mailing list
>> keycloak-dev at lists.jboss.org
>> https://lists.jboss.org/mailman/listinfo/keycloak-dev
>>
>
>