[csharp] sql injection detection not working?

[indy-singh]

We are evaluating GHAS for our app sec pipeline and we can't seem to get it to flag for sql injection.

public void SubscribeTo(int systemKeyId, ThirdPartyType thirdParty, string userReference)
{

#pragma warning disable SYSLIB0021
#pragma warning disable SCS0010
// this is detected
  System.Security.Cryptography.SymmetricAlgorithm serviceProvider = new System.Security.Cryptography.DESCryptoServiceProvider(); 
#pragma warning restore SCS0010
#pragma warning restore SYSLIB0021

#pragma warning disable CS0618 // Type or member is obsolete
// none of these detected
  var adapterA = new SqlCommand("SELECT ITEM,PRICE FROM PRODUCT WHERE ITEM_CATEGORY='" + userReference + "' ORDER BY PRICE");
  var adapterB = new SqlCommand("SELECT ITEM,PRICE FROM PRODUCT WHERE ITEM_CATEGORY='" + userReference + "' ORDER BY PRICE", null);
  var adapterC = new SqlCommand("SELECT ITEM,PRICE FROM PRODUCT WHERE ITEM_CATEGORY='" + userReference + "' ORDER BY PRICE", null, null);
#pragma warning restore CS0618 // Type or member is obsolete

  using (var session = _pgDatabase.OpenSession())
  using (var transaction = session.BeginTransaction(System.Data.IsolationLevel.ReadCommitted))
  {
// this is not detected
    session.CreateSQLQuery(@$"insert into thirdpartymonitor.subscription(user_reference, thirdparty_id, system_id) VALUES ('{userReference}', {thirdParty}, {systemKeyId}) ON CONFLICT DO NOTHING;")
      .ExecuteUpdate();

    transaction.Commit();
  }
}

I would expect the SqlCommand to be detected as per https://github.com/github/codeql/blob/fe18e0e414daa3a5bf5b27ae6690e8c485850d44/csharp/ql/lib/ext/System.Data.SqlClient.model.yml#L6

But I'm not sure if CreateSQLQuery is detected as I don't see a sink for nhibernate.CreateSQLQuery?

I've attached my workflow file too.

codeql.yml

Cheers, Indy

[hvitved]

Hi

The parameter userReference is not actually user controllable, so this is why we do not flag it. See https://github.com/github/codeql/tree/main/csharp/ql/test/query-tests/Security%20Features/CWE-089 for some test examples.

[indy-singh]

Hi @hvitved,

Yeah that's what I suspected was happening, but this is an internal BE app and the incoming requests are all user controllable.

Can I instruct CodeQL to taint thing everything under a namespace (e.g. DataMining.Api.*)? So that method call is populated from this api request:-

using ProtoBuf;

namespace DataMining.Api.ThirdPartyMonitor.Types.Subscribe;

[ProtoContract(IsGroup = true)]
public sealed class SubscribeRequest : Codeweavers.Communication.IApiRequest
{
    [ProtoMember(1)]
    public Codeweavers.Common.Api.Types.User User { get; set; }

    [ProtoMember(3)]
    public Codeweavers.Common.Core.Types.ThirdPartyType ThirdParty { get; set; }

    [ProtoMember(4)]
    public string UserReference { get; set; }
}

Cheers, Indy

[indy-singh]

Kinda slowly piecing this together; by following some documentation.

So far I've managed to download the codeql database from the branch that has the test sql injection in.

I guess I need to find out how to query the namespace and then taint it somehow?

Cheers, Indy

[indy-singh]

Not making much progress with the api namespaces yet, I've got as far as this:-

import csharp

from Class c
where c.getFile().getAbsolutePath().matches("%/src/DataMining/app/Api%")
select c.getNamespace()

In the meantime, I tried another PoC:-

builder.UseEndpoints(x => x.MapGet("services/application.svc/thread-pool/{authenticationToken}", (string authenticationToken) =>
{
  System.Security.Cryptography.SymmetricAlgorithm serviceProvider = new System.Security.Cryptography.DESCryptoServiceProvider();

  var adapterA = new SqlCommand("SELECT ITEM,PRICE FROM PRODUCT WHERE ITEM_CATEGORY='" + authenticationToken + "' ORDER BY PRICE");
  var adapterB = new SqlCommand("SELECT ITEM,PRICE FROM PRODUCT WHERE ITEM_CATEGORY='" + authenticationToken + "' ORDER BY PRICE", null);
  var adapterC = new SqlCommand("SELECT ITEM,PRICE FROM PRODUCT WHERE ITEM_CATEGORY='" + authenticationToken + "' ORDER BY PRICE", null, null);

  using (var session = PostgresDatabase.Instance.OpenSession())
  using (var transaction = session.BeginTransaction(System.Data.IsolationLevel.ReadCommitted))
  {
    session.CreateSQLQuery(@$"insert into thirdpartymonitor.subscription(user_reference, thirdparty_id, system_id) VALUES ('{authenticationToken}', 1, 1) ON CONFLICT DO NOTHING;")
      .ExecuteUpdate();

    transaction.Commit();
  }

  return application.ThreadPool();
}));

