[Prev][Next][Index][Thread]

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

- Date: Thu, 21 Sep 1995 12:08:49 PDT
- From:
- Cc: moo-cows@parc.xerox.com

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

- Prev: Re: [SERVER, SECURITY] bug in set_task_perms() ?
- Next: Re: [SERVER, SECURITY] bug in set_task_perms() ?
- Index: MOO-cows
- Thread: MOO-cows

Home | Subject Index | Thread Index