Thijs/wiki.techinc.nl
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Add taint-check annotations to database methods

Browse source 
These were copied verbatim from MediaWikiSecurityCheckPlugin.php.

The exception is selectRowCount(), whose return value was hardcoded as
tainted in taint-check, but that's clearly not the case because it
returns just an integer.

Suppress a new issue in DbTestPreviewer that wasn't previously spotted
because the code ends up using IReadableDatabase::selectRow, but the
hardcoded data in taint-check only considers IDatabase and concrete
classes, not IReadableDatabase.

Bug: T321806
Change-Id: I0c59d286c50de4ea79571242649bdda8aec67b57
This commit is contained in:
Daimona Eaytoy 2023-09-23 19:06:11 +02:00
parent ebb742d04f
commit d31d5bfa92
6 changed files with 72 additions and 2 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
includes/libs/rdbms/database/DbQuoter.php
View file

@ -11,7 +11,9 @@ interface DbQuoter {
* Escape and quote a raw value string for use in a SQL query
*
* @param string|int|float|null|bool|Blob $s
* @param-taint $s escapes_sql
* @return string
* @return-taint none
*/
public function addQuotes( $s );
}

17
includes/libs/rdbms/database/IDatabase.php
View file

@ -273,10 +273,12 @@ interface IDatabase extends IReadableDatabase {
* Methods like startAtomic(), endAtomic(), and cancelAtomic() can be used instead.
*
* @param string|Query $sql Single-statement SQL query
* @param-taint $sql exec_sql
* @param string $fname Caller name; used for profiling/SHOW PROCESSLIST comments
* @param int $flags Bit field of IDatabase::QUERY_* constants.
* @return bool|IResultWrapper True for a successful write query, IResultWrapper object
* for a successful read query, or false on failure if QUERY_SILENCE_ERRORS is set.
* @return-taint tainted
* @throws DBQueryError If the query is issued, fails, and QUERY_SILENCE_ERRORS is not set.
* @throws DBExpectedError If the query is not, and cannot, be issued yet (non-DBQueryError)
* @throws DBError If the query is inherently not allowed (non-DBExpectedError)
@ -356,19 +358,24 @@ interface IDatabase extends IReadableDatabase {
* @internal callers outside of rdbms library should use InsertQueryBuilder instead.
*
* @param string $table Table name
* @param-taint $table exec_sql
* @param array|array[] $rows Row(s) to insert, as either:
* - A string-keyed map of (column name => value) defining a new row. Values are
* treated as literals and quoted appropriately; null is interpreted as NULL.
* - An integer-keyed list of such string-keyed maps, defining a list of new rows.
* The keys in each map must be identical to each other and in the same order.
* The rows must not collide with each other.
* @param-taint $rows exec_sql_numkey - NOTE: This does not work when inserting multiple rows (T290563)
* @param string $fname Calling function name (use __METHOD__) for logs/profiling
* @param-taint $fname exec_sql
* @param string|array $options Combination map/list where each string-keyed entry maps
* a non-boolean option to the option parameters and each integer-keyed value is the
* name of a boolean option. Supported options are:
* - IGNORE: Boolean: skip insertion of rows that would cause unique key conflicts.
* IDatabase::affectedRows() can be used to determine how many rows were inserted.
* @param-taint $options exec_sql
* @return bool Return true if no exception was thrown (deprecated since 1.33)
* @return-taint none
* @throws DBError If an error occurs, {@see query}
*/
public function insert( $table, $rows, $fname = __METHOD__, $options = [] );
@ -382,6 +389,7 @@ interface IDatabase extends IReadableDatabase {
* @internal callers outside of rdbms library should use UpdateQueryBuilder instead.
*
* @param string $table Table name
* @param-taint $table exec_sql
* @param array $set Combination map/list where each string-keyed entry maps a column
* to a literal assigned value and each integer-keyed value is a SQL expression in the
* format of a column assignment within UPDATE...SET. The (column => value) entries are
@ -389,17 +397,22 @@ interface IDatabase extends IReadableDatabase {
* assignment format is useful for updates like "column = column + X". All assignments
* have no defined execution order, so they should not depend on each other. Do not
* modify AUTOINCREMENT or UUID columns in assignments.
* @param-taint $set exec_sql_numkey
* @param array|string $conds Condition in the format of IDatabase::select() conditions.
* In order to prevent possible performance or replication issues or damaging a data
* accidentally, an empty condition for 'update' queries isn't allowed.
* IDatabase::ALL_ROWS should be passed explicitly in order to update all rows.
* @param-taint $conds exec_sql_numkey
* @param string $fname Calling function name (use __METHOD__) for logs/profiling
* @param-taint $fname exec_sql
* @param string|array $options Combination map/list where each string-keyed entry maps
* a non-boolean option to the option parameters and each integer-keyed value is the
* name of a boolean option. Supported options are:
* - IGNORE: Boolean: skip update of rows that would cause unique key conflicts.
* IDatabase::affectedRows() can be used to determine how many rows were updated.
* @param-taint $options none
* @return bool Return true if no exception was thrown (deprecated since 1.33)
* @return-taint none
* @throws DBError If an error occurs, {@see query}
*/
public function update( $table, $set, $conds, $fname = __METHOD__, $options = [] );
@ -542,12 +555,16 @@ interface IDatabase extends IReadableDatabase {
* @internal callers outside of rdbms library should use DeleteQueryBuilder instead.
*
* @param string $table Table name
* @param-taint $table exec_sql
* @param string|array $conds Array of conditions. See $conds in IDatabase::select()
* In order to prevent possible performance or replication issues or damaging a data
* accidentally, an empty condition for 'delete' queries isn't allowed.
* IDatabase::ALL_ROWS should be passed explicitly in order to delete all rows.
* @param-taint $conds exec_sql_numkey
* @param string $fname Name of the calling function
* @param-taint $fname exec_sql
* @return bool Return true if no exception was thrown (deprecated since 1.33)
* @return-taint none
* @throws DBError If an error occurs, {@see query}
*/
public function delete( $table, $conds, $fname = __METHOD__ );

35
includes/libs/rdbms/database/IReadableDatabase.php
View file

@ -164,14 +164,21 @@ interface IReadableDatabase extends ISQLPlatform, DbQuoter, IDatabaseFlags {
* @internal callers outside of rdbms library should use SelectQueryBuilder instead.
*
* @param string|array $table Table name. {@see select} for details.
* @param-taint $table exec_sql
* @param string|array $var The field name to select. This must be a valid SQL fragment: do not
* use unvalidated user input. Can be an array, but must contain exactly 1 element then.
* {@see select} for details.
* @param-taint $var exec_sql
* @param string|array $cond The condition array. {@see select} for details.
* @param-taint $cond exec_sql_numkey
* @param string $fname The function name of the caller.
* @param-taint $fname exec_sql
* @param string|array $options The query options. {@see select} for details.
* @param-taint $options none This is special-cased in MediaWikiSecurityCheckPlugin
* @param string|array $join_conds The query join conditions. {@see select} for details.
* @param-taint $join_conds none This is special-cased in MediaWikiSecurityCheckPlugin
* @return mixed|false The value from the field, or false if nothing was found
* @return-taint tainted
* @throws DBError If an error occurs, {@see query}
*/
public function selectField(
@ -189,14 +196,21 @@ interface IReadableDatabase extends ISQLPlatform, DbQuoter, IDatabaseFlags {
* @internal callers outside of rdbms library should use SelectQueryBuilder instead.
*
* @param string|array $table Table name. {@see select} for details.
* @param-taint $table exec_sql
* @param string $var The field name to select. This must be a valid SQL
* fragment: do not use unvalidated user input.
* @param-taint $var exec_sql
* @param string|array $cond The condition array. {@see select} for details.
* @param-taint $cond exec_sql_numkey
* @param string $fname The function name of the caller.
* @param-taint $fname exec_sql
* @param string|array $options The query options. {@see select} for details.
* @param-taint $options none This is special-cased in MediaWikiSecurityCheckPlugin
* @param string|array $join_conds The query join conditions. {@see select} for details.
* @param-taint $join_conds none This is special-cased in MediaWikiSecurityCheckPlugin
*
* @return array The values from the field in the order they were returned from the DB
* @return-taint tainted
* @throws DBError If an error occurs, {@see query}
* @since 1.25
*/
@ -211,6 +225,7 @@ interface IReadableDatabase extends ISQLPlatform, DbQuoter, IDatabaseFlags {
* instead, which is more readable and less error-prone.
*
* @param string|array $table Table name(s)
* @param-taint $table exec_sql
*
* May be either an array of table names, or a single string holding a table
* name. If an array is given, table aliases can be specified, for example:
@ -246,6 +261,7 @@ interface IReadableDatabase extends ISQLPlatform, DbQuoter, IDatabaseFlags {
* not have characters outside of the Basic multilingual plane.
*
* @param string|array $vars Field name(s)
* @param-taint $vars exec_sql
*
* May be either a field name or an array of field names. The field names
* can be complete fragments of SQL, for direct inclusion into the SELECT
@ -261,6 +277,7 @@ interface IReadableDatabase extends ISQLPlatform, DbQuoter, IDatabaseFlags {
* Untrusted user input must not be passed to this parameter.
*
* @param string|array $conds
* @param-taint $conds exec_sql_numkey
*
* May be either a string containing a single condition, or an array of
* conditions. If an array is given, the conditions constructed from each
@ -304,8 +321,10 @@ interface IReadableDatabase extends ISQLPlatform, DbQuoter, IDatabaseFlags {
* 'actor' => [ 'JOIN', 'rev_actor = actor_id' ],
*
* @param string $fname Caller function name
* @param-taint $fname exec_sql
*
* @param string|array $options Query options
* @param-taint $options none This is special-cased in MediaWikiSecurityCheckPlugin
*
* Optional: Array of query options. Boolean options are specified by
* including them in the array as a string value with a numeric key, for
@ -371,6 +390,7 @@ interface IReadableDatabase extends ISQLPlatform, DbQuoter, IDatabaseFlags {
* - SQL_CALC_FOUND_ROWS
*
* @param string|array $join_conds Join conditions
* @param-taint $join_conds none This is special-cased in MediaWikiSecurityCheckPlugin
*
* Optional associative array of table-specific join conditions.
* Simple conditions can also be specified in the regular $conds,
@ -386,6 +406,7 @@ interface IReadableDatabase extends ISQLPlatform, DbQuoter, IDatabaseFlags {
*
* @internal
* @return IResultWrapper Resulting rows
* @return-taint tainted
* @throws DBError If an error occurs, {@see query}
*/
public function select(
@ -409,12 +430,19 @@ interface IReadableDatabase extends ISQLPlatform, DbQuoter, IDatabaseFlags {
*
* @internal
* @param string|array $table Table name
* @param-taint $table exec_sql
* @param string|array $vars Field names
* @param-taint $vars exec_sql
* @param string|array $conds Conditions
* @param-taint $conds exec_sql_numkey
* @param string $fname Caller function name
* @param-taint $fname exec_sql
* @param string|array $options Query options
* @param-taint $options none This is special-cased in MediaWikiSecurityCheckPlugin
* @param array|string $join_conds Join conditions
* @param-taint $join_conds none This is special-cased in MediaWikiSecurityCheckPlugin
* @return stdClass|false
* @return-taint tainted
* @throws DBError If an error occurs, {@see query}
*/
public function selectRow(
@ -470,12 +498,19 @@ interface IReadableDatabase extends ISQLPlatform, DbQuoter, IDatabaseFlags {
*
* @internal
* @param string|string[] $tables Table name(s)
* @param-taint $tables exec_sql
* @param string $var Column for which NULL values are not counted [default "*"]
* @param-taint $var exec_sql
* @param array|string $conds Filters on the table
* @param-taint $conds exec_sql_numkey
* @param string $fname Function name for profiling
* @param-taint $fname exec_sql
* @param array $options Options for select
* @param-taint $options none This is special-cased in MediaWikiSecurityCheckPlugin
* @param array $join_conds Join conditions (since 1.27)
* @param-taint $join_conds none This is special-cased in MediaWikiSecurityCheckPlugin
* @return int Row count
* @return-taint none
* @throws DBError If an error occurs, {@see query}
*/
public function selectRowCount(

15
includes/libs/rdbms/platform/ISQLPlatform.php
View file

@ -95,7 +95,9 @@ interface ISQLPlatform {
* Depending on the database this will either be `backticks` or "double quotes"
*
* @param string $s
* @param-taint $s escapes_sql NOTE: this is subpar, as addIdentifierQuotes isn't always the right type of escaping.
* @return string
* @return-taint none
* @since 1.33
*/
public function addIdentifierQuotes( $s );
@ -176,14 +178,17 @@ interface ISQLPlatform {
* This would set $sql to "rev_page = '$id' AND (rev_minor = 1 OR rev_len < 500)"
*
* @param array $a Containing the data
* @param-taint $a escapes_sql - Note, this is also special-cased in MediaWikiSecurityCheckPlugin
* @param int $mode IDatabase class constant:
* - IDatabase::LIST_COMMA: Comma separated, no field names
* - IDatabase::LIST_AND: ANDed WHERE clause (without the WHERE).
* - IDatabase::LIST_OR: ORed WHERE clause (without the WHERE)
* - IDatabase::LIST_SET: Comma separated with field names, like a SET clause
* - IDatabase::LIST_NAMES: Comma separated field names
* @param-taint $mode none
* @throws DBError If an error occurs, {@see IDatabase::query}
* @return string
* @return-taint none
*/
public function makeList( array $a, $mode = self::LIST_COMMA );
@ -292,8 +297,11 @@ interface ISQLPlatform {
*
* @since 1.16 in IDatabase, moved to ISQLPlatform in 1.39
* @param array[]|string|LikeMatch $param
* @param-taint $param escapes_sql
* @param string|LikeMatch ...$params
* @param-taint ...$params escapes_sql
* @return string Fully built LIKE statement
* @return-taint none
*/
public function buildLike( $param, ...$params );
@ -493,12 +501,19 @@ interface ISQLPlatform {
* @see IDatabase::select()
*
* @param string|array $table Table name(s)
* @param-taint $table exec_sql
* @param string|array $vars Field names
* @param-taint $vars exec_sql
* @param string|array $conds Conditions
* @param-taint $conds exec_sql_numkey
* @param string $fname Caller function name
* @param-taint $fname exec_sql
* @param string|array $options Query options
* @param-taint $options none This is special-cased in MediaWikiSecurityCheckPlugin
* @param string|array $join_conds Join conditions
* @param-taint $join_conds none This is special-cased in MediaWikiSecurityCheckPlugin
* @return string SQL query string
* @return-taint onlysafefor_sql
*/
public function selectSQLText(
$table,

1
tests/parser/DbTestPreviewer.php
View file

@ -134,6 +134,7 @@ class DbTestPreviewer extends TestRecorder {
printf( "\n%4d %s\n", $count, $label );
foreach ( $breakdown[$code] as $differing_test_name => $statusInfo ) {
// @phan-suppress-next-line SecurityCheck-XSS CLI-only script
print " * $differing_test_name [$statusInfo]\n";
}
}

4
tests/phan/TaintCheckAnnotationsTest.php
View file

@ -130,7 +130,7 @@ class TaintCheckAnnotationsTest {
$db->selectRowCount( $_GET['a'], '' ); // @phan-suppress-current-line SecurityCheck-SQLInjection
$db->selectRowCount( '', $_GET['a'] ); // @phan-suppress-current-line SecurityCheck-SQLInjection
$db->selectRowCount( '', '', [ $_GET['a'] ] ); // @phan-suppress-current-line SecurityCheck-SQLInjection
echo $db->selectRowCount( 'safe', 'safe' ); // @phan-suppress-current-line SecurityCheck-XSS
echo $db->selectRowCount( 'safe', 'safe' ); // Safe
$db->selectRow( $_GET['a'], '', [] ); // @phan-suppress-current-line SecurityCheck-SQLInjection
$db->selectRow( '', $_GET['a'], [] ); // @phan-suppress-current-line SecurityCheck-SQLInjection
@ -196,7 +196,7 @@ class TaintCheckAnnotationsTest {
$db->selectRowCount( $_GET['a'], '' ); // @phan-suppress-current-line SecurityCheck-SQLInjection
$db->selectRowCount( '', $_GET['a'] ); // @phan-suppress-current-line SecurityCheck-SQLInjection
$db->selectRowCount( '', '', [ $_GET['a'] ] ); // @phan-suppress-current-line SecurityCheck-SQLInjection
echo $db->selectRowCount( 'safe', 'safe' ); // @phan-suppress-current-line SecurityCheck-XSS
echo $db->selectRowCount( 'safe', 'safe' ); // Safe
$db->selectRow( $_GET['a'], '', [] ); // @phan-suppress-current-line SecurityCheck-SQLInjection
$db->selectRow( '', $_GET['a'], [] ); // @phan-suppress-current-line SecurityCheck-SQLInjection