1

We have been tasked with running security audit scans on our MVC web applications with IBM AppScan and OWASP ZAP. We've been able to understand and mitigate 99% of the vulnerabilities highlighted by these tools, but I've come across one that has me stumped. In one of our projects, we have a few custom data validation classes that implement the System.ComponentModel.DataAnnotations.ValidationAttribute. The general pattern of these classes is:

[AttributeUsage(AttributeTargets.Property | AttributeTargets.Field | AttributeTargets.Parameter)]
public class WhateverAttribute : ValidationAttribute
{
    public Type ServiceType { get; set; }
    public string ServiceValidationMethodName { get; set; }

    protected override ValidationResult IsValid(object value, ValidationContext context)
    {
        if(value == null)
            return ValidationResult.Success;
        var service = DependencyResolver.Current.GetService(ServiceType);
        if(service == null)
            throw new ArgumentNullException("....");
        var instanceType = context.ObjectInstance.GetType();
        var valueType = value.GetType();
        var method = ServiceType.GetMethod(ServiceValidationMethodName, new [] { instanceType, valueType });
        if(method == null)
            throw new ArgumentNullException("....");
        var isValid = (bool)method.Invoke(service, new[] { context.ObjectInstance, value });
        return isValid ? ValidationResult.Success : CreateValidationError(context);
    }
}

AppScan flags the line that actually invokes the validation method with a "Malicious DynamicCode Execution" vulnerability. I've done a bit of reading around the interwebs and cannot puzzle out whether or not using reflection to invoke methods in this context is a security risk. Is it? If so, how can it be mitigated? If not, what do I tell my security team to convince them that we are OK?

AJ.
  • 16,368
  • 20
  • 95
  • 150
  • Have you seen this article [Security Considerations for Reflection](https://msdn.microsoft.com/en-us/library/stfy7tfc(v=vs.110).aspx) – Nkosi Aug 08 '16 at 20:37
  • @Nkosi yes, I did, though I had trouble figuring out how that applied. – AJ. Aug 08 '16 at 21:11
  • I'm thinking, your attribute allows for the service type and method to be invoked. If a malicious party is able to emit their own controller and attach your attribute they can invoke their own validation code which would be in their provided service type with the context. – Nkosi Aug 08 '16 at 21:21
  • There used to be a lot of debate about reflection usage in web sites and something called "medium trust". This is all obsolete. What applies to your case is the most important now (and ever I guess) is to focus on protection at OS level. Check this post for more: http://stackoverflow.com/questions/16849801/is-trying-to-develop-for-medium-trust-a-lost-cause – Simon Mourier Aug 09 '16 at 06:18

1 Answers1

3

Let's take a look at OWASP explanation for Dynamic Code Evaluation ('Eval Injection')

The nightmare scenario is

  $myvar = "varname"; 
  $x = $_GET['arg']; 
  eval("\$myvar = \$x;");

The infamous JavaScript Eval() function using string param to do just about anything the UI user asks!

The other examples they give are 2 types of SQL Injection, HTML XSS, and use of php Server.Execute() .

What do these cases have in common? They are all dynamic environments. The worst ones are literally EXECUTING passed parameter strings.

How does any of this relate back to your code?

Mostly, it's your use of Invoke() and DependencyResolver. A cousin of the dread Eval(), under the right circumstances, Invoke() and "Unmitigated Reflection" could be used to

find a constructor to leverage as part of an attack

.

Scary? Very!

What does it really mean? It's not exactly "Dynamic Code Evaluation"...

.

The details are in OWASP explanation of Unsafe use of Reflection

If an attacker can supply values that the application then uses to determine which class to instantiate or which method to invoke, the potential exists for the attacker to create control flow paths through the application that were not intended by the application developers. This attack vector may allow the attacker to bypass authentication or access control checks or otherwise cause the application to behave in an unexpected manner.

.

This language is purposely vague. "May allow ... attacker to create control flow paths... or otherwise cause... behave in an unexpected manner." But even if you take the warning literally, this is nothing like "Dynamic Code Evaluation" .

Their specific example: 

   String ctl = request.getParameter("ctl");
   Class cmdClass = Class.forName(ctl + "Command");
   Worker ao = (Worker) cmdClass.newInstance();
   ao.doAction(request);

.

The vulnerability here is instantiation of a Class whose name comes from a string parameter from the UI. I could see how, given enough attempts, a script kiddy could find a way to... do... something...

.

Q: What is potentially dangerous in this example?

A: This class could be an access attribute, an UserManager, or an access cookie factory.

Q: How would an attacker leverage it?

A: Somehow...

.

I hope you've become a little skeptical by now, because... There's some serious reach here.

Eval(), Invoke() and doAction() are types of Dynamic Run-Time methods that can be dangerous.

They are "Run-Time" evaluated and executed. Under the worst circumstances, they are able to create code dynamically. The more dynamic, the more dangerous.

If they instantiated a class around "user input", a user could possibly "find a constructor to leverage as part of an attack".

.

Conclusion:

Application Scanners look for smoke, signs of potential trouble. Dynamic Code, in general, is dangerous, and therefor SHOULD be flagged and checked. If you want to be truly safe, don't create anything. Certainly don't do anything dynamic! (sarcasm)

Note: Most security holes are caused by Dynamic Run-Time Operations.

The problem here, again, is that App Scanners are better off identifying dubious risks than missing something. And this is an example of a dubious risk, because no one get concrete about it. Not, at least, the same we could with SQL Injection, Eval Injection, XSS, etc.

Your specific example is a long way from OWASP's example of "Unsafe use of Reflection". No magic strings Evaluated.

Your code is nothing like "Dynamic Code [Execution]". It IS Dynamic Code, and NO ONE can tell you that there ISN'T any danger scenario.

But it feels like a reach. Unless I'm missing something, your code is not creating Objects with names passed by parameter strings.

Final Note: The OWASP Page on Reflection injection was deleted in favor of "Unsafe use of Reflection". Reflection Injection has better a more menacing feel... but the term requires more concrete examples akin to SQL Injection.

Dave Alperovich
  • 32,320
  • 8
  • 79
  • 101