None of those are detected?

[indy-singh]

Okay so I guess nibernate for c# isn't modelled inside of CodeQL:-

And I guess it doens't know about minimal api's yet, hence it didn't detect anything inside of MapGet from my earlier example.

using Microsoft.AspNetCore.Mvc;
using System.Data.SqlClient;
using DataMining.Data.Database;

namespace DataMining.WebApp;

[Route("api/[controller]")]
[ApiController]
public class ValuesController : ControllerBase
{
  // POST api/<ValuesController>
  [HttpPost]
  public void Post([FromBody] string value)
  {
// detected
    System.Security.Cryptography.SymmetricAlgorithm serviceProvider = new System.Security.Cryptography.DESCryptoServiceProvider();
// detected
    var adapterA = new SqlCommand("SELECT ITEM,PRICE FROM PRODUCT WHERE ITEM_CATEGORY='" + value + "' ORDER BY PRICE");
// detected
    var adapterB = new SqlCommand("SELECT ITEM,PRICE FROM PRODUCT WHERE ITEM_CATEGORY='" + value + "' ORDER BY PRICE", null);
// detected
    var adapterC = new SqlCommand("SELECT ITEM,PRICE FROM PRODUCT WHERE ITEM_CATEGORY='" + value + "' ORDER BY PRICE", null, null);

    using (var session = PostgresDatabase.Instance.OpenSession())
    using (var transaction = session.BeginTransaction(System.Data.IsolationLevel.ReadCommitted))
    {
// NOT DETECTED
      session.CreateSQLQuery(@$"insert into thirdpartymonitor.subscription(user_reference, thirdparty_id, system_id) VALUES ('{value}', 1, 1) ON CONFLICT DO NOTHING;")
        .ExecuteUpdate();

      transaction.Commit();
    }
  }

  // PUT api/<ValuesController>/5
  [HttpPut("{id}")]
  public void Put(int id, [FromBody] string value)
  {
// detected
    System.Security.Cryptography.SymmetricAlgorithm serviceProvider = new System.Security.Cryptography.DESCryptoServiceProvider();
// detected
    var adapterA = new SqlCommand("SELECT ITEM,PRICE FROM PRODUCT WHERE ITEM_CATEGORY='" + value + "' ORDER BY PRICE");
// detected
    var adapterB = new SqlCommand("SELECT ITEM,PRICE FROM PRODUCT WHERE ITEM_CATEGORY='" + value + "' ORDER BY PRICE", null);
// detected
    var adapterC = new SqlCommand("SELECT ITEM,PRICE FROM PRODUCT WHERE ITEM_CATEGORY='" + value + "' ORDER BY PRICE", null, null);

    using (var session = PostgresDatabase.Instance.OpenSession())
    using (var transaction = session.BeginTransaction(System.Data.IsolationLevel.ReadCommitted))
    {
// NOT DETECTED
      session.CreateSQLQuery(@$"insert into thirdpartymonitor.subscription(user_reference, thirdparty_id, system_id) VALUES ('{value}', 1, 1) ON CONFLICT DO NOTHING;")
        .ExecuteUpdate();

      transaction.Commit();
    }
  }
}

[indy-singh]

Alright, using https://github.com/GitHubSecurityLab/CodeQL-Community-Packs/blob/main/csharp/ext-library-sources/generated/Npgsql.model.yml as a baseline example. I've managed to get it to detect NHibernate.ISession.CreateSQLQuery in the Values Controller. That ticks off that problem.

The remaining problem is how do I get CodeQL to recognise that everything under {{APPNAME}}.Api namespace should be tainted (e.g. it is user controllable).

extensions:
  - addsTo:
      pack: codeql/csharp-all
      extensible: sinkModel
    data:
      - ["NHibernate", "ISession", True, "CreateSQLQuery", "(System.String)", "", "Argument[0]", "sql-injection", "manual"]

import csharp
import semmle.code.csharp.security.dataflow.SqlInjectionQuery
import SqlInjection::PathGraph
import semmle.code.csharp.security.dataflow.flowsources.FlowSources

