[crossfire] Hardening plugin system

[Mark Wedel]

Nicolas Weeger wrote:
> Hello.
> 
> Currently, a plugin can easily crash the server, which doesn't check
> parameters (just call a function with a NULL pointer, nice crash
> guaranteed). Also, server doesn't checks parameters and such, which can
> lead to invalid values (Str of 50 for a player...).
> 
> So should that be fixed in a way a plugin can *not* crash the server
> through a callback, or send invalid parameters?
> Note that preventing a plugin ever crashing the server is hard, since
> the plugin itself can crash leading to server crash (and that isn't
> something easy to avoid i think).
> 
> IMO "no" is an acceptable answer. We can after all "trust" a plugin to
> do the right thing.
> 
> If we choose the "yes, let's harden the plugin system" option, here's my
> suggestion for hardening:
> * when plugin requests an object/map/archetype, server keeps the pointer
> in an array and sends the index, which is used in subsequent functions
> * this way, it's easy to check the pointer's validity - check index, if
> inbounds ok, else issue
> * when the plugin returns, server knows what objects were affected
> (array can contain a "set_parameter" field, set when a setter is
> called), and can send updates to player accordingly - should also
> simplify plugin's code. Also can recalculate stuff when object is
> removed, whatever.

  Presumably for that last point to work, all the functions the change values in 
the plugin code would need to set some flag.  Otherwise, I don't see how the 
server can know an object changes.

  For example, plugin is called with an object whose object pointer is deadbeef. 
  The plugin makes some changes (say changes name) and returns.  The object 
pointer is still deadbeef.

  Unless the server keeps a copy of the object, and then runs a comparison, I 
don't see how the server can really know the object has changed.  And keeping a 
copy, running a comparison, and updating it would seem to be a fairly costly 
operation (maybe not a big deal, but I could potentially see that as preventing 
really wide spread use of the plugin if the performance is bad).

  I personally do think that the plugin should do basic checking - making sure 
set values are valid, etc.  I don't think that stuff is too hard.  And I think 
that may catch errors object comparison can't do.

  For example, if a field in the object type is an sint16, the valid value is 
-32768 to 32767.  If something tries to set the valid beyond that, it is bad.

  However, object comparison won't catch that - some plugin sets the value to 
123,456.  The value gets truncated, so is a legal value, but probably no what 
the plugin wants, and thus causes problems.  OTOH, if the actual plugin code 
checks the valid for the -32768 to 32767 range, it can log an error right there.

  Another thing (unrelated to this) is plugin verification of compatibility. 
More than once on my own working copy, I've done a make but not done a make 
install, only to track it down to the plugin being out of date.

  What I think could be done is that the plugin code has code someplace like:

#define PY_OB_SIZE sizeof(object)
#define PY_PL_SIZE sizeof(player)
etc for major structures.

  The server could have similar code, but different name.  In the server init 
code, it could initialize some global variables to those values.

  The plugin could then compare the size it thinks the structures should be with 
what the server thinks they are, and if any difference, logs an error and either 
exits (because it is sure to crash otherwise), or disables itself.

  This isn't foolproof - if fields are moved in the structure but size remains 
the same, that won't catch the difference (if really paranoid, could add some 
offset checks).  However, I think the case of fields moving about is low - the 
more common case is new fields being added or old fields being removed, and 
sizeof checks should catch that.