Taint tracking with C++ virtual tables

[nununoisy]

Description of the issue

I am trying to run the ExecTainted.ql query on some test C++ code and I am finding that the query is not finding the vulnerable code. I've posted the test code in a

I am expecting the taint to be tracked from std::fgets to a call to std::system within the base class. The derivative class uses std::strcat to assemble a potentially vulnerable command string. I did not use C++ strings since I have found that they are always lost in taint tracking, but that's a separate issue.

I have tried modifying some of the taint tracking QL, such as modifying ExecTainted.ql to use taintedIncludingGlobalVars, as well as modifying the DataFlowFunction overrides for some relevant C string functions. However, I was still unable to get CodeQL to return the intended result.

[geoffw0]

We should check that the models library models these std:: functions correctly - my hunch is that they're missing. I believe the limited ability to track taint through std::string is also caused by the models library at present (I believe it understands the std::string constructor and c_str(), but nothing else).

[nununoisy]

If the code in the Gist above is modified to include std::system(buf); before return 0;, the query does return an expected warning. If buf is cast to a C++ string and converted back to a C string with c_str(), no warning is generated. And in either case, no warning is generated for the call in BaseClass.

[nununoisy]

IIRC from reading the QL I did see references to the std:: namespace when searching for certain library functions. I suspect there are some minor semantics missing for c_str() and traversal through classes, though I'm not sure if it's classes as a whole or virtual functions with overrides in a derivative class.

[geoffw0]

You're right - std::gets, std::strcpy, std::strcat are all modelled and std::system is accepted by ExecTainted.ql. By the looks of it there may be an issue resolving one or both virtual calls - what is the result if you remove virtual from doSystemCall and/or initializeCommand?

[nununoisy]

Even if both virtual definitions are removed no warning is generated.

To remove the virtual from BaseClass::initializeCommand I replaced it's body with the body of DerivClass::initializeCommand to ensure the program behavior did not change.

[geoffw0]

OK, it looks like I'll have to set this up locally and take a closer look (thanks for providing very concise code that demonstrates the issue!)

[nununoisy]

No problem. Thanks for looking in to this for me.

[geoffw0]

Hi, our taint tracking has gone through numerous improvements since this issue was filed, including taint flow through std:: library functions inspired by this and similar tickets. On my local machine the code in the gist does produce a result now (on line 19) - so I think this is fixed, or will be in the next release.

Sorry for not getting back to you earlier.

[nununoisy]

Hey, no problem. Thanks for the response!