FishEye and Crucible

A bit more detail on this:

"Session fixation attacks are possible on a shared desktop where a attacker
pregenerates an INVALID session and then tricks a valid user to authenticate.
As a result both the attacker and the valid user have a valid session.
Automated vulnerability testers like WhiteHat flag this issue as a security hole."

An alternate fix was proposed at the same time:

"b) Use hidden fields to maintain pre-authn state - instead of a invalid session
on the server."

PS: There should be a JIRA issue linked to this, right?

Please assign me to issue OPENAM-72, and I will fix them too.

We believe the original security attack is false.

"shared desktop where a attacker pregenerates an INVALID session and then tricks a valid user to authenticate. "

You cannot pregenerate an INVALID session, you can generate a OpenAM session that's state is invalid. Typically you will have 120 seconds to do the following:

a) The attacker accesses the Auth UI
b) The attacker captures the SSOTokenID by writing down the SSOTokenID
c) The attacker then finds someone to login within the amount of time left who failed to see this from happening.

We just don't see this as a realistic attack vector and the fix was to break the original OpenAM architecture.

The java.util.logging.LogManager#getLogger checks whether the weak referenced object is null, and if it is, then it removes from the loggers Hashtable, so when the execution is on line 461, then the whole thing will work as there was never such named Logger at all.

Decision reached: review complete, OPENAM-75 created.

OPENAM-57

OPENAM-54

OPENAM-51 and OPENAM-78

This review is empty - I think one of your test reviews that can be deleted?

finalURL cannot be "" at this point, ref block in lines 546-549 of the changed version

                if(finalURL == null || finalURL.equals("")) {
                   showError(response,"GOTO or TARGET parameter is missing"
                       +" in the request");
                   return;
                }
 

As for the CDCClientServlet, how can we specify service, realm, method etc? I realise this was not possible before either so perhaps best left to a separate issue?

Crucible has gone wierd on me and is not showing any code changes.
From what I could see then this looks fine

I have successfully used this property before on the main server. I don't see it in the JIRA description but from the commit, I am assuming that the core functionality has not been broken and that it is only a DistUI issue.

yes this was wrong, it should be loginURI rather than finalURL

Yes a complex type would of been more cumbersome.

Yes, this was a problem only on the DAS

I read this method name first as the method that will return the same DN regardless of the original case, i.e. changing it if necessary.
Perhaps "normalizeDNMaintainingCase" is more descriptive?

Logout

Looks good to me, just one question I am not sure what happens with the "bindings" ?

This patch looks great, but I don't seem to see any logic, that resets the retryRunLogin boolean (so maybe this is only doable once or will there be a new AuthContext for the new retry?)

Looks fine to me too

Well in general this solution is better, then the original was, but this solution does not care with sites and servers, so if the next in the list is on the other site, it would connect to it, even if later in the list there is a server from the same site. (??)

If this commit stays in trunk, then it is really critical to fix OPENAM-89 for the next release.

okay, I'm just saying, you wrote the very same code 3 times in a single patch. I'm thinking something like:
private SSOToken getAdminToken() {
SSOToken adminToken = null;
try {
adminToken = (SSOToken) AccessController.doPrivileged(AdminTokenAction.getInstance());
} catch (AMSecurityPropertiesException aspe) {
if (authDebug.errorEnabled()) {
authDebug.error("AuthContext::getAdminToken: unable to get app ssotoken " + aspe.getMessage); //maybe attaching stacktrace too
}
}
return adminToken;
}

but if it's only used here, then maybe it's not necessary at all.

the question was more about, if it's setted only once true, then what happens in the following situation?

• openam container stops
• openam container starts, and the clientsdk protected app gets a new token and retryRunLogin is now false
• openam container stops
• openam container starts, -> is the retryRunLogin still false, or will there be a new AuthContext instance (so the retryRunLogin is true again)?