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

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

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...)
Home | Subject Index | Thread Index