Page MenuHomePhabricator
Files F14769116

0003-SECURITY-Cleanup-of-database-queries.patch
Ejegg (Elliott Eggleston)
Actions

Authored By
Ejegg
Mar 8 2018, 10:50 PM
Size
10 KB
Referenced Files
None
Subscribers
None

0003-SECURITY-Cleanup-of-database-queries.patch
View Options

From edadd69e8a0d3181c6ab5945e5d38e212a42d134 Mon Sep 17 00:00:00 2001
From: Brian Wolff <bawolff+wn@gmail.com>
Date: Sun, 4 Mar 2018 19:11:17 -0600
Subject: [PATCH 3/4] SECURITY: Cleanup of database queries
None of these issues are exploitable, but will
hopefully make the extension more robust against
future changes.
Adds missing quoting and validation of strings in
queries, removes some incorrect escaping.
Change-Id: Ib93555f8920913e915e6f16939ca78a42b8a14f7
Co-authored-by: Andrew Green <andrew.green.df@gmail.com>
Bug: T171987
---
CentralNoticeBannerLogPager.php | 3 +-
CentralNoticeCampaignLogPager.php | 2 +-
CentralNoticePager.php | 6 ++--
includes/Banner.php | 5 +--
includes/Campaign.php | 73 +++++++++++++++++++++++++--------------
5 files changed, 57 insertions(+), 32 deletions(-)
diff --git a/CentralNoticeBannerLogPager.php b/CentralNoticeBannerLogPager.php
index 914c9c7..c1d75ac 100644
--- a/CentralNoticeBannerLogPager.php
+++ b/CentralNoticeBannerLogPager.php
@@ -205,10 +205,11 @@ class CentralNoticeBannerLogPager extends CentralNoticeCampaignLogPager {
$oldrow = false;
if ( $newrow->tmplog_action === 'modified' ) {
$db = CNDatabase::getDb();
+ $tmplogId = (int)$newrow->tmplog_id;
$oldrow = $db->selectRow(
array( 'cn_template_log' => 'cn_template_log' ),
'*',
- array( 'tmplog_template_id' => $newrow->tmplog_template_id, "tmplog_id < {$newrow->tmplog_id}" ),
+ array( 'tmplog_template_id' => $newrow->tmplog_template_id, "tmplog_id < {$tmplogId}" ),
__METHOD__,
array( 'ORDER BY' => 'tmplog_id DESC', 'LIMIT' => 1 )
);
diff --git a/CentralNoticeCampaignLogPager.php b/CentralNoticeCampaignLogPager.php
index d8f8804..38c7de7 100644
--- a/CentralNoticeCampaignLogPager.php
+++ b/CentralNoticeCampaignLogPager.php
@@ -64,7 +64,7 @@ class CentralNoticeCampaignLogPager extends ReverseChronologicalPager {
if ( $filterUser ) {
$user = User::newFromName( $filterUser );
$userId = $user->getId();
- $info['conds'][] = "notlog_user_id = $userId";
+ $info['conds']["notlog_user_id"] = $userId;
}
}
diff --git a/CentralNoticePager.php b/CentralNoticePager.php
index a05ef7e..24f7cd7 100644
--- a/CentralNoticePager.php
+++ b/CentralNoticePager.php
@@ -57,8 +57,10 @@ class CentralNoticePager extends TemplatePager {
'join_conds' => array(
'assignments' => array(
'LEFT JOIN',
- "assignments.tmp_id = templates.tmp_id " .
- "AND assignments.not_id = $noticeId"
+ array(
+ "assignments.tmp_id = templates.tmp_id ",
+ "assignments.not_id" => $noticeId,
+ )
)
)
);
diff --git a/includes/Banner.php b/includes/Banner.php
index 757f1cd..c3f2092 100644
--- a/includes/Banner.php
+++ b/includes/Banner.php
@@ -1267,6 +1267,7 @@ class Banner {
$id = Banner::fromName( $name )->getId();
$dbr = CNDatabase::getDb();
+ $tsEnc = $dbr->addQuotes( $ts );
$newestLog = $dbr->selectRow(
"cn_template_log",
@@ -1274,7 +1275,7 @@ class Banner {
"log_id" => "MAX(tmplog_id)",
),
array(
- "tmplog_timestamp <= $ts",
+ "tmplog_timestamp <= $tsEnc",
"tmplog_template_id = $id",
),
__METHOD__
@@ -1292,7 +1293,7 @@ class Banner {
"fundraising" => "tmplog_end_fundraising",
),
array(
- "tmplog_id = {$newestLog->log_id}",
+ "tmplog_id" => $newestLog->log_id,
),
__METHOD__
);
diff --git a/includes/Campaign.php b/includes/Campaign.php
index 4cd7c3d..0f15b1a 100644
--- a/includes/Campaign.php
+++ b/includes/Campaign.php
@@ -252,8 +252,7 @@ class Campaign {
static function campaignExists( $campaignName ) {
$dbr = CNDatabase::getDb();
- $eCampaignName = htmlspecialchars( $campaignName );
- return (bool)$dbr->selectRow( 'cn_notices', 'not_name', array( 'not_name' => $eCampaignName ) );
+ return (bool)$dbr->selectRow( 'cn_notices', 'not_name', array( 'not_name' => $campaignName ) );
}
/**
@@ -452,13 +451,15 @@ class Campaign {
*/
static function getHistoricalCampaigns( $ts ) {
$dbr = CNDatabase::getDb();
+ $tsEnc = $dbr->addQuotes( $dbr->timestamp( $ts ) );
+
$res = $dbr->select(
"cn_notice_log",
array(
"log_id" => "MAX(notlog_id)",
),
array(
- "notlog_timestamp <= $ts",
+ "notlog_timestamp <= $tsEnc",
),
__METHOD__,
array(
@@ -468,7 +469,7 @@ class Campaign {
$campaigns = array();
foreach ( $res as $row ) {
- $singleRes = $dbr->select(
+ $campaignRow = $dbr->selectRow(
"cn_notice_log",
array(
"id" => "notlog_not_id",
@@ -484,18 +485,18 @@ class Campaign {
"throttle" => "notlog_end_throttle",
),
array(
- "notlog_id = {$row->log_id}",
- "notlog_end_start <= $ts",
- "notlog_end_end >= $ts",
+ "notlog_id" => $row->log_id,
+ "notlog_end_start <= $tsEnc",
+ "notlog_end_end >= $tsEnc",
"notlog_end_enabled = 1",
),
__METHOD__
);
- $campaign = $singleRes->fetchRow();
- if ( !$campaign ) {
+ if ( !$campaignRow ) {
continue;
}
+ $campaign = (array)$campaignRow;
$campaign['projects'] = explode( ", ", $campaign['projects'] );
$campaign['languages'] = explode( ", ", $campaign['languages'] );
$campaign['countries'] = explode( ", ", $campaign['countries'] );
@@ -967,8 +968,7 @@ class Campaign {
static function addTemplateTo( $noticeName, $templateName, $weight, $bucket = 0 ) {
$dbw = CNDatabase::getDb( DB_MASTER );
- $eNoticeName = htmlspecialchars( $noticeName );
- $noticeId = Campaign::getNoticeId( $eNoticeName );
+ $noticeId = Campaign::getNoticeId( $noticeName );
$templateId = Banner::fromName( $templateName )->getId();
$res = $dbw->select( 'cn_assignments', 'asn_id',
array(
@@ -981,7 +981,6 @@ class Campaign {
return 'centralnotice-template-already-exists';
}
- $noticeId = Campaign::getNoticeId( $eNoticeName );
$dbw->insert( 'cn_assignments',
array(
'tmp_id' => $templateId,
@@ -1011,8 +1010,7 @@ class Campaign {
*/
static function getNoticeId( $noticeName ) {
$dbr = CNDatabase::getDb();
- $eNoticeName = htmlspecialchars( $noticeName );
- $row = $dbr->selectRow( 'cn_notices', 'not_id', array( 'not_name' => $eNoticeName ) );
+ $row = $dbr->selectRow( 'cn_notices', 'not_id', array( 'not_name' => $noticeName ) );
if ( $row ) {
return $row->not_id;
} else {
@@ -1036,8 +1034,7 @@ class Campaign {
static function getNoticeProjects( $noticeName ) {
$dbr = CNDatabase::getDb();
- $eNoticeName = htmlspecialchars( $noticeName );
- $row = $dbr->selectRow( 'cn_notices', 'not_id', array( 'not_name' => $eNoticeName ) );
+ $row = $dbr->selectRow( 'cn_notices', 'not_id', array( 'not_name' => $noticeName ) );
$projects = array();
if ( $row ) {
$res = $dbr->select( 'cn_notice_projects', 'np_project',
@@ -1052,8 +1049,7 @@ class Campaign {
static function getNoticeLanguages( $noticeName ) {
$dbr = CNDatabase::getDb();
- $eNoticeName = htmlspecialchars( $noticeName );
- $row = $dbr->selectRow( 'cn_notices', 'not_id', array( 'not_name' => $eNoticeName ) );
+ $row = $dbr->selectRow( 'cn_notices', 'not_id', array( 'not_name' => $noticeName ) );
$languages = array();
if ( $row ) {
$res = $dbr->select( 'cn_notice_languages', 'nl_language',
@@ -1068,8 +1064,7 @@ class Campaign {
static function getNoticeCountries( $noticeName ) {
$dbr = CNDatabase::getDb();
- $eNoticeName = htmlspecialchars( $noticeName );
- $row = $dbr->selectRow( 'cn_notices', 'not_id', array( 'not_name' => $eNoticeName ) );
+ $row = $dbr->selectRow( 'cn_notices', 'not_id', array( 'not_name' => $noticeName ) );
$countries = array();
if ( $row ) {
$res = $dbr->select( 'cn_notice_countries', 'nc_country',
@@ -1129,6 +1124,9 @@ class Campaign {
return;
} else {
$settingName = strtolower( $settingName );
+ if ( !self::settingNameIsValid( $settingName ) ) {
+ throw new InvalidArgumentException( "Invalid setting name" );
+ }
$dbw = CNDatabase::getDb( DB_MASTER );
$dbw->update( 'cn_notices',
array( 'not_' . $settingName => (int)$settingValue ),
@@ -1169,6 +1167,9 @@ class Campaign {
return;
} else {
$settingName = strtolower( $settingName );
+ if ( !self::settingNameIsValid( $settingName ) ) {
+ throw new InvalidArgumentException( "Invalid setting name" );
+ }
$dbw = CNDatabase::getDb( DB_MASTER );
$dbw->update( 'cn_notices',
array( 'not_'.$settingName => $settingValue ),
@@ -1351,9 +1352,15 @@ class Campaign {
);
foreach ( $beginSettings as $key => $value ) {
+ if ( !self::settingNameIsValid( $key ) ) {
+ throw new InvalidArgumentException( "Invalid setting name" );
+ }
$log[ 'notlog_begin_' . $key ] = $value;
}
foreach ( $endSettings as $key => $value ) {
+ if ( !self::settingNameIsValid( $key ) ) {
+ throw new InvalidArgumentException( "Invalid setting name" );
+ }
$log[ 'notlog_end_' . $key ] = $value;
}
@@ -1365,27 +1372,41 @@ class Campaign {
}
}
+ /**
+ * Check that a string is a valid setting name.
+ * @param string $settingName
+ * @return boolean
+ */
+ protected static function settingNameIsValid( $settingName ) {
+ return ( preg_match( '/^[a-z_]*$/', $settingName ) === 1 );
+ }
+
static function campaignLogs( $campaign=false, $username=false, $start=false, $end=false, $limit=50, $offset=0 ) {
+ // Read from the master database to avoid concurrency problems
+ $dbr = CNDatabase::getDb();
$conds = array();
if ( $start ) {
- $conds[] = "notlog_timestamp >= $start";
+ $conds[] = "notlog_timestamp >= " . $dbr->addQuotes( $start );
}
if ( $end ) {
- $conds[] = "notlog_timestamp < $end";
+ $conds[] = "notlog_timestamp < " . $dbr->addQuotes( $end );
}
if ( $campaign ) {
- $conds[] = "notlog_not_name LIKE '$campaign'";
+ // This used to be a LIKE, but that was undocumented,
+ // and filters prevented the % and \ character from being
+ // used. The one character _ wildcard could have been used
+ // from the api, but that was completely undocumented.
+ // This was sketchy security wise, so the LIKE was removed.
+ $conds["notlog_not_name"] = $campaign;
}
if ( $username ) {
$user = User::newFromName( $username );
if ( $user ) {
- $conds[] = "notlog_user_id = {$user->getId()}";
+ $conds["notlog_user_id"] = $user->getId();
}
}
- // Read from the master database to avoid concurrency problems
- $dbr = CNDatabase::getDb();
$res = $dbr->select( 'cn_notice_log', '*', $conds,
__METHOD__,
array(
--
2.16.1

File Metadata

Mime Type
text/x-diff
Storage Engine
blob
Storage Format
Raw Data
Storage Handle
5608328
Default Alt Text
0003-SECURITY-Cleanup-of-database-queries.patch (10 KB)

Event Timeline

Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL