C++: SqlPqxxTainted query searches for sql injections via pqxx connector to postgres

[japroc]

Hello!

The main goal of my PR it to add support of pqxx connector.
Right now there are only MyQSL and SQlite support. And it is based solely on function name.
Pqxx sink is a bit more complex, because functions like "exec" may lead to high false positive rate.

I basically reused existing SqlTainted query, and created a custom SqlPqxxTainted query. I think that pqxx support should be implemented in Security.qll, but i seems not so simple to overwrite the logic behind SqlTainted query and it's Taint Configuration. That is why i decides to create separate query. I have also added example file SqlPqxxTainted.c and SqlPqxxTainted.qhelp which almost completely repeats SqlTainted.qhelp.

[MathiasVP]

Hi @japroc,

Thank you for your contribution. We will find a reviewer for your query soon :)

I took a quick and very high-level look at your query, and I think it looks good. One thing you need to change is to give your query a unique @id (as the one you've picked currently conflicts with the SqlTainted query). Perhaps cpp/sql-injection-via-pqxx?

[japroc]

Nice, thanks!

I also want to mention that i dont check namespace. It thought that this operation may be redundant.
You can see that second parameters of predicates calls pqxxTransationClassNames and pqxxConnectionClassNames are blank. I can add namespace check by adding t.getParentScope().(Namespace).getName() instead of _.

[jbj]

Thank you for the submission. I expect to review this PR next week.

[MathiasVP]

Thanks for your contribution, @aproc!

The PR looks super good, so I hope it's okay I commented on a few stylistic things. When these are fixed I'll happily merge your contribution :)

[MathiasVP]

We use camelCalse for variables in QL.

[japroc]

Thanks for your contribution, @japroc!

The PR looks super good, so I hope it's okay I commented on a few stylistic things. When these are fixed I'll happily merge your contribution :)

Hi, @MathiasVP!

Nice! I have made changes suggested by you.
What do you think about checking namespace. I could add t.getParentScope().(Namespace).getName() in getPqxxSqlArgument function and isEscapedPqxxArgument predicate. There are some name collisions possible. If you think it's redundant enhancement, we could skip this step and go on.

Thanks!

[MathiasVP]

What do you think about checking namespace. I could add t.getParentScope().(Namespace).getName() in getPqxxSqlArgument function and isEscapedPqxxArgument predicate. There are some name collisions possible. If you think it's redundant enhancement, we could skip this step and go on.

Good point. I think it's a good idea to check for the namespace. Your solution sounds fine, but you might be able to simplify the implementation if you change the type of t in getPqxxSqlArgument from Type to Struct. Then you can use the getNamespace() predicate and avoid dealing with parent scopes.

[japroc]

What do you think about checking namespace. I could add t.getParentScope().(Namespace).getName() in getPqxxSqlArgument function and isEscapedPqxxArgument predicate. There are some name collisions possible. If you think it's redundant enhancement, we could skip this step and go on.

Good point. I think it's a good idea to check for the namespace. Your solution sounds fine, but you might be able to simplify the implementation if you change the type of t in getPqxxSqlArgument from Type to Struct. Then you can use the getNamespace() predicate and avoid dealing with parent scopes.

Struct type does not fit this case. Because we lose work which is typedef transaction<> work;. But UserType which is indirect superclass of Struct works good.

[MathiasVP]

LGTM!