Empirical overview of security vulnerabilities [2/4]

The second post of this series focused on security vulnerabilities addresses two distinct issues: concurrent access and object encapsulation.

Concurrent access

This is one of the most difficult areas to master in development. At the design stage, many of us having only one brain, or at the test stage, because simulating concurrent operations is often tricky to set up and even more to reproduce. Concurrent access have impact on several quality dimensions: reliability, performance, maintainability, but also security. Here are some examples.

Sharing of attributes

One of the recurring problems in Java or C# development is the thread-safe notion: is the objet usable simultaneously by several threads or not ? This information should be available in the API documentation. Misuse can lead to unpredictable results. Through Cockpit, we no longer count unsynchronized uses of formatting classes in Java from java.text package, such as java.text.SimpleDateFormat. Because few Java developers know that without synchronizing calls to these classes, they will sometimes get very strange results...

Another example with a direct impact on security is related to the use of some web frameworks. In order to improve performances, many of them use instance pools for providing components handling HTTP requests. For example in Java: Struts (V1), Spring MVC or even servlets. Same problem with C# frameworks such as ASP.NET MVC.

The problem is that this pool mechanism is not always well exposed in the documentation, and the consequences may not be understood by developers. Worst case may lead to mix information from different users.

An example with a servlet (basic Java component handling a HTTP request):

public class SomeServlet extends HttpServlet
{
  private Account account;

  @Override
  protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException
  {
    account = retrieveAccount(req);
    doSomething();
  }

  protected void doSomething()
  {
    String name = account.getName();
    ...
  }
}  

This code stores information read from the HTTP request into an attribute of the servlet. Then it performs some operations through another method, operation which use the same attribute. The problem is that if a second request comes before the doSomething()method is called, the attribute will be replaced with information related to the second query. Because a servlet is provided by default through a single shared instance.

I remember having worked on a project for a worldwide known client where users complained sometimes about losing their context and viewing information related to other users. This was exactly this servlet attribute issue! Consequences were limited here because it was an intranet with information which were not confidential, but you can easily imagine the situation in critical environments such as bank applications.

So check your API documentation and track down attributes in components which are not thread-safe. In web applications, data must be stored in the context of the request, session or application.

Singletons

Although the use of singletons is being discarded in favour of dependency injection, it remains a common pattern considered as trivial to write. Yet our results show that more than one project on two contains singletons poorly written.

An example in C#, here the singleton is created at the first call (lazy instantiation):

public sealed class BalanceHistory
{
  private static BalanceHistory INSTANCE;

  private BalanceHistory() { }

  public static BalanceHistory Instance
  {
    get
    {
      if (INSTANCE == null)
      {
        INSTANCE = new BalanceHistory();
      }
      
      return INSTANCE;
    }
  }
}     

If the getter getInstance() is called two times simultaneously, the singleton may be recreated twice: if both tests find the singleton as null, they will both create a new instance, and callers will work on distinct singletons (and the singleton created first will be lost)...

There are several ways to solve this problem:

• create the instance at declaration: private static BalanceHistory INSTANCE = new BalanceHistory();. This is usually the best solution, this instantiation will be really effective when class will be loaded, so in the case of a singleton, at the first call of getInstance()!
• synchronize the creation of the instance: in C# through [MethodImplAttribute (MethodImplOptions.Synchronized)], or via some lock on a dedicated lock object
• use a pattern based on inner classes, cf. Initialization on demand holder idiom pattern
• use Double-checked locking pattern, even if it causes usually more problems than it solves (we considered this pattern as forbidden in the Cockpit)

Object Encapsulation

Protecting access to data is one of the basic principle of Object-Oriented Programming (OOP), but it does not mean that this kind of problems are solved by OOP.

Data accessibility

OOP allows to protect data by using keywords defining their visibility from the rest of the application, according to the location of classes: private, protected, public, internal (C#), extern (C#), ... Other keywords allow to prevent overloading a class or method: final (Java) or sealed (C#). This configuration of accessibility is not trivial. It requires to find the right balance between security and scalability, while keeping simple code.

Regarding the visibility of class attributes, the developer must add accessors to give them more visibility, keeping some way to check or modify data. Since this work is not very rewarding, that the code is burdened by methods (and properties in C#) which do nothing but set or return attributes, and that developers are lazy by nature, these attributes are sometimes defined as non-private.

And regarding overloading of classes or methods, most of developers do not take care of finalizing/sealing their code, without considering the risks.

These risks may be classified into two types:

• some ill-intentioned uses from third-party code. This objection is often theoretical, because class loaders do not easily allow such exploitation. However, some designs based on plugins or dependency injection makes these vulnerabilities easier to exploit
• implementations originally well-intentioned may introduce flaws by overloading critical functions which should be protected, for example a method retrieving user rights.

Developers must therefore always take care of visibility about data or critical processings.

Exposure of mutable data

Another area difficult to manage in OOP and which has a direct impact on security is the exposure of mutable attributes. A mutable object is an object whose state can be changed after initialization, e.g. an array or a StringBuilder but also most of the POJO / POCO created in an application.

The problem is that a getter on a mutable attribute does not only give some simple access permission, it allows the caller to change the state of this attribute. A list of user rights could therefore be easily changed:

public class User
{
  private Set<UserRole> roles = new HashSet<UserRole>();

  ...
  
  public Set<UserRole> getRoles()
  {
    return roles;
  }
}

public class MaliciousCode
{
  public void someMethod()
  {
    User user = userRegistry.findUser("myUserId");
    user.getRoles().add(UserRole.Admin);
  }
}

To guard against such problems, several defensive options are available:

• return a copy of the attribute:

  public class User
  {
    public Set<UserRole> getRoles()
    {
      return new HashSet<UserRole>(roles);
    }
  }
  

  In some cases, the copy must be deeply cloned, e.g. if the attribute is a list of mutable objects, each mutable object of the cloned list must also be cloned. The problem is that the callers will not have the original instance: if attribute is updated somewhere, cloned instances will no be synchronised.

• return a proxy wrapping mutable object and blocking update methods. For example:

  public class User
  {
    public Set<UserRole> getRoles()
    {
      return new HashSet<UserRole>(roles)
      {
        @Override
        public boolean add(UserRole o)
        {
          throw new UnsupportedOperationException();
        }
  
        @Override
        public boolean remove(Object o)
        {
          throw new UnsupportedOperationException();
        }
        
        ...
      };
    }
  }
  

  All update methods must be overridden. Compared to the previous option, avantage is that you can finely control access to each method, and most important, you keep original instance through proxy encapsulation

  Java also provides an API to lock easily collections in read-only using this mechanism, thanks to the class java.util.Collections. Just write for example:

  public class User
  {
    public Set<UserRole> getRoles()
    {
      return Collections.unmodifiableSet(roles);
    }
  }
  

Whatever the cases, these mechanisms must be reserved for specific cases where exposed data are critical, in order to avoid reduction of performance because of unnecessary instantiations.

To be continued... In the next post, we will discuss issues related to code injection.