string getSourceType(DataFlow::Node node) { result = node.(SourceNode).getSourceType() }

from SqlInjection::PathNode source, SqlInjection::PathNode sink
where SqlInjection::flowPath(source, sink)
select sink.getNode(), source, sink, "This query AAAAAAAAA depends on $@.", source,
  ("this " + getSourceType(source.getNode()))

[hvitved]

Thanks for identifying the lacking CreateSQLQuery sink; this is definitely something we should add.

Regarding the missing taint sources, I wonder if the right thing would be to mark all accesses to fields with the ProtoMember attribute as tainted, or whether that is too broad?

[indy-singh]

@hvitved Thanks! That is really helpful. I did try and create a local model pack (e.g. they are local to the repo, but not published), but while I can get it working in vscode + codeql (as per my screenshot) I've had no luck in getting it registered in Actions:-

      - uses: github/codeql-action/init@v4
        with:
          languages: csharp
          build-mode: none
          packs: ./codeql-custom-queries-csharp/qlpack.yml # I've tried a lot of variations of this line
          debug: true
          config: |
            packs:-
              - ./codeql-custom-queries-csharp/qlpack.yml # I've tried a lot of variations of this line
            paths: 
              - '/src/DataMining/**'
            paths-ignore:
              - '/src/DataMining/test/**'
        env:
          GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}

Where qlpack.yml looks like this:-

---
library: false
warnOnImplicitThis: false
name: codeql-extra-queries-csharp
version: 1.0.0
dependencies:
  codeql/csharp-all: ^5.4.2
dataExtensions:
  - models/**/*.model.yml

And models/NHibernate.model.yml looks like:-

extensions:
  - addsTo:
      pack: codeql/csharp-all
      extensible: sinkModel
    data:
      - ["NHibernate", "ISession", True, "CreateSQLQuery", "(System.String)", "", "Argument[0]", "sql-injection", "manual"]

Regarding the missing taint sources, I wonder if the right thing would be to mark all accesses to fields with the ProtoMember attribute as tainted, or whether that is too broad?

I think that would work as a first pass; but in reality that is just a marker for our serlization layer, it we wanted to change out ProtoBuf for SomethingNewAndCool, I don't want to have to update the codeql definition.

Is it not possible to mark everything under a namespace as tainted? Or are their performances considerations to be concerned about?

Cheers, Indy

[hvitved]

Is it not possible to mark everything under a namespace as tainted? Or are their performances considerations to be concerned about?

There can certainly be performance issues with that, depending on exactly what it means. Do you mean any parameter of any method in the given namespace?

[indy-singh]

No, I mean any property/field of any class in a namespace. So to give a naïve example:-

using Codeweavers.Communication;
using ProtoBuf;

namespace Organami.Api.v1.Types;

[ProtoContract(IsGroup = true)]
public class LoginRequest : IApiRequest
{
    [ProtoMember(2)]
    public string Username { get; set; } // we should taint this

    [ProtoMember(3)]
    public string Password { get; set; } // we should taint this
}

I'd got as far as getting all the class with types in folder called "/api/". But I guess to be more strict; we want to get the fully qualified namespace (e.g. appname.api.*) and then traverse all the class defintions and mark each property/field as tainted

Cheers, Indy

[hvitved]

Something like this should work:

private class AdHocRemoteFlowSource extends RemoteFlowSource {
  AdHocRemoteFlowSource() {
    exists(AssignableMember m |
      m.getDeclaringType().getNamespace().getParentNamespace*().getFullName() = "appname.api" and
      this.asExpr() = m.getAnAccess()
    )
  }

  override string getSourceType() { result = "ad hoc remote flow source" }
}

[indy-singh]

Oooo, yeah that is still running on my machine (30+ minutes).

What about ditching the namespace idea, and using the physical file path?

import csharp
import semmle.code.csharp.security.dataflow.SqlInjectionQuery
import SqlInjection::PathGraph
import semmle.code.csharp.security.dataflow.flowsources.FlowSources

/** Restricts analysis to files within the DataMining directory */
predicate isInDataMiningPath(Element e) {
  e.getLocation().getFile().getAbsolutePath().matches("%/src/DataMining/app/Api%")
}

private class AdHocRemoteFlowSource extends RemoteFlowSource {
  AdHocRemoteFlowSource() {
    exists(Property p |
      // Physical path check for faster filtering
      isInDataMiningPath(p) and
      // Namespace check for precision
      this.asExpr() = p.getAnAccess()
    )
  }

  override string getSourceType() { result = "ad hoc remote flow source" }
}

Cheers, Indy