Re: [PATCH] speed up svn_repos_authz_check_access

On Wed, Aug 7, 2013 at 9:04 PM, Ben Reser <ben_at_reser.org> wrote:

> 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.
>

Err... the cache apr_hash_t by construction cannot contain keys (i.e.paths)
that are not in the authz file also, so is bounded by the size of the
corresponding svn_config_t. In fact, one could precompute the maximal
cache on the first call to svn_repos_authz_check_access() by
iterating over all paths in svn_config_t.

... 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.
>

I see your point. "Clearing the cache" here means a single svn_clear_pool()
call...

> 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.
>

Good spotting. I'll revert to the previous

   if (user) ...
   else if (authz->cached_user) ...

logic. So much for going for elegance :)

Cheers, Roderich
Received on 2013-08-07 22:03:35 CEST