[CVE-2020-1732] Adjust JASPIC integration to create a new ServerAuthModule for each request.

[darranl]

No description provided.

[arjantijms]

@darranl Are you sure this is really a CVE?

We discussed exactly this case back then in the EG, as I too was afraid of exactly this possibility.

However, it was assured (and backed by the JASPIC spec on which we layer) that the SAM and specifically the CallBackHandler is reusable between threads, or put differently, that it should not hold state as instance variables. As a matter of fact, I even tested this on WildFly back then (must have been WF 10 I think).

Did something change with Elytron that the CallBackHandler is now holding on to instance state?

[darranl]

I see from the spec that an integration of JASPIC could choose to use a single CallbackHandler what I don't see from the spec is anything requiring it to do so.

The WildFly Elytron CallbackHandler is responsible for creation of the identity being established so holds the state for the present request.

So despite calling AuthConfigProvider.getConfigProvider to ensure our desired CallbackHandler is set the singleton ServerAuthModule then is assigned the CallbackHandler instance from multiple threads, potentially swapping out the CallbackHandler mid request processing.

[darranl]

One change I could make to this PR is to cache the SAM instance in the AuthConfigProvider instance, we already know that Soteria does not make use of any of the other parameters passed in on initialisation so this would ensure the 1:1 relationship between Callbackhandler and ServerAuthModule whilst as long as an AuthConfigProvider is reused the single SAM with be used.

[arjantijms]

@darranl

I see from the spec that an integration of JASPIC could choose to use a single CallbackHandler what I don't see from the spec is anything requiring it to do so.

I'm quoting some parts from the discussion we had about this back then:

The spec says

3.7.1 Authentication Context Identifiers

This profile does NOT impose any profile specific requirements on authentication context   identifiers. 
As defined in Section 2.1.3, “Acquire AuthContext Identifier,” on page 17, the authentication
context identifier used in the call to getAuthContext must be equivalent to the value that would 
be acquired by calling getAuthContextID with the MessageInfo that will be used in the call to
validateRequest.

Moreover, the call to getAuthContext doesn't have to be made on every request; although it is typically simpler to make the call, than
to figure out if none of the arguments to the call have changed.

Once an authentication context is acquired, it may be reused to process subsequent requests
of the application for which an equivalent authentication context identifier, Subject, and 
properties Map (as used in the getAuthContext) applies. Runtimes that wish to be dynamic 
with respect to changes in context configuration should call getAuthContext for every request.

FWIW, this wording was chosen to make it possible minimize the performance impact of the 196 spi calls.

Followed up by:

So an authentication context may be reused, but then the runtime that does 
this reusing somehow has to pass the new CallbackHandler instance in for a
specific request and make sure that same authentication context instance is
not used for concurrent requests?

Or is the CallbackHandler supposed to be thread-safe and reusable itself by
multiple (concurrent or not) requests?

yes the callback handler is expected to be thread safe. I don't know if that is ever stated as a requirement, but it must be the case, unless the encompassing container knows it is somehow serializing its calls to validateRequest, such that the CBH it gives to a module will never be concurrently invoked.

[arjantijms]

Some more relevant comments:

"Servlets typically run on multithreaded servers, so be aware that a servlet must handle
concurrent requests and be careful to synchronize access to shared resources. Shared
resources include in-memory data such as instance or class variables and external objects
such as files, database connections, and network connections"

In case of Servlets it's well known that a single instance of a Servlet services multiple clients (threads) and therefor instance variables have to be thread-safe. But in the case of a server auth module it's not suggested that a single class instance of that should be reused for multiple requests, right?

no, a single server auth module does need to be able to process concurrent requests.

[arjantijms]

That all said, it's probably a good idea to make Soteria friendly to runtimes that use callbackhandlers that are not thread-safe, but I think that this should technically not strictly be needed given the above.

I think there's generally at least two options to implement the callbackhandler; use thread locals to store the collected data, or just store everything in the subject that's passed in.

For the latter, that's how we did it for Elios (standalone jaspic implementation based on GF):

https://github.com/omnifaces/eleos/blob/master/src/main/java/org/omnifaces/eleos/config/helper/BaseCallbackHandler.java#L40

[darranl]

As I say the gap in the calls is the server integration is choosing to request a new ServerAuthConfig instance so it can provide a new CallbackHandler instance into the new ServerAuthConfig.

