Werdna has uploaded a new change for review.
https://gerrit.wikimedia.org/r/100521
Change subject: Add function for checking options arrays for SQL injection
......................................................................
Add function for checking options arrays for SQL injection
Now used anywhere where external options are accepted
Change-Id: I8749aab2242fa29e4f3f47af07a7427cd3eefaa4
---
M includes/Block/TopicList.php
M includes/Data/ObjectManager.php
M includes/Data/RevisionStorage.php
3 files changed, 87 insertions(+), 1 deletion(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/Flow
refs/changes/21/100521/1
diff --git a/includes/Block/TopicList.php b/includes/Block/TopicList.php
index 505b6ee..974a3be 100644
--- a/includes/Block/TopicList.php
+++ b/includes/Block/TopicList.php
@@ -62,6 +62,11 @@
$storage = $this->storage;
$defStorage = $this->storage->getStorage( 'Definition' );
$sourceDef = $defStorage->get(
$this->workflow->getDefinitionId() );
+
+ if ( ! $sourceDef ) {
+ throw new \MWException( "Unable to retrieve definition
for this workflow" );
+ }
+
$topicDef = $defStorage->get( $sourceDef->getOption(
'topic_definition_id' ) );
if ( !$topicDef ) {
throw new \MWException( 'Invalid definition owns this
TopicList, needs a valid topic_definition_id option assigned' );
diff --git a/includes/Data/ObjectManager.php b/includes/Data/ObjectManager.php
index 4e01c48..61a10f1 100644
--- a/includes/Data/ObjectManager.php
+++ b/includes/Data/ObjectManager.php
@@ -663,12 +663,83 @@
return true;
}
- if ( ! preg_match( '/^[A-Za-z0-9\._]+$/', $key ) ) {
+ if ( ! preg_match( '/^' . $this->getFieldRegex() .
'$/', $key ) ) {
return true;
}
}
return false;
+ }
+
+ /**
+ * Returns a regular expression fragment suitable for matching a valid
+ * SQL field name, and hopefully no injection attacks
+ * @return string Regular expression fragment
+ */
+ protected function getFieldRegexFragment() {
+ return '\s*[A-Za-z0-9\._]+\s*';
+ }
+
+ /**
+ * Internal security function to check an options array for
+ * SQL injection and other funkiness
+ * @todo Currently only supports LIMIT, OFFSET and ORDER BY
+ * @param array $options An options array passed to a query.
+ * @return boolean
+ */
+ protected function validateOptions( $options ) {
+ static $validUnaryOptions = array(
+ 'UNIQUE',
+ 'EXPLAIN',
+ );
+
+ $fieldRegex = $this->getFieldRegexFragment();
+
+ foreach( $options as $key => $value ) {
+ if ( is_numeric( $key ) && in_array( strtoupper( $value
), $validUnaryOptions ) ) {
+ continue;
+ } elseif ( is_numeric( $key ) ) {
+ wfDebug( __METHOD__.": Unrecognised unary
operator $value\n" );
+ return false;
+ }
+
+ if ( $key === 'LIMIT' ) {
+ // LIMIT is one or two integers, separated by a
comma.
+ if ( ! preg_match ( '/^\d+(,\d+)?$/', $value )
) {
+ wfDebug( __METHOD__.": Invalid LIMIT
$value\n" );
+ return false;
+ }
+ } elseif ( $key === 'ORDER BY' ) {
+ // ORDER BY is a list of field names with ASC /
DESC afterwards
+ if ( is_string( $value ) ) {
+ $value = explode( ',', $value );
+ }
+ $orderByRegex =
"/^\s*$fieldRegex\s*(ASC|DESC)?\s*$/i";
+
+ foreach( $value as $orderByField ) {
+ if ( ! preg_match( $orderByRegex,
$orderByField ) ) {
+ wfDebug( __METHOD__.": invalid
ORDER BY field $orderByField\n" );
+ return false;
+ }
+ }
+ } elseif ( $key === 'OFFSET' ) {
+ // OFFSET is just an integer
+ if ( ! is_numeric( $value ) ) {
+ wfDebug( __METHOD__.": non-numeric
offset $offset\n" );
+ return false;
+ }
+ } elseif ( $key === 'GROUP BY' ) {
+ if ( ! preg_match(
"/^{$fieldRegex}(,{$fieldRegex})+$/", $value ) ) {
+ wfDebug( __METHOD__.": invalid GROUP BY
field\n" );
+ }
+ } else {
+ wfDebug( __METHOD__.": Unknown option $key\n" );
+ return false;
+ }
+ }
+
+ // Everything passes
+ return true;
}
}
@@ -765,6 +836,10 @@
$attributes = $this->preprocessSqlArray( $attributes );
+ if ( ! $this->validateOptions( $options ) ) {
+ throw new MWException( "Validation error in database
options" );
+ }
+
$res = $this->dbFactory->getDB( DB_MASTER )->select(
$this->table,
'*',
diff --git a/includes/Data/RevisionStorage.php
b/includes/Data/RevisionStorage.php
index 4843c45..4349bd4 100644
--- a/includes/Data/RevisionStorage.php
+++ b/includes/Data/RevisionStorage.php
@@ -7,6 +7,7 @@
use Flow\Repository\TreeRepository;
use DatabaseBase;
use ExternalStore;
+use MWException;
use User;
abstract class RevisionStorage extends DbStorage {
@@ -44,6 +45,11 @@
protected function findInternal( array $attributes, array $options =
array() ) {
$dbr = $this->dbFactory->getDB( DB_MASTER );
+
+ if ( ! $this->validateOptions( $options ) ) {
+ throw new MWException( "Validation error in database
options" );
+ }
+
$res = $dbr->select(
array( $this->joinTable(), 'rev' => 'flow_revision' ),
'*',
--
To view, visit https://gerrit.wikimedia.org/r/100521
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I8749aab2242fa29e4f3f47af07a7427cd3eefaa4
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/Flow
Gerrit-Branch: master
Gerrit-Owner: Werdna <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
