[Opensim-dev] Fix provider selection and possible security issue with plugins

[Ryan McDougall]

On Tue, Jul 29, 2008 at 9:07 PM, Melanie <melanie at t-data.com> wrote:
> Hello,
>
> as commented on Mantis, it would be wrong to disallow loading of
> arbitrary DLLs. There are already extensions that are not in the

That statement just isn't correct. It would be _madness_ to allow
loading of arbitrary executable code. Luckily we are talking about
something far more specific, so please read on.

> core SVN, and there will be more. These currently work by dropping
> them into bin.

What exactly are these?

If they are RegionModules, then I haven't touched them. I sense the
possibility of dragons.

If these are any of the plugins that I have modified already, have any
of them broken? If not they my changes haven't broken anything, have
they? Lets consider this a bit deeper:

The use case here was initially OpenSim.Data.MySQL.dll,
OpenSim.Data.MSSQL.dll, et al. All of which do the exact same thing,
but in a mutually exclusive way. They both extend a specific extension
point, and thus only one of this type should ever be loaded. Correct?

Currently this is accomplished in *_Config.xml in a field called
"database_provider" which is usually set to OpenSim.Data.MySQL.dll or
some related, and is successfully loaded and executed so long as they
implement IGridData.

The previous loader, before my work, took that provider config string,
passed it directly to a loader which naively attempted to place the
file it names in memory and execute anything that implemented the
IGridData interface. You could argue that the only way for an attacker
to use that vulnerability was if he had local access to the machine,
and thus all bets were off anyways, but in principle I tend to avoid
arbitrarily executing things when not necessary. Its just sane best
practise.

A previous version of the code, I think the one everyone is running
now, works by doing string operations on the provider string to hack
out something that is already embedded within the Mono.Addins
manifest. However I believe its clearly documented as a hack, and I
generally detest poor quality code, even (especially?) if its mine. I
raised this issue previously on the mailing list but no one commented
at the time, likely because they had no idea what I was on about.

This patch changes it from being a hack on a provider string to tease
out a "Mono.Addins id", to being somewhat self-documenting "provider"
attribute in the manifest. Its smart and sane and good software
engineering, regardless to its resemblance to "just renaming".

> I believe it's wrong to limit OpenSim to loading plugins known at
> compile time.

Where on earth is it written that this is a compile time issue??

If fact one of the nice things about an external XML manifest is that
you can change it without a recompile. Nothing here is remotely
compile-time as that would *entirely* defeat the purpose of a dynamic
loader and would be silliness of a monty python magnitude.

> I don't know if that is where you want to go, but one of your
> comments gave me that impression. I would feel very uncomfortable
> with any change that limits the system to loading only DLLs listed
> in core code.

Fhew! We dodged a bullet then, because there is no danger of that happening! :)

If I have misunderstood, or somehow broken 3rd party modules, please
let me know so I can fix things immediately.

> Melanie
>

Thank you for your feedback and concern Melanie. :)

Cheers,


> Ryan McDougall wrote:
>> Details on mantis.
>>
>> http://opensimulator.org/mantis/view.php?id=1852
>>
>> Review and commit as usual would be greatly appreciated. Next step is
>> documenting the process of making a "new-style" plugin:
>> http://opensimulator.org/wiki/How_to_create_a_dynamic_plugin
>>
>> Finally, I think the most important dynamic loading is done, outside of
>> RegionModules, which I haven't touched for now. The completed plugins
>> are:
>>
>> IApplicationPlugin
>> IGridPlugin
>> IGridDataPlugin
>> ILogDataPlugin
>>
>> The plugins I should also do because they are probably important:
>>
>> IAssetProvider
>> IInventoryData
>> IUserData
>>
>> The rest should be a simple transformation based on my instructions
>> (when they're finished). Anyone want to go for some low-hanging fruit by
>> finishing the process?
>>
>> IGenericConfig
>> IDataSnapshotProvider
>> IClientNetworkServer
>> IPhysicsPlugin
>> IMeshingPlugin
>> IRegionDataStore
>> IEstateDataStore
>> ITerrainEffect
>> ITarget
>> IDataNode
>>
>> Lastly, is there a special meaning given to ifaces that end with Data or
>> Provider?
>>
>> Cheers,
>>
>> _______________________________________________
>> Opensim-dev mailing list
>> Opensim-dev at lists.berlios.de
>> https://lists.berlios.de/mailman/listinfo/opensim-dev
>>
>>
> _______________________________________________
> Opensim-dev mailing list
> Opensim-dev at lists.berlios.de
> https://lists.berlios.de/mailman/listinfo/opensim-dev
>