CodeQL queries 1.25
Skip to end of metadata
Go to start of metadata

Name: Serialization check bypass

Description: A write that looks like it may be bypassing runtime checks.

ID: cs/serialization-check-bypass

Kind: problem

Severity: warning

Precision: medium

Query: RuntimeChecksBypass.ql
/**
 * @name Serialization check bypass
 * @description A write that looks like it may be bypassing runtime checks.
 * @kind problem
 * @id cs/serialization-check-bypass
 * @problem.severity warning
 * @precision medium
 * @tags security
 *       external/cwe/cwe-20
 */

import semmle.code.csharp.serialization.Serialization
import semmle.code.csharp.controlflow.Guards
import semmle.code.csharp.dataflow.DataFlow

/**
 * The result is a write to the field `f`, assigning it the value
 * of variable `v` which was checked by the condition `check`.
 */
GuardedExpr checkedWrite(Field f, Variable v, IfStmt check) {
  result = v.getAnAccess() and
  result = f.getAnAssignedValue() and
  check.getCondition().getAChildExpr*() = result.getAGuard(_, _)
}

/**
 * The result is an unsafe write to the field `f`, where
 * there is no check performed within the (calling) scope of the method.
 */
Expr uncheckedWrite(Callable callable, Field f) {
  result = f.getAnAssignedValue() and
  result.getEnclosingCallable() = callable and
  not callable.calls*(checkedWrite(f, _, _).getEnclosingCallable()) and
  // Exclude object creations because they were not deserialized
  not exists(Expr src | DataFlow::localExprFlow(src, result) |
    src instanceof ObjectCreation or src.hasValue()
  )
}

from BinarySerializableType t, Field f, IfStmt check, Expr write, Expr unsafeWrite
where
  f = t.getASerializedField() and
  write = checkedWrite(f, t.getAConstructor().getAParameter(), check) and
  unsafeWrite = uncheckedWrite(t.getADeserializationCallback(), f)
select unsafeWrite, "This write to $@ may be circumventing a $@.", f, f.toString(), check, "check"

Fields that are deserialized should be validated, otherwise the deserialized object could contain invalid data.

This query finds cases where a field is validated in a constructor, but not in a deserialization method. This is an indication that the deserialization method is missing a validation step.

Recommendation

If a field needs to be validated, then ensure that validation is also performed during deserialization.

Example

The following example has the validation of the Age field in the constructor but not in the deserialization method:

using System;
using System.Runtime.Serialization;

[Serializable]
public class PersonBad : ISerializable
{
    public int Age;

    public PersonBad(int age)
    {
        if (age < 0)
            throw new ArgumentException(nameof(age));
        Age = age;
    }

    [OnDeserializing]
    void ISerializable.GetObjectData(SerializationInfo info, StreamingContext context)
    {
        Age = info.GetInt32("age");  // BAD - write is unsafe
    }
}

The problem is fixed by adding validation to the deserialization method as follows:

using System;
using System.Runtime.Serialization;

[Serializable]
public class PersonGood : ISerializable
{
    public int Age;

    public PersonGood(int age)
    {
        if (age < 0)
            throw new ArgumentException(nameof(age));
        Age = age;
    }

    [OnDeserializing]
    void ISerializable.GetObjectData(SerializationInfo info, StreamingContext context)
    {
        int age = info.GetInt32("age");
        if (age < 0)
            throw new SerializationException(nameof(Age));
        Age = age;  // GOOD - write is safe
    }
}

References