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

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

Pavel claimed:
>>  I claim that this valid(caller_perms()) test was always bogus;
>>  the proper test is `caller_perms() == #-1'

Rog urked:
>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'.
...
>Note that under the current regime both forms are EQUALLY BROKEN since
>wizard code is indeed allowed to do set_task_perms(#-1).

In a message that wasn't sent to the list, Seth answered to my last
question - is anyone using this 'feature'?
His answer is yes:
>It's possible to make a secure verb +x and have it check for
>valid(caller_perms()) before raising an error if appropriate.  Then it
>may be called from the command line or by extended parsing mechanisms,
>with no significant changes to the code.
>After I've done my in-DB parsing I can leave 'player' alone and set the
>perms to #-1 and then be done with it.

This is fairly convincing as good usage. How will all this system change
(in 1.8.0?) when all the parsing is done in-db, and all command calls are
initiated by verbs?

Rog continues:
>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.

I'm not sure why using callers() for determining 'the absence of a calling
frame' is breaking any abstractions. It isn't nice that it's so
inefficient, to build sometimes huge lists of lists, just to test that
they're different from {}.
Maybe a simple solution would be to create a bf_has_callers() that would
return a simple boolean.

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

Hmmm... by the same reasoning, we could claim that the server should
enforce that all objects have to be valid, so we needn't use this kind of
checks when working with objects?
I guess the most common way to simplify code when you can't assume things
is to make it !d.

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

A possible solution to this, and in fact to all these problems, would be to
modify the task execution sequence so a verb owned by invalid perms is
ignored, as if the verb weren't defined at all.
Or, the verb could be removed as well.

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

The main problem I can see with this is compatibility between databases.
Some code might work nicely with a certain setting in one database, and
become a security hole (or fail to work) in another database with other
settings. Sure - people should pay attention to these things when porting,
but why invite errors?

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

This is the reason why I'm trying to come up with a fix to set_task_perms.

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

Or has_callers(), as above. Alternatively, call it is_top_level() with the
opposite meaning.

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

Or in Seth's suggested in-db parsing scenario.

In my previous message, I suggested making set_task_perms(invalid) return
E_INVARG.
Then I noticed that this can be as bad as the original problem, as in !d
verbs that set_task_perms(something); and ignore the result. If the result
was E_INVARG and not a perms change, the task is still wizardly -

This is another possibility:

static package
bf_set_task_perms(Var arglist, Byte next, void *vdata, Objid progr)
{ /* (player) */
    /* warning!!  modifies top activation */
    Objid oid = arglist.v.list[1].v.obj;
    Var r;

    free_var(arglist);

    if (progr != oid && !is_wizard(progr))
        return make_error_pack(E_PERM);

    if (valid(oid)) {
      RUN_ACTIV.progr = oid;
      return no_var_pack();
    }

    if (get_prop_safe(SYSTEM_OBJECT, "no_one", &r, 0) == E_NONE
        && r.type == TYPE_OBJ
        && valid(r.v.obj)) {
      RUN_ACTIV.progr = r.v.obj;
    } else {
      RUN_ACTIV.progr = SYSTEM_OBJECT;
    }
    return make_error_pack(E_INVARG);
}

If the object is valid, use it and return 0.
If the object is invalid, attempt to set_task_perms($no_one). If even
$no_one is invalid or just not defined, set the task perms to the system
object, which can hopefully be assumed to be valid and non-wizardly. In any
case, return E_INVARG.
I know this is far from being a good solution. Suggestions?

-------------------------------------------------------------
Gustavo Glusman               Founder/administrator of BioMOO
-- Gustavo@bioinformatics.weizmann.ac.il
-- http://bioinformatics.weizmann.ac.il:70/0h/Gustavo/Glusman
-- BioMOO: telnet bioinformatics.weizmann.ac.il 8888
           WWW:   http://bioinfo.weizmann.ac.il:8888
Follow-Ups:
Home | Subject Index | Thread Index