First of all I'd like to say thanks for doing this. I'd thought
briefly about doing something like this yesterday. This is a good
start.
The biggest issue I see with this is that you're adding an unbounded
cache. An attacker can open a connection and starting making requests
to fill up a servers memory. As long as they keep making authz
requests as the same user on the same connection they will be able to
increase the server usage everytime we do authz against an entry that
exists in the authz file and hasn't been cached. This is a distinctly
different behavior than our caching of the parsed authz configuration
in memory since every connection with the same conf caches the same
data. Yes I think this is a relatively small issue for most users but
at the large end of the scale it may be a viable attack. So there
needs to be some way to limit the number of entries we are caching or
the amount of memory we are using.
The other issue I see is that this code assumes that a connection
belongs to a single client. That is probably true for the vast
majority of our users but if there is a proxy that proxy might be
serving multiple clients connections on the same connection to the SVN
server. In these cases the authz cache is going to be cleared
regularly for that connection and will actually hurt performance.
For these two reasons I think we need a knob. My preference would be
a single knob that lets you set the number of entries or the amount of
memory to use for the cache. Sites that have proxies as described
above can simply set this to 0 to disable this functionality. I'm not
sure what the best way to go about implementing this but the svn_cache
functions might be a good start. Then again they look way more
complex than what's really needed here.
On Wed, Aug 7, 2013 at 10:19 AM, Roderich Schupp
<roderich.schupp_at_gmail.com> wrote:
> I revised the patch so that svn_authz_t->pool is now a sub pool of the pool
> passed
> to authz_create() and will be cleared when we encounter a different user.
>
> /* Check whether USER is still the same as authz->cached_user;
> * otherwise blow away authz->cache and remember the new USER.
> * Create authz->cache if it doesn't exist yet.
> * Note: Use authz->pool instead of pool as the cache must have the
> * same lifetime as authz.
> */
> if (!(user && authz->cached_user
> && strcmp(user, authz->cached_user) == 0))
> {
> svn_pool_clear(authz->pool);
> authz->cache = NULL;
> authz->cached_user = user ? apr_pstrdup(authz->pool, user) : NULL;
> }
This code will incorrectly clear the cache on every anonymous access
even when user and authz->cached_user are both NULL.
Received on 2013-08-07 21:05:09 CEST