Add Column, From, JoinClause, *Join, Having, OrderByClause methods for squirrel SQLi query

[lyoung-confluent]

Changes

Building on @tunnelshade's work in #611, add a few additional methods with similar signatures/issues.
These are vulnerable sinks if the first parameter is tainted input.
Some of these functions accept interface{} type in which case only string type (StringType) is tainted/vulnerable.

Links

• SelectBuilder.Column
• SelectBuilder.From
• SelectBuilder.JoinClause
• SelectBuilder.LeftJoin
• SelectBuilder.RightJoin
• SelectBuilder.InnerJoin
• SelectBuilder.CrossJoin
• SelectBuilder.Having
• SelectBuilder.OrderByClause

[smowton]

I don't have detailed knowledge of the package to know whether the Having and Clause methods perform sufficient escaping of their arguments to prevent a meaningful injection attack: have you tested that they really are exploitable?

[lyoung-confluent]

I don't have detailed knowledge of the package to know whether the Having and Clause methods perform sufficient escaping of their arguments to prevent a meaningful injection attack: have you tested that they really are exploitable?

Yes, demo: https://go.dev/play/p/7Ipxor9Z029

Having actually uses the exact same newWhereClause as Where does internally so is vulnerable in the same way: https://github.com/Masterminds/squirrel/blob/v1.5.2/select.go#L351

OrderByClause uses newPart which directly returns the pred (first argument) when ToSQL is invoked if it is a string: https://github.com/Masterminds/squirrel/blob/def598cbb358368fbfc3f6a9a914699a36846992/part.go#L24

[smowton]

👍 ping me when this is in a ready state

[lyoung-confluent]

I added "Join", "LeftJoin", "RightJoin", "InnerJoin", "CrossJoin" as well as these functions are all aliases of JoinClause with a small prefix added, ex: https://github.com/Masterminds/squirrel/blob/v1.5.2/select.go#L292

Also From which uses newPart as well: https://github.com/Masterminds/squirrel/blob/v1.5.2/select.go#L275
And finally Columns which also uses newPart: https://github.com/Masterminds/squirrel/blob/v1.5.2/select.go#L260

[lyoung-confluent]

@smowton I believe it's ready for a round of review/merge (assuming tests pass, currently blocked on github actions being started)

[owen-mc]

CI is currently failing when trying to extract ql/test/library-tests/semmle/go/frameworks/SQL/. You need to update the version of Squirrel in ql/test/library-tests/semmle/go/frameworks/SQL/vendor/modules.txt to match the corresponding go.mod. With that change, the tests fail because lines 46-58 of ql/test/library-tests/semmle/go/frameworks/SQL/main.go should have the comment // $ querystring=querypart querystring="users".

[smowton]

Noting this was merged accidentally due to changes to the automerge criteria and then reverted in #748