Why you should check values of public methods' parameters

Jun 11 2021

Author: Nikita Lipilin , Sergey Vasiliev

This note will answer the question - why PVS-Studio considers parameters of public methods potential sources of tainted data. The analyzer can issue warnings if such parameters haven't been checked before use.

The point is that undue confidence in external data may lead to various vulnerabilities - SQLI, XSS, path traversal and others. Most obvious examples of external data sources: parameter values of requests or text that a user enters (for example, in a text field).

Excessive trust in parameters of public methods can be even more dangerous. Public methods are the ones that can be called from other assemblies. For example, these are public methods of public classes. Most likely, such methods are APIs for interacting with the library.

So, what is the danger?

Developers who use library's APIs may expect data to be verified inside the called method. That's why data from an external source won't be checked before passing to it.

Developers of the library itself may assume that input data is already verified. Therefore, there is no need to check it.

Here's what we may eventually face with. Neither a library user, nor its developer has checked input data. This may lead to vulnerabilities when an attacker passes tainted data.

Note. Below are examples with V5608 diagnostic (search for possible SQL injections). This information relates to other OWASP diagnostics that also consider public methods to be potential sources of tainted data.

Let's see what it might look like in code:

public class DBHelper
{
  public void ProcessUserInfo(String userName)
  {
    ....
    var command = "SELECT * FROM Users WHERE userName = '" + userName + "'";
    ExecuteCommand(command);
    ....
  }

  private void ExecuteCommand(String rawCommand)
  {
    using (SqlConnection connection = new SqlConnection(_connectionString))
    {
      ....
      using (var sqlCommand = new SqlCommand(rawCommand, connection))
      {
        using (var reader = sqlCommand.ExecuteReader())
          ....
      }
    }
  }
}

The DBHelper class provides the ProcessUserInfo method for external use because the method is available from other assemblies. Note that this method's parameter - userName - is not checked before use. The external value is used to create the command directly (command variable). The resulting command is then passed to the ExecuteCommand method. There the command is used without a prior check to create an object of SQLCommand type.

In this case, if you take userName as a source of tainted data, the analyzer will issue a warning about a potential SQLI.

Look at a possible way to use the ProcessUserInfo method by an external application:

static void TestHelper(DBHelper helper)
{
  var userName = Request.Form["userName"];
  helper.ProcessUserInfo(userName);
}

The author of this code fragment might not have access to the DBHelper class code and will hope the data is verified inside the ProcessUserInfo method. Neither the current code, nor the ProcessUserInfo method has checked data. This means the application will be vulnerable to SQL injections.

The analyzer won't warn you about potential SQL injection when checking the TestHelper method's code. It is because the tool doesn't have access to the source code of the ProcessUserInfo method. As we see, the case is tricky, so we'd like to let you know about possible dangers beforehand.

Therefore, the analyzer will issue a warning where it will be able to do so. Which is, when analyzing the ProcessUserInfo method's source code. You will get the V5608 warning at the low certainty level.

If you'd like to rid yourself of such warnings, disable them using the comment //-V::5608:3 in the .pvsconfig file. This way, V5608 warnings (SQLI) of low certainty level won't be in the report. Read more about .pvsconfig files in the documentation (section "Suppression of false positives through diagnostic configuration files (.pvsconfig)").

On the other hand, you might want to set high certainty level for such warnings. If so, use the comment //V_LEVEL_1::5608. For details, see ''How to Set Your Level for Specific Diagnostics'' in the documentation.

#CSharp #Security