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

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

- Date: Sat, 23 Sep 1995 10:13:11 PDT
- From:

    w.r.t. Seth's use of set_task_perms(#-1) to fake a top-level call to
    +x command verbs; I'd call this a hack.

    Note that the current situation is somewhat strange in that
    some parsing is accomplished by the server and leads to a top-level call
    while other parsing is done in-db and leads to a call from some parsing verb.
    This basically means that what you really want to be testing for in the
    +x command verbs is NOT top-level-ness but rather called-by-the-parser-ness,
    which for now happens to coincide with top-level-ness, which in Seth's world
    might be something like

       callers() == {} || caller == $parser

    and which in the totally-in-db-parsing world might be just

       caller == $parser

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

    When I said "massive abstraction violation" I wasn't referring to this
    particular use of callers() but rather the availability of callers() in
    general.
    The central principle here is being able to understand what a verb is
    supposed to do in terms of what arguments you give it.
    By referring to callers() and thus being able to refer to ANYTHING AT ALL in
    your call stack, verbs can have arbitrarily strange dependences on how they
    are called, which in turn makes the verb difficult to reason about, difficult
    to debug, and, if we ever decide to try compiling this code someday, difficult
    to optimize.

    This is not to say that there aren't legitimate uses for callers().
    In particular the canonical use is for debugging --- getting a stack trace
    is a standard thing to want to be able to do.

    However, if, after you finish your debugging, you still find essential uses of
    callers() in your code, this is a good sign that you should consider a
    redesign,
    e.g., rather than assume that the value of `this' 3 frames up is going to be
    something useful, why not fix the protocol so that that caller can either stash
    that value in a property somewhere or pass it as an actual argument?  If that
    caller should happen to be reimplemented later and use a different sequence of
    verb calls (say, using 4 verbs to get to your verb instead of 3), things may
    mysteriously break.

    And if you're wondering about the way callers() gets used in LambdaCore's
    $player:tell and :moveto verbs, I'd say the same reasoning applies.
    The current protocol is indeed broken.  If it were the case that there were
    an explicit originator argument for these verbs, e.g.,
       victim:tell(me, "Hi")
    rather than
       victim:tell("Hi")
    the :tell verb would only have to check that its immediate caller were trusted
    to be supplying a correct originator.  There would be no need to search up the
    stack for the originator.  True, most callers would not actually BE trusted,
    which would mean that if you wanted to send a message to a random person, you'd
    have to make use of some mutually trusted agent, like the paging system, the
    mail system, etc...  While this may seem somewhat more cumbersome and require a
    bit more work on the part of programmers, you might compare its OVERALL cost
    with what happens now where EVERY listener is permanently in debug-mode,
    searching the entire call stack on every utterance and move attempt.

    It's this kind of routine use of callers() that is bad, and what I object to
    is anything that might FORCE callers() to be used routinely.  I'd rather have
    people use
       $code_utils:top_level()

    and, in the hopes that a top_level() builtin becomes available later, secretly
    implement it for the nonce as

       length(callers())==1

    rather than tell people to use callers() directly.

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

    You're about to trigger another of my long-delayed postings, but I'll save it;
    this message is long enough.  The short story is that there are indeed things
    the server could be doing with invalid object references that would likewise
    simplify coding.  (You can't however eliminate invalid objects altogether
    without eliminating recycle(); this would have a decidedly negative impact).

    The basic point is that the more invariants the server can enforce (cheaply!)
    the easier life is (ultimately) for the programmer.  The trick is picking
    invariants that are useful without being too restrictive.

    >  I guess the most common way to simplify code when you can't assume things
    >  is to make it !d.

    Yes but this is a bad way to simplify code, especially if there are
    non-intuitive things being done with the error values being silently
    returned.
    The goal is to have code that is simple in the sense of being READABLE,
    not merely terse, i.e., to have code that is not only short,
    but also does exactly what it says it's doing.
    >
    >  >(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.
    ...
    >  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.
    Wonderful.  Just have things silently fail.  Even better we'll just have the
    caller charge on ahead as if nothing was wrong so that the ultimate error can
    occur miles from the scene of the crime.
    >  Or, the verb could be removed as well.
    and remove all possibility of recovery, debugging, etc...  no thank you.

    >  >Have a $server_options flag to indicate which condition the server enforces
    >  >   ....
    >  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?

    The point is the *current* situation is error-prone.  People writing code
    within a single DB are making mistakes and there is no way to catch them.

    Porting code between DBs that follow distinct coding conventions is always
    fraught with problems.  Doing porting in any kind of automatic way is a
    fundamentally insoluble problem; the programmer has work to do no matter
    what.

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

    The problem here is the use of !d verbs.  In theory, we're getting an
    exception system RSN so this might actually be moot.  (yeah right...)

- 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