C++: Restrict sinks in cpp/cleartext-storage-database
#12469
Conversation
…abase'. This reduces the number of sinks considered on the 'sysown/proxysql' from > 62000 sinks to ~1000 sinks.
…om the sink definition to the source definition. The dataflow library starts tracking flow from the sources, so it's better to to rule out the entire database in the source definition than in the sink definition.
…ready be considering flow starting at those sources) and out of sinks (since we'll already be alerting on this sink if it's relevant).
MathiasVP
added
the
depends on internal PR
|
@geoffw0 I'd like to hear your input on this one as well. Did I capture all the relevant functions as sinks? |
There was a problem hiding this comment.
LGTM. There is a risk that we're loosing support for other sqlite interfaces besides the common one, which sqlite% may well have matched by semi-coincidence whereas the more specific modelling will not. On the flip side, we lose false positives like you were seeing this morning. I'm happy with the change.
| .getName() | ||
| .matches([ | ||
| "sqlite3_bind_blob", "sqlite3_bind_blob64", "sqlite3_bind_text", "sqlite3_bind_text16", | ||
| "sqlite3_bind_text64", "sqlite3_bind_value", "sqlite3_bind_pointer" |
There was a problem hiding this comment.
I would personally preserve modelling of sqlite3_bind_int64 etc as well here, though I'm not sure they have much value and are potentially more likely to lead to false positives???
There was a problem hiding this comment.
Yeah, I wasn't really sure about the integer versions. Since the sources for this query is "sensitive data" I can't really imagine why sensitive data would end up in an integer (in a way that reveals any sensitive data).
There was a problem hiding this comment.
Well in principle you can store a bank account number as a uint64 or something. I hope it's uncommon.
MathiasVP
merged commit d89b8ba
into
github:mathiasvp/replace-ast-with-ir-use-usedataflow
This PR reduces the set of sinks considered in the
cpp/cleartext-storage-databasequery. Previously we used "all functions prefixed withsqlite", and after this PR it's confined to a specific set of functions. This fixes a performance issue I was seeing on a database that had a particularly bad naming convention of prefixing their own functions withsqlite😂.I've added two additional commits that each should provide a small performance boost.