I can not find a single reference that suggests multiple calls to AuthConfigProvider.getServerAuthConfig() require the same CallbackHandler and I can not find anything that would suggest it is acceptable that a later call to getServerAuthConfig would replace the CallbackHandler in a ServerAuthConfig created previously.

IMO this is the most relevent quote in section 3.5 of the specification within the Serlvet Profile definition: -

The CallbackHandler passed to ServerAuthModule.initialize is determined by the handler argument
passed in the AuthConfigProvider.getServerAuthConfig call that acquired the corresponding authentication
context configuration object. The handler argument must not be null, and the argument handler and the
CallbackHandler passed to ServerAuthModule.initialize must support the following callbacks

By creating a new ServerAuthConfig I am expecting this statement to be honoured, by not honouring this the ServerAuthModule is using a CallbackHandler passed in to other calls to AuthConfigProvider.getServerAuthConfig resulting in requests randomly manipulating the CallbackHandlers of other requests.

Regarding the rest of the comments on state, yes I agree once the ServerAuthConfig is obtained instances can be cached to prevent the need to recreate them but with the requirement that the ServerAuthModule is initialised using the CallbackHandler used to obtain the ServerAuthConfig caching earlier than this is not possible.

I am going to make a change to both of these PRs that cache on the ServerAuthConfig, that way the stateless nature is preserved where the server caches the ServerAuthConfig and where it doesn't the correct CallbackHandler is used to initialise the ServerAuthModule as described in section 3.5.

[arjantijms]

By creating a new ServerAuthConfig I am expecting this statement to be honoured, by not honouring this the ServerAuthModule is using a CallbackHandler passed in to other calls to AuthConfigProvider.getServerAuthConfig resulting in requests randomly manipulating the CallbackHandlers of other requests.

Yes, I hear you. This was my exact confusion as well when initially reading this and when we came up with the code as used by Soteria today.

The clarification I got however was that essentially there's one callback handler instance in the system, that can/should work on behalf of multiple requests.

I'm 100% with you that it feels weird in the way the API is setup now that you're passing in what you think is a new instance of the callback handler, while in fact it's assumed it's always the same one.

My assumption (I don't know if this is the actual reason) is that the API was designed such that you don't need to keep track of which is the first request. You pass in the one and only system callback handler, and if it's the first request it's used, and if it's the second request it's either ignored or set again without effect (since it's the same one again).

Just out of curiosity and not playing down the fact that certain systems have unique (non-single) callback handlers, but why is the callback handler in Elytron/WF specific to requests these days? In WF10 days there used to be a single one, right? Again, just curious and not trying to reduce the importance of this PR.

[arjantijms]

ps, re-quoting your original comment again:

I see from the spec that an integration of JASPIC could choose to use a single CallbackHandler what I don't see from the spec is anything requiring it to do so.

I guess this is what it all boils down to. The spec is unclear here, and "could" should have been strongly stated as "must". This is following the discussion about exactly this point which at the time was clarified by:

yes the callback handler is expected to be thread safe. I don't know if that is ever stated as a requirement, but it must be the case [...]

There was a number of these clarifications ready to go into the JASPIC spec for Java EE 8, but unfortunately that never happened.

[darranl]

Looks like a conflict specific to master so will adjust that separately.

I have just pushed another change to the branch, this changes the logic to cache the ServerAuthModule instance within the ServerAuthConfig instance that is created - anything now caching the ServerAuthConfig will be using a single instance of HttpBridgeServerAuthModule which is created at the point we have enough information i.e the CallbackHandler as that is the one piece not known in advance.

Another possible issue was the CallbackHandler reference was not volatile and access was not synchonized despite being set across multiple threads, as the CallbackHandler is now available at the time the mechanism is instantiated this can be stored in a final variable making concurrent access across Threads safe.

This I believe brings the implemntation into alignment with section 3.5 of the JASPIC specification so the CallbackHandler in use by the mechanism is the one that was used to create the corresponding authentication context configuration object.

Overall the general reason the CallbackHandler is stateful is it takes multiple Callbacks to establish a resulting identity, this is one of the APIs where although the handler could be called with a single call it can also be called multiple times so the state is tracked to create the resulting identity - although not as applicable in the Soteria case that could certainly be the case for general JASPIC configurations where multiple ServerAuthModules could be configured.

[darranl]

That last push should clean up the merge conflict.

[darranl]

BTW off topic - these travis failures should be resolvable by switching to openjdk8 instead of oraclejdk8 we saw this on some of our repos. I can submit a PR for that if you wanted unless this needs to be tied to the Oracle JDK.