MOO-cows Mailing List Archive
[Prev][Next][Index][Thread]

Re: [SERVER, SECURITY] bug in set_task_perms() ?

>  Gustavo Glusman writes:
>  > The problem is that wizperms can set_task_perms() to an invalid object.
>  > Some verbs rely on testing whether valid(caller_perms()), as a 
general test
>  > for 'am I being called from command line, or from another verb?', just
>  > because this is cheaper than using callers(). But setting task perms to an
>  > invalid may fool this.
>
>  I claim that this valid(caller_perms()) test was always bogus;
>  the proper test is `caller_perms() == #-1'

Urk...  no.

The former was perfectly okay back when taskperms were required to be players.
Back then, having the taskperm object be invalid guaranteed that it was #-1
and hence that this was a top-level call.  Thus there was no semantic
distinction between `valid(caller_perms())' and `caller_perms() == #-1'.
One might want to make a stylistic argument, but I don't see it; both look
equally artificial to me (i.e., if the intent is to test toplevelness).

Note that under the current regime both forms are EQUALLY BROKEN since
wizard code is indeed allowed to do set_task_perms(#-1).

The real problem is that there are simply no longer any reserved return
value(s) that caller_perms() might use to unequivocably indicate the
absence of a calling frame.

Moreover, independently of this, I think there *are* good arguments for
being able to enforce in-server that the task owner always be within a
certain set of objects.

(1) Being able to make assumptions about taskperms (e.g., that they are
    valid objects or that they are descendants of $player or $corporation)
    simplifies coding where one wants to do security (or other) checks
    based on various attributes of a task owner.

Compare
	caller_perms().wizard
	caller_perms().studly
with
	valid(cp=caller_perms()) && cp.wizard
	valid(cp=caller_perms()) && $object_utils:isa(cp,$player) && cp.studly

(2) IMO the vast majority of situations where code on LambdaMOO and similar
    sorts of places is running with invalid taskperms are of the form

	player/task-corporation FOO gets recycled
	FOO owns some verbs and nobody knows about them
	  or gets around to @chowning them
	one or more of these verbs get called.

Whether this is a mistake is a matter of policy.  Often, however it is
desired that all running tasks be charged to some existing task-running-entity
(TRE --- read "player" if you want) so that the task-queue scheduling 
mechanisms
in the server can be used to their best advantage, i.e., so as to keep any
particular TRE from monopolizing the server.  In such an environment, the
aforementioned situation is indeed a mistake since the forgotten verbs are not
running in a queue that anyone is responsible for.  One can moreover imagine
variants where someone donates some or all of his verbs to players that he
knows are to be recycled soon, and, with their collusion, set up a scheme
where he can use a larger fraction of the server than he would otherwise be
entitled.

The problem here is that should one actually want to enforce some restriction
on which objects are allowed to own tasks, there is NO WAY to do it in the
current server.  While I agree that the requirement (is_player()==1)
enforced by earlier servers was too restrictive, this is far different from
saying there should be no restriction at all or that it's not appropriate for
the server to be enforcing such a restriction.

PROPOSAL #1:
Have a $server_options flag to indicate which condition the server enforces
   (I)    task owner must not be #-1
   (II)   task owner must be valid
   (III)  task owner must be some object in a given list or a descendant
          thereof (e.g., {$player, $corporation, $hacker, $no_one}).
and perhaps also
   (IIII) task owner must be is_player()
if we care about being able to run old DBs.  The default should be 
either I or II.

Admittedly, we're not talking about a modification to just set_task_perms()
but also to set_verb_info() and the verb-call mechanism.

Since having improperly-owned verbs bombing out on call may offend people,
I'll also suggest

PROPOSAL #1A:
Have a hook #0:orphan_verb(obj,verb,args) to be called whenever an
incorrectly-owned (by whichever criterion happens to be in effect)
verb is called.  Its contract would be to either return a new (correct)
owner -- the orphan verb is chowned and then called --- or 0 to bomb
with E_PERM.  This allows enforcing restrictions while also
allowing recycled-player verbs to invisibly (modulo whatever logging
might happen in #0:orphan_verb) continue to work.


As for what to do about the toplevelness test in the absence of such a
server modification, there are only two possibilities:

(1)  give up entirely on using caller_perms() to test toplevelness.

The arguments against this mainly have to do with
     (a) the large amounts of legacy code that assume caller_perms()
         can be used in this way.
     (b) the only alternative being the `callers() == {}' or `!callers()'
         constructions, which are bad because of the potential expense
         of building the call-stack value that callers() returns.

By the way, there are many contexts where caller-stack nonempty is NOT a
failure case.  Consider the expression

        <top-level-test> ? player | caller_perms()

which occurs nearly everywhere a command verb has been made +x for the sake
of being callable by pass() or the :huh machinery (a whole topic in itself,
but never mind).

On esthetic grounds, given that callers() is such a massive abstraction
violation and that encouraging people to use it for ANY purpose is just BAD,
I dislike the idea of providing a use of callers() that is not only
legitimate but REQUIRED.

(2)  establish reserved illegal-to-use-for-taskperms values by
     coding convention.

For example, the de facto convention for LambdaCore is that all invalid
taskperms are reserved for this purpose.  Unfortunately, given the lack
of enforcement in the server, you don't get to find out about convention
violations until well after the fact.

It's double bad if an inadvertant violation of convention introduces a
security hole (e.g., wizard code inadvertantly does set_task_perms(foo.bar.baz)
where it turns out that foo.bar.baz == #-1, then calls some command
verb which is written assuming the relatively innocuous foo.bar.baz taskperms
but instead runs using player's taskperms, player happens to be a wizard,
suitably grabbed from a :tell verb, etc... the usual story).
Home | Subject Index | Thread Index