Hey there,
On Thu, 22 Feb 2001, Bodo Moeller wrote:
> On Wed, Feb 21, 2001 at 07:06:29PM +0100, [EMAIL PROTECTED] wrote:
>
> > Log:
> > This change allows a callback to be used to override the generation of
> > SSL/TLS session IDs in a server. According to RFC2246, the session ID is an
> > arbitrary value chosen by the server. It can be useful to have some control
[snip]
> For other callbacks and parameters that exist both in SSL_CTX and in
> SSL structures, we copy the SSL_CTX default to the SSL structure in
> SSL_new and don't look at the SSL_CTX value later. IMO, we should do
> that here too for consistency.
Yup - fair call. The SSL structure *is* having its callback copied from the
SSL_CTX during SSL_new() anyway, so what you say is pretty easy to achieve.
However, I'm trying to fix a more serious problem first ... :-)
> > + /* Finally, check for a conflict */
> > + if(SSL_CTX_has_matching_session_id(s->ctx, ss->session_id,
> > + ss->session_id_length))
> > + {
> > + SSLerr(SSL_F_SSL_GET_NEW_SESSION,
> > + SSL_R_SSL_SESSION_ID_CONFLICT);
> > + SSL_SESSION_free(ss);
> > + return(0);
>
> This has the same race condition as the previous code (there's a time
> window between checking for a conflict and adding the session to the
> session cache), except that now we can't reason that the PRNG is
> extremely unlikely to ever generate a collision of that length.
Hmm. Yes.
However, the problem remains that if external session caching is being used,
even if the race in the "local" cache is resolved, the same race needs to
resolved in the external cache which is less trivial. There would need to be an
atomic operation that simultaneously checks if the session ID is unique *and*
adds it to the cache if it is. Actually it's worse than that - the point at
which we check for uniqueness of the session ID is too early in the handshake to
have a session to put in the cache - so we're actually talking about reserving a
session ID in the cache if it doesn't conflict. Ugh. Trying to write this as a
two-phase commit, (ie. first commit to the local cache, second commit to the
external cache, with a rollback if necessary), together with getting the locks
right for local cache operation as well as external cache operation (the latter,
if it involves networking directly or via nfs, etc, can't be done inside a
global lock, can it?), could be tricky.
Anyway, as I mentioned there's something else I'm trying to address first. The
uniqueness checking is not right currently - SSL_SESSION_cmp() (as used in the
local cache, thought the lhash "cmp" callback) factors in the ssl version (which
it should). But the SSL_CTX_has_matching.... function is using the ssl version
from the SSL_CTX's "meth". This could be different to the actual SSL/TLS version
being negotiated in the SSL handshake in which this check is being made. (This
was my oversight - Lutz spotted it). I'm checking in a fix for this soon
hopefully.
> Previously the race condition did not matter because we had control
> over the generation of session IDs, but now the application takes over
> responsibility for this. Also we'd have to query the external cache
> for conflicts. (With the current API for external session caches, we
> can't avoid the race condition, so the API would have to be extended.)
This can't *really* be done anyway. The interaction between local and external
caching is too loose I think. Eg. a "get_session" operation on an external cache
might be called if a requested session ID doesn't exist in the local cache, but
by the time it returns, another thread may have created a conflicting session ID
in the internal cache. Similarly, a "put_session" may succeed locally but fail
externally - even if we check externally first because another thread (or
another machine even) may get to the external cache inbetween our check and our
"put".
> The previous conflict-checking code was not really functional;
> it was just there mostly as a reminder that session IDs must
> be unique (and maybe as a sanity check for PRNG output).
> Now that the session ID generation becomes more complicated,
> we should either fix these problems or declare that applications
> are solely responsible for ensuring uniqueness of session IDs;
> in the latter case, we could delete the conflict checks
> to keep the code clean.
Frankly, that's tempting. Not sure if it's the best way to go or not, but it
would certaily make OpenSSL's side of things simpler, albeit that callbacks may
be obligated to be *very* careful to avoid race conditions blowing up their
SSL/TLS handshakes (actually more likely is that we could let handshakes
continue, but the race would affect whether sessions got cached or not).
For now at least, one option is to pass a configurably smaller buffer to the
callbacks for it to choose without making any uniqueness considerations. So on
return, we could at least be sure OpenSSL is going to fill *enough* of the
remaining session ID with random data. This reduces us to an equivalent form of
the existing code - namely assuming our session ID space is large enough for
this problem to go away. Except of course, we give part of that space to the
callback for application-specific control, and use the what remains of it for
our randomness and uniqueness assumptions.
I'm less enthusiastic about that last option though.
Cheers,
Geoff
______________________________________________________________________
OpenSSL Project http://www.openssl.org
Development Mailing List [EMAIL PROTECTED]
Automated List Manager [EMAIL PROTECTED]
