views:

573

answers:

6

I have used the following code in a number of applications to load .DLL assemblies that expose plugins.

However, I previously was always concerned with functionality, rather than security.

I am now planning to use this method on a web application that could be used by groups other than me, and I would like to make sure that the security of the function is up-to-snuff.

private void LoadPlugins(string pluginsDirectory)
{
    List<IPluginFactory> factories = new List<IPluginFactory>();

    foreach (string path in Directory.GetFiles(pluginsDirectory, "*.dll"))
    {
        Assembly assembly = Assembly.LoadFile(path);
        foreach (Type type in assembly.GetTypes())
        {
            IPluginEnumerator instance = null;
            if (type.GetInterface("IPluginEnumerator") != null)
                instance = (IPluginEnumerator)Activator.CreateInstance(type);
            if (instance != null)
            {
                factories.AddRange(instance.EnumerateFactories());
            }
        }
    }

    // Here, I would usually collate the plugins into List<ISpecificPlugin>, etc.
}

The first few concerns I have:

  1. This function reads the entire directory and doesn't care about what assemblies it loads, and instead just loads all of them. Is there a way to detect whether an assembly is a valid, functional .NET assembly before loading it with Assembly.LoadFile()?
  2. What kind of exception handling should be added to the function to prevent initialization of the assembly from halting my code?
  3. If I want to deny the assembly the right to do the following: Read/Write files, Read/Wite the registry, etc, how would I do that?

Are there any other security concerns I should be worried about?

EDIT: Keep in mind that I want anybody to be able to write a plug-in, but I still want to be secure.

+2  A: 
  1. If you are concerned with the security of your assemblies, you should Strongly Name them. This provides a high level of security that the assembly is indeed the one you intend for it to be.

  2. Exceptions you might encounter during load are as follows. Add try/catch around your load attempt at Assembly.Load() and react according to the error type:

    • ArgumentNullException
    • FileLoadException
    • FileNotFoundException
    • BadImageFormatException
  3. Assemblies you load dynamically should have the same rights as the user account which loaded them unless this assembly is in the GAC. Create a service account with the rights you desire, and run your application using this account to control access.

Chris Ballance
I like your answer to #2, but for #3, can I "Jail" the assembly I'm loading so that my application can run free, but the plugin is limited?
John Gietzen
Btw, I'm pretty sure IIS+ASP.NET can "Jail" applications.
John Gietzen
+1  A: 

If you're running 3.5, there's a lot to be said for the new System.AddIns stuff - take a look over at http://www.codeplex.com/clraddins for examples.

-Oisin

x0n
A: 

I don't know if this is the best way but when you call LoadFile on an invalid assembly you will get a BadImageFOrmatException because the assembly does not have a manifest.

The way your code is written currently you are pretty wide open to a Process Control Attack. Anyone who can get access to the directory and drop down an assembly that implements your interface can execute this attack. They do not even have to implement the interface very well, they can just provide a default constructor and do all the damage in that. This allows an attacker to execute code under the privilege of your application which is always bad.

So your only current protection is to protect access to the directory on an OS level. This may work for you but it is only one level of defense and you are reliant on security state you cannot control.

Here are two things you could look at:

  1. Strong naming the assembly and requiring the assembly be registered in the GAC is most likely the safest way to do this. If you can do this you need to find a way to provide the Assembly's full name to your application and load it with Assembly.Load().

However I doubt you want to install these plugins into the GAC so you could do it this way:

  1. Within your application provide a way for users to register a plugin, essentially a mini-GAC. When they do you store the location and name of the Assembly as well as the public key. This requires that the Assembly is strong named.

This way you will only load Assemblies that someone with privilege to your application has provided, most likely someone who has rights to add a plugin. Before you load the Assembly you can check that the Public Key matches what was provided when the Assembly was registered to prevent an attacker from just replacing the Assembly. This code is fairly simple:

 private bool DoPublicKeysCompare(Assembly assembly, byte[] expectedPublicKey)
 {
  byte[] assemblyKey = assembly.GetName().GetPublicKey();
  return expectedPublicKey.SequenceEqual(assemblyKey);
 }

So now to execute an attack on you I must somehow get privileged to change the PublicToken value and get access to the directory and change out the file.

Flory
+1  A: 

1) strong name the assembly with a certain key. * you do not have to put it in the GAC * you can re-use a key to sign more than one assembly * when you re-use the key, you get the same "public key" on each signed assembly

2) on load, check that the assembly has been strong named with the key you're expecting * You can store the public key as a binary file, an embedded resource, or use the existing public key of the executing assembly * this last way may not be the best way as you may want to differentiate assemblies signed with the "plugin" key from those signed with a regular key)

Example:

public static StrongName GetStrongName(Assembly assembly)
{
    if(assembly == null)
        throw new ArgumentNullException("assembly");
    AssemblyName assemblyName = assembly.GetName();

    // get the public key blob
    byte[] publicKey = assemblyName.GetPublicKey();
    if(publicKey == null || publicKey.Length == 0)
       throw new InvalidOperationException( String.Format("{0} is not strongly named", assembly));

    StrongNamePublicKeyBlob keyBlob = new StrongNamePublicKeyBlob(publicKey);

    // create the StrongName
    return new StrongName(keyBlob, assemblyName.Name, assemblyName.Version);
}


// load the assembly:
Assembly asm = Assembly.LoadFile(path);
StrongName sn = GetStrongName(asm);

// at this point
// A: assembly is loaded
// B: assembly is signed
// C: we're reasonably certain the assembly has not been tampered with
// (the mechanism for this check, and it's weknesses, are documented elsewhere)

// all that remains is to compare the assembly's public key with 
// a copy you've stored for this purpose, let's use the executing assembly's strong name
StrongName mySn = GetStrongName(Assembly.GetExecutingAssembly());

// if the sn does not match, put this loaded assembly in jail
if (mySn.PublicKey!=sn.PublicKey)
    return false;

note: code has no been tested or compiled, may contain syntax errors.

Oplopanax
+1  A: 

Take a look at AddIn framework from Microsoft as it provides some of the capabilities that you're looking for such as security and isolation.

I would also recommend that you use reflection only methods such as ReflectionOnlyLoad and ReflectionOnlyLoadFrom to ensure nothing from the assembly you're querying metadata from gets executed as you examine it. Once you determine that the assembly has what you're looking for only then you can load it.

Mehmet Aras
A: 

My own preference is to use Assembly.ReflectionOnlyLoadFrom(...)

see the following MSDN article for more information:

How to: Load Assemblies into the Reflection-Only Context

csharptest.net
This is not what I want. I want a plug-in.
John Gietzen