[PATCH][saslauthd] cyrus-sasl-2.1.10/saslauthd credential caching

[Jeremy Rumpf]

Thanks, excellent feedback. I've been busy coding away all day. I'm not quite 
ready for another realease, but I thought I'd drop a line with some of the 
mods I'm making (they're pretty major).

> The biggie:
>
> cache_lookup() writes the user-provided password hash into the hash table
> BEFORE it is verfied.  There's now a race where an attacker can submit an
> arbitrary string as a password, (if there's currently an entry in the
> cache, it will be evicted), then while the real lookup is happening he'll
> try a second authentication, which will succeed if cache_purge has not yet
> been called.
>
> I think I have a general problem with cache_lookup writing to the hash
> table, I'd expect "lookup" to be a read-only operation.  If it fails, we
> do a lookup to the backend anyway, and then only if we SUCCEED in that
> lookup do we write the good password to the table.


Round 3 now has:

 cache_lookup()
 cache_commit()
 cache_discard()


>
> Another important one:
>
> No locking of any kind is used on the shared memory segment.  I'm not sure
> this is directly exploitable, but it could give odd behavior.
>
> Let's assume that we've fixed the above problem.  Now we have a
> cache_write() (that looks similar to the writing portion of the
> current cache_lookup()) instead of a cache_purge().
>
> If two processes are in cache_write at the same time, there's a race where
> they could both pick the same block to evict, and the result could be some
> mangled combination of both (or one or the other, but not both).
>
> Take this for instance (we assume that we had two cache misses, and that
> we only have one processor):
> .....
> ....
> ...
>
> There clearly needs to be some sort of semaphore on the table (or on
> entries, or something).
>

I've put in a first attempt at per bucket level locking via fcntl(). The only 
thing left to consider is if we should acquire a lock when the stats counters 
are updated. I'm not sure if the perf difference would be worth some 
inaccuracies in the hits/misses/attempts counters.


> Wasn't this originally going to use libmm?
>

My knowledge on libmm is brief (quick reading today),

Saslauthd already depends on fcntl() being there natively. 

If I understand correctly, libmm only does shm segment level locking. I'm not 
sure what the perf differences would be between per bucket level locking via 
fcntl() and table level locking via fcntl(). Since fcntl() is required by 
saslauthd, I went with per bucket level locking.

libmm introduces another dependency (including autconf checks), unless libmm 
would be distributed with the saslauthd package. I originally thought it 
beneficial to keep any additional dependencies down if a good alternative 
could be presented. 

Plus, all in all, it sounded pretty challenging. Haven't done any hard core 
coding in awhile :).


> Finally:
>
> I think shared memory is way overkill for the doors version, since it all
> takes place within one process.  Though, we'd still need to use some sort
> of councurrancy control.
>
> -Rob
>

Yes, I've started to addresses this to. I've broken out the memory allocation 
so that the unix IPC method will use a shared memory segment, while the doors 
IPC method would stick with a malloc'd segment. The doors IPC method will 
retain the fcntl() based locking for the malloc'd segment. I have some 
reading up to do on doors IPC (is it thread based?).

> -=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
> Rob Siemborski * Andrew Systems Group * Cyert Hall 207 * 412-268-7456
> Research Systems Programmer * /usr/contributed Gatekeeper

Thanks,
Jeremy