[AFS3-std] Unlimited array lengths

[Benjamin Kaduk]

On Wed, 13 Mar 2013, Andrew Deason wrote:

> In the current AFS RPC-L definitions, there are several places where we
> have an array with no upper bound on its length. While implementations
> can and do cap this to prevent 32-bit rollover while allocating memory,
> this still means that we can cause an endpoint to allocate up to about
> 2G memory per relevant structure for a single call before the RPC
> handler is even reached (at least, the way that rxgen works in OpenAFS
> right now). On systems that overcommit memory, and even to some extent
> on systems that don't, that can be a serious DoS problem.
>
> Many instances of this are a part of RPCs that are usually only run by
> administrators, such as AFSVolCreateVolume's "name", so I don't think
> that's as much of an urgent concern. However, some are part of RPCs that
> are run by unauthenticated users, such as namelist/idlist in PT_NameToID
> and PT_IDToName, which were recently involved in an OpenAFS security
> advisory. And of course, many are involved in a server's response to a
> client, potentially allowing a server (or an attacker impersonating a
> server) to use up a ton of client memory.
>
> In OpenAFS, one proposed way to improve this is just to modify our .xg
> files to put limits on all variable-length arrays (openafs gerrit 9588
> is one such possible change). We could also have rxgen have an arbitrary
> universal cap, but I'm not sure about that, since what a "reasonable"
> max length is potentially varies so much between different
> structures/RPCs.
>
> Since modifying the RPC-L means modifying the protocol, it was mentioned
> that this issue should be brought up here. While in the short term, I
> think OpenAFS may just implement some limits to ensure server/client
> stability, it seems like we should agree on array limits here, at least
> for the long term (do we need an I-D just to specify a bunch of max
> lengths?). I'm also not clear on if it's considered acceptable to just
> put some limits in for existing structures/RPCs, since that would change
> the signature of the RPC.
>
> Ideally, I think I would want to modify OpenAFS' rxgen to not allow
> unbounded variable arrays; if something really warrants ~2G allocations,
> it should be explicitly specified. But I'm not clear on if that's
> allowed, or how we get there.

As you note, the issue with modifying rxgen is that a reasonable limit 
will be different in different cases.  I agree that something should be 
done, but have some preference towards analyzing each case (or at least 
glancing at it, doesn't have to be terribly intensive) to ballpark a 
limit.  Though, I suppose it would still be reasonable to have rxgen 
include a default limit that can (should!) be overridden so that 
sloppiness does not result in vulnerabilities.

I do not think that there should be a hard-and-fast rule that putting 
limit on variable-length arrays constitutes a protocol change; this would 
not really help if (e.g.) new deployed servers must retain the old 
(non-limited) interface for compatilibity with old clients.

The biggest issue I would be concerned with is whether imposing a limit 
causes some functionality to be impossible.  E.g., for PT_NameToID, if 
there is legitimiate need for more mappings than will fit in a single 
array, the RPC can just be called again with more of the list.  But for 
PT_ListElements or PT_ListOwned, if the list would "overflow" a fixed 
limit, there is no other way for the truncated information to be obtained. 
It seems like that case might require a new RPC to exist, even if the old 
one does get a new limit imposed on it.  This is kind of the opposite of 
what normally happens when a new RPC is added!

It kind of sounds like some person or persons are going to need to go 
through the existing interface descriptions and look at the cases which 
use variable-lengthe arrays.  I suspect we won't have a giant crowd of 
volunteers popping up, though.  It would be nice to at least get some more 
support that this is the right way to go.

-Ben