Security analysis of a repository pattern and asking ChatGPT about it.
Tags: programming, security, sast, csharp
Published: 2022-12-17 03:05:59

This week I discovered that SAST tools are struggling with a repository pattern.

So, let's say we have an `IRepo` interface with a single defined method.

We have 2 implementations: `Repo` and `DummyRepo`

```csharp
    public interface IRepo
    {
        void DoRepoStuff(string username, string name);
    }

    public class Repo : IRepo
    {
        public void DoRepoStuff(string username, string name)
        {
            string concatSql = "SELECT * FROM Accounts WHERE Username='" + username + "' OR  Name='" + name + "'";

            using (SqlConnection connection = new SqlConnection("dummyconnectionstring"))
            {
                SqlCommand concatSqlCommand = new SqlCommand()
                {
                    CommandText = concatSql,
                    CommandType = CommandType.Text,
                };

                concatSqlCommand.ExecuteReader();
            }
        }
    }

    public class DummyRepo : IRepo
    {
        public void DoRepoStuff(string username, string name)
        {
            return;
        }
    }
```

We declare instances somewhere.

```csharp
public IRepo _repoVulnerable = new Repo();

public IRepo _repoDummy = new DummyRepo();
```

**Test case 1:** Let's call a vulnerable method `DoRepoStuff` of `_repoVulnerable` instance. Note, that it is `IRepo`.

```csharp
public IActionResult SqlClientVulnerable(string username, string name)
    {
        _repoVulnerable.DoRepoStuff(username, name);
        return new OkResult();
    }
```

So we found a security vulnerability - SQL Injection, right?
Yes, but not all tools will report it.

**Test case 2:** Let's call a safe method `DoRepoStuff` of `_repoDummy` instance. Note, that it is `IRepo`.

```csharp
public IActionResult SqlClientDummySafe(string username, string name)
{
    _repoDummy.DoRepoStuff(username, name);
    return new OkResult();
}
```

We should not have a problem here, but some security tools will report security vulnerability by following the `IRepo` interface.

**Test case 3:**
Declare `_repoVulnerable` as `Repo` and `_repoDummy` as `DummyRepo`:

```csharp
public Repo _repoVulnerable = new Repo();

public DummyRepo _repoDummy = new DummyRepo();
```

It can help some tools to properly identify the true positive `_repoVulnerable` and do not trigger safe `_repoDummy` because no need to do a taint analysis by interface.

I can try to group security tools by behaviour.

1. Following interface implementations and marking all invocations vulnerable
1.1. If a tool displays only a single source, you may not see this behaviour
1.2. If a tool displays all sources that lead to the sink, you will most likely get a security vulnerability for the `Dummy` repo.

2. Not following interface
2.1. Do not display anything - no findings
2.2. Do not use Taint Analysis decreasing the confidence level.

Which tools are doing what.
1. Github Code Scanning will follow by interface, thus you will identify all true positives along with `dummy` false positives.
`Dummy` interface implementation simply returns `return` containing no security issues.

![Image description](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/qzyxethahzzn7yre0u2o.png)

2. SonarCloud will follow by interface but it shows a single source for a single sink. So a single finding.
Note that SonarCloud has 2 rules to detect SQL injections:
2.1. `S2077` doesn't use Taint Analysis, thus it is a security hotspot.
2.2. `S3649` use Taint Analysis, but like I mention it doesn't show all paths.

3. Security Code Scan is not following by interface, at least in NoAudit mode.

4. Semgrep has no clue about used sinks and shows nothing. Knowing its taint analysis limitation, it should find a known sink in the best case without providing the code flow.

5. Snyk Code use a single source to report a vulnerability, so it is hard to tell for sure if the `Dummy` stuff is in equation. It also decreases security severity of the finding to `Low` in my case.

![Image description](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/x6im9knqd41qw5bzaqr7.png)

Which is funny, because the same issue is reported as `High` without using interfaces. So, it is more about confidence then?

**Summary**
The repository pattern can be tricky for static code analyzers, be ready to do some manual review!

PS: I mentioned ChatGPT, so let's ask it.
**Why static code analyzers cannot properly check code that uses a repository pattern?**

**Answer:**
> Static code analyzers are tools that automatically scan source code to identify potential issues, such as bugs, security vulnerabilities, and coding errors. These tools are typically designed to work with a specific programming language and follow a set of rules and best practices for that language.

> One of the challenges of using static code analyzers with code that uses a repository pattern is that the repository pattern is a design pattern that is implemented in a variety of ways, and each implementation can have its own unique characteristics. Because static code analyzers are designed to work with specific programming languages and follow a set of rules, they may not be able to properly analyze code that uses a repository pattern if it deviates from those rules.

> Additionally, the repository pattern is often used to abstract away the details of how data is stored and retrieved, which can make it difficult for static code analyzers to understand the full context of the code and identify potential issues. For these reasons, static code analyzers may have difficulty properly checking code that uses a repository pattern.

Sounds smart but not a real answer. I consider it as a nice try!

My code repository is located [here](https://github.com/dbalikhin/SharpSaster).