Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

C++: Adding a model implementation for ODBC. #14647

Merged

Conversation

bdrodes
Copy link
Contributor

@bdrodes bdrodes commented Oct 31, 2023

Adds a model implementation for ODBC database APIs.

@bdrodes bdrodes requested a review from a team as a code owner October 31, 2023 15:59
@github-actions github-actions bot added the C++ label Oct 31, 2023
@owen-mc owen-mc changed the title Adding a model implementation for ODBC. C++: Adding a model implementation for ODBC. Oct 31, 2023
Copy link
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of comments.

Additionally, would it be possible to add some tests to the end of the https://github.com/github/codeql/blob/main/cpp/ql/test/query-tests/Security/CWE/CWE-089/SqlTainted/test.c file? Or if you prefer to the C++ file: https://github.com/github/codeql/blob/main/cpp/ql/test/query-tests/Security/CWE/CWE-089/SqlTainted/test.cpp

Basically, we just want a test that confirms that we can track tainted flow coming into the sink.

@@ -9,6 +9,7 @@ private import implementations.Iterator
private import implementations.MemberFunction
private import implementations.Memcpy
private import implementations.Memset
private import implementations.ODBC
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should move it down to the other SQL-like imports?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

cpp/ql/lib/semmle/code/cpp/models/implementations/ODBC.qll Outdated Show resolved Hide resolved
@MathiasVP
Copy link
Contributor

DCA looks good: performance is unaffected, and we're finding 4 new results that seem genuine. So I think this PR is good to go once the above comments have been addressed 🎉

@bdrodes
Copy link
Contributor Author

bdrodes commented Nov 2, 2023

Thanks. I'll look over the change suggestions shortly.

@bdrodes
Copy link
Contributor Author

bdrodes commented Nov 2, 2023

@MathiasVP, I've made the suggested changes and added a quick ODBC sql injection test. Let me know if there is anything else need to close this out. Thanks

@MathiasVP
Copy link
Contributor

Looks good. Thanks!

The only thing we need now is to make the autoformatter happy. CI tells me that the ODBC.qll file needs to be formatted correctly.

You can do this using codeql query format -i path-to-the-file to format it in-place. Alternatively, you can do it via the Format Document command in VSCode.

@bdrodes
Copy link
Contributor Author

bdrodes commented Nov 2, 2023

Looks good. Thanks!

The only thing we need now is to make the autoformatter happy. CI tells me that the ODBC.qll file needs to be formatted correctly.

You can do this using codeql query format -i path-to-the-file to format it in-place. Alternatively, you can do it via the Format Document command in VSCode.

Whoops. I did this on my initial push but forgot to do it for the recent changes. It should be all good now.

* The other source of input to a `SQLExecute` is via a `SQLBindParameter`, which sanitizes user input,
* and would be considered a barrier to SQL injection.
*/
private class ODBCExecutionFunction extends SqlExecutionFunction {

Check warning

Code scanning / CodeQL

Acronyms should be PascalCase/camelCase. Warning

Acronyms in ODBCExecutionFunction should be PascalCase/camelCase.
Copy link
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@MathiasVP MathiasVP merged commit 679d64f into github:main Nov 2, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants