C++: Allow MaD barriers

[owen-mc]

The first commit was done by Opus 4.5. I then rebased it after C++: Support models-as-data barriers and barrier guards was merged and deleted most of its work.

[Copilot]

Pull request overview

This PR enables Models-as-Data (MaD) barriers for SQL injection detection in C/C++ code. The change allows SQL injection barriers to be defined using the extensible Models-as-Data framework, complementing the existing hardcoded barrier for integral types.

Changes:

• Added MaD barrier support to the SQL injection query's isBarrier predicate

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[jketema]

It seems this might also need a test?

[owen-mc]

@jketema The PR that added the ability to specify barrier nodes added a test of that (cpp/ql/test/library-tests/dataflow/ir-barrier-guards). So I feel as if the functionality is tested.

With the other languages I've done this for, there have been existing sanitizers that I can convert, so that has acted as a kind of test for these extension points. Now that I look closer, I see that there are two classes extending SqlBarrierFunction that could possibly be converted to MaD. However, I'm not really familiar enough with our C++ library, or how it implements MaD, to do that easily. Do you think you could do it?

[jketema]

But that ir barrier guard test does not exercise the change made here, or does it?

[jketema]

However, I'm not really familiar enough with our C++ library, or how it implements MaD, to do that easily. Do you think you could do it?

I can have a look.

[jketema]

Looks like the MySql one should be easy to rewrite. However, it seems there is something a bit off with the query. The use of isBarrierIn seems incorrect. It seems that all that code needs to be in isBarrier (which it was at some point).

It also seems we have no test cases for the MySql barriers. We have internal tests.

I'll try fixing those two issues first.

[jketema]

This may be C++ specific, but how do MaD summary models and MaD barrier models interact?

I tried to rewrite the MySQL barrier model to use MaD, but realized that the tests weren't triggering an injection without the model either, because we don't model flow through the escaping functions. So, I though I'd add both a summary and a barrier model:

# partial model of the MySQL api
extensions:
  - addsTo:
      pack: codeql/cpp-all
      extensible: summaryModel
    data: # namespace, type, subtypes, name, signature, ext, input, output, kind, provenance
      - ["", "", False, "mysql_real_escape_string", "", "", "Argument[*2]", "Argument[*1]", "taint", "manual"]
      - ["", "", False, "mysql_real_escape_string_quote", "", "", "Argument[*2]", "Argument[*1]", "taint", "manual"]
  - addsTo:
      pack: codeql/cpp-all
      extensible: barrierModel
    data: # namespace, type, subtypes, name, signature, ext, input, kind, provenance
      - ["", "", False, "mysql_real_escape_string", "", "", "Argument[*2]", "sql-injection", "manual"]
      - ["", "", False, "mysql_real_escape_string_quote", "", "", "Argument[*2]", "sql-injection", "manual"]

Quick-eval'ing isBarrier with you change, I see a results for mysql_real_escape_string calls (I obviously removed the relevant bits of MqSql.qll first). However, the internal queries that test this still give me injection alerts. It somehow seems like the taint model takes preference over the barrier model?

WIP branch: https://github.com/github/codeql/tree/jketema/sql-barrier

[jketema]

it seems there is something a bit off with the query. The use of isBarrierIn seems incorrect. It seems that all that code needs to be in isBarrier (which it was at some point).

@owen-mc I merged a fix for this.

[MathiasVP]

Oh! I know why this doesn't work. It comes from a misunderstand of the "Argument" column of the new barrier MaD models.

Looking at the shared code for how these rows are interpreted we see that the column @jketema specified is interpreted as an "output" column and not an "input" column (let's ignore C++ specific details such as indirections for this discussion).

This means that for a call such as mysql_real_escape_string(mySql, to, from, length) the barrier specified by the new MaD barrier model places the barrier on from [post update] and not on from.

However, the summary specified for mysql_real_escape_string gives flow from from to to [post update]. So as the node from [post update] is never part of the dataflow path the barrier doesn't do anything.

If you place the barrier on Argument[*1] instead I expect that things will Just Work.

I think this is a mistake in the design of the new MaD rows. It should be possible to place a barrier not only on output columns, but also on input columns ... but I can also see the benefit of this simple model (i.e., "it's always interpreted as an output"), but it should probably be documented.

[jketema]

🤦‍♂️ I think using the output argument (1st argument) is fine here. I think I just got confused by a bit too many copy-and-pastes. And I also got confused by the MySql.qll code using both the input and the output.

[jketema]

@owen-mc I've opened a PR against your branch here: owen-mc#5

[jketema]

Assuming Owen looked my changes over, this LGTM. I'm currently still running DCA, so want to hold off merging until that comes back ok.

[owen-mc]

Yes, I reviewed your changes. Does Argument[*1] mean "the thing pointed to by Argument[1]"? We have very occsaional need for expressing that in Go models and I've picked a different syntax (Argument[1].Dereference), but it would probably be better to be consistent with C++.

[jketema]

Does Argument[*1] mean "the thing pointed to by Argument[1]"?

Correct. Note that multiple * can occur, and also an @ symbol. The latter indicates an arbitrary under of indirections (deferences) up to a certain maximum hard coded in the dataflow library.

[jketema]

DCA was uneventful.