Page MenuHomePhabricator
Files F14769188

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

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

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

From 965369d79afd3ec3e2367379e56d6eabf10683f0 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 f9dd4a0..4b0d9cb 100644
--- a/CentralNoticeBannerLogPager.php
+++ b/CentralNoticeBannerLogPager.php
@@ -206,11 +206,12 @@ class CentralNoticeBannerLogPager extends CentralNoticeCampaignLogPager {
$oldrow = false;
if ( $newrow->tmplog_action === 'modified' ) {
$db = CNDatabase::getDb();
+ $tmplogId = (int)$newrow->tmplog_id;
$oldrow = $db->selectRow(
[ 'cn_template_log' => 'cn_template_log' ],
'*',
[ 'tmplog_template_id' => $newrow->tmplog_template_id,
- "tmplog_id < {$newrow->tmplog_id}" ],
+ "tmplog_id < {$tmplogId}" ],
__METHOD__,
[ 'ORDER BY' => 'tmplog_id DESC', 'LIMIT' => 1 ]
);
diff --git a/CentralNoticeCampaignLogPager.php b/CentralNoticeCampaignLogPager.php
index 08b4bad..65cbdad 100644
--- a/CentralNoticeCampaignLogPager.php
+++ b/CentralNoticeCampaignLogPager.php
@@ -65,7 +65,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 5d8e7b3..fb58f77 100644
--- a/CentralNoticePager.php
+++ b/CentralNoticePager.php
@@ -57,8 +57,10 @@ class CentralNoticePager extends TemplatePager {
'join_conds' => [
'assignments' => [
'LEFT JOIN',
- "assignments.tmp_id = templates.tmp_id " .
- "AND assignments.not_id = $noticeId"
+ [
+ "assignments.tmp_id = templates.tmp_id ",
+ "assignments.not_id" => $noticeId
+ ]
]
]
];
diff --git a/includes/Banner.php b/includes/Banner.php
index 75ca262..c333abb 100644
--- a/includes/Banner.php
+++ b/includes/Banner.php
@@ -1325,6 +1325,7 @@ class Banner {
$id = self::fromName( $name )->getId();
$dbr = CNDatabase::getDb();
+ $tsEnc = $dbr->addQuotes( $ts );
$newestLog = $dbr->selectRow(
"cn_template_log",
@@ -1332,7 +1333,7 @@ class Banner {
"log_id" => "MAX(tmplog_id)",
],
[
- "tmplog_timestamp <= $ts",
+ "tmplog_timestamp <= $tsEnc",
"tmplog_template_id = $id",
],
__METHOD__
@@ -1350,7 +1351,7 @@ class Banner {
"fundraising" => "tmplog_end_fundraising",
],
[
- "tmplog_id = {$newestLog->log_id}",
+ "tmplog_id" => $newestLog->log_id,
],
__METHOD__
);
diff --git a/includes/Campaign.php b/includes/Campaign.php
index 583ddbe..f171993 100644
--- a/includes/Campaign.php
+++ b/includes/Campaign.php
@@ -253,8 +253,7 @@ class Campaign {
static function campaignExists( $campaignName ) {
$dbr = CNDatabase::getDb();
- $eCampaignName = htmlspecialchars( $campaignName );
- return (bool)$dbr->selectRow( 'cn_notices', 'not_name', [ 'not_name' => $eCampaignName ] );
+ return (bool)$dbr->selectRow( 'cn_notices', 'not_name', [ 'not_name' => $campaignName ] );
}
/**
@@ -454,13 +453,15 @@ class Campaign {
*/
static function getHistoricalCampaigns( $ts ) {
$dbr = CNDatabase::getDb();
+ $tsEnc = $dbr->addQuotes( $dbr->timestamp( $ts ) );
+
$res = $dbr->select(
"cn_notice_log",
[
"log_id" => "MAX(notlog_id)",
],
[
- "notlog_timestamp <= $ts",
+ "notlog_timestamp <= $tsEnc",
],
__METHOD__,
[
@@ -470,7 +471,7 @@ class Campaign {
$campaigns = [];
foreach ( $res as $row ) {
- $singleRes = $dbr->select(
+ $campaignRow = $dbr->selectRow(
"cn_notice_log",
[
"id" => "notlog_not_id",
@@ -486,18 +487,18 @@ class Campaign {
"throttle" => "notlog_end_throttle",
],
[
- "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'] );
@@ -979,8 +980,7 @@ class Campaign {
static function addTemplateTo( $noticeName, $templateName, $weight, $bucket = 0 ) {
$dbw = CNDatabase::getDb( DB_MASTER );
- $eNoticeName = htmlspecialchars( $noticeName );
- $noticeId = self::getNoticeId( $eNoticeName );
+ $noticeId = self::getNoticeId( $noticeName );
$templateId = Banner::fromName( $templateName )->getId();
$res = $dbw->select( 'cn_assignments', 'asn_id',
[
@@ -993,7 +993,6 @@ class Campaign {
return 'centralnotice-template-already-exists';
}
- $noticeId = self::getNoticeId( $eNoticeName );
$dbw->insert( 'cn_assignments',
[
'tmp_id' => $templateId,
@@ -1023,8 +1022,7 @@ class Campaign {
*/
static function getNoticeId( $noticeName ) {
$dbr = CNDatabase::getDb();
- $eNoticeName = htmlspecialchars( $noticeName );
- $row = $dbr->selectRow( 'cn_notices', 'not_id', [ 'not_name' => $eNoticeName ] );
+ $row = $dbr->selectRow( 'cn_notices', 'not_id', [ 'not_name' => $noticeName ] );
if ( $row ) {
return $row->not_id;
} else {
@@ -1048,8 +1046,7 @@ class Campaign {
static function getNoticeProjects( $noticeName ) {
$dbr = CNDatabase::getDb();
- $eNoticeName = htmlspecialchars( $noticeName );
- $row = $dbr->selectRow( 'cn_notices', 'not_id', [ 'not_name' => $eNoticeName ] );
+ $row = $dbr->selectRow( 'cn_notices', 'not_id', [ 'not_name' => $noticeName ] );
$projects = [];
if ( $row ) {
$res = $dbr->select( 'cn_notice_projects', 'np_project',
@@ -1064,8 +1061,7 @@ class Campaign {
static function getNoticeLanguages( $noticeName ) {
$dbr = CNDatabase::getDb();
- $eNoticeName = htmlspecialchars( $noticeName );
- $row = $dbr->selectRow( 'cn_notices', 'not_id', [ 'not_name' => $eNoticeName ] );
+ $row = $dbr->selectRow( 'cn_notices', 'not_id', [ 'not_name' => $noticeName ] );
$languages = [];
if ( $row ) {
$res = $dbr->select( 'cn_notice_languages', 'nl_language',
@@ -1080,8 +1076,7 @@ class Campaign {
static function getNoticeCountries( $noticeName ) {
$dbr = CNDatabase::getDb();
- $eNoticeName = htmlspecialchars( $noticeName );
- $row = $dbr->selectRow( 'cn_notices', 'not_id', [ 'not_name' => $eNoticeName ] );
+ $row = $dbr->selectRow( 'cn_notices', 'not_id', [ 'not_name' => $noticeName ] );
$countries = [];
if ( $row ) {
$res = $dbr->select( 'cn_notice_countries', 'nc_country',
@@ -1141,6 +1136,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',
[ 'not_' . $settingName => (int)$settingValue ],
@@ -1183,6 +1181,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',
[ 'not_'.$settingName => $settingValue ],
@@ -1365,9 +1366,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;
}
@@ -1379,28 +1386,42 @@ 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 = [];
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__,
[
--
2.16.1

File Metadata

Mime Type
text/x-diff
Storage Engine
blob
Storage Format
Raw Data
Storage Handle
5608338
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