Page Menu
Home
Phabricator
Search
Configure Global Search
Log In
Files
F4414552
T125177.patch
dpatrick (Darian Anthony Patrick)
Actions
View File
Edit File
Delete File
View Transforms
Subscribe
Mute Notifications
Award Token
Flag For Later
Authored By
•
dpatrick
Aug 29 2016, 10:37 PM
2016-08-29 22:37:35 (UTC+0)
Size
6 KB
Referenced Files
None
Subscribers
None
T125177.patch
View Options
From 5970841646d67894b5ab45c83471ea191e51ecf2 Mon Sep 17 00:00:00 2001
From: Brad Jorsch <bjorsch@wikimedia.org>
Date: Thu, 18 Aug 2016 13:37:05 -0400
Subject: [PATCH] SECURITY: API: Don't log "sensitive" parameters
Stuff like passwords and CSRF tokens shouldn't be in the logs.
The fact of being sensitive is intentionally separated from the need to
be in the POST body because, for example, the wltoken parameter to
ApiQueryWatchlist needs to be in the query string to serve its purpose
but still shouldn't be logged.
Bug: T125177
Change-Id: I1d61f4dcf792d77401ee2e2988b1afcb2a2ad58f
---
RELEASE-NOTES-1.28 | 2 ++
includes/api/ApiAuthManagerHelper.php | 1 +
includes/api/ApiBase.php | 12 ++++++++++++
includes/api/ApiCheckToken.php | 1 +
includes/api/ApiLogin.php | 1 +
includes/api/ApiMain.php | 25 ++++++++++++++++++++++++-
includes/api/ApiQueryWatchlist.php | 3 ++-
includes/api/ApiQueryWatchlistRaw.php | 3 ++-
8 files changed, 45 insertions(+), 3 deletions(-)
diff --git a/RELEASE-NOTES-1.28 b/RELEASE-NOTES-1.28
index 0b7e68b..0d09514 100644
--- a/RELEASE-NOTES-1.28
+++ b/RELEASE-NOTES-1.28
@@ -90,6 +90,8 @@ production.
interact with ApiParse and ApiExpandTemplates.
* (T139565) SECURITY: API: Generate head items in the context of the given title
* (T115333) SECURITY: Check read permission when loading page content in ApiParse
+* (T125177) SECURITY: API parameters may now be marked as "sensitive" to keep
+ their values out of the logs.
=== Languages updated in 1.28 ===
diff --git a/includes/api/ApiAuthManagerHelper.php b/includes/api/ApiAuthManagerHelper.php
index 8e57f93..2d0f265 100644
--- a/includes/api/ApiAuthManagerHelper.php
+++ b/includes/api/ApiAuthManagerHelper.php
@@ -173,6 +173,7 @@ class ApiAuthManagerHelper {
$this->module->getMain()->markParamsUsed( array_keys( $data ) );
if ( $sensitive ) {
+ $this->module->getMain()->markParamsSensitive( array_keys( $sensitive ) );
try {
$this->module->requirePostedParameters( array_keys( $sensitive ), 'noprefix' );
} catch ( UsageException $ex ) {
diff --git a/includes/api/ApiBase.php b/includes/api/ApiBase.php
index 66c1b53..eeeab47 100644
--- a/includes/api/ApiBase.php
+++ b/includes/api/ApiBase.php
@@ -171,6 +171,13 @@ abstract class ApiBase extends ContextSource {
*/
const PARAM_SUBMODULE_PARAM_PREFIX = 16;
+ /**
+ * (boolean) Is the parameter sensitive? Note 'password'-type fields are
+ * always sensitive regardless of the value of this field.
+ * @since 1.28
+ */
+ const PARAM_SENSITIVE = 17;
+
/**@}*/
/** Fast query, standard limit. */
@@ -948,6 +955,10 @@ abstract class ApiBase extends ContextSource {
$type = 'NULL'; // allow everything
}
}
+
+ if ( $type == 'password' || !empty( $paramSettings[self::PARAM_SENSITIVE] ) ) {
+ $this->getMain()->markParamsSensitive( $encParamName );
+ }
}
if ( $type == 'boolean' ) {
@@ -2307,6 +2318,7 @@ abstract class ApiBase extends ContextSource {
$params['token'] = [
ApiBase::PARAM_TYPE => 'string',
ApiBase::PARAM_REQUIRED => true,
+ ApiBase::PARAM_SENSITIVE => true,
ApiBase::PARAM_HELP_MSG => [
'api-help-param-token',
$this->needsToken(),
diff --git a/includes/api/ApiCheckToken.php b/includes/api/ApiCheckToken.php
index 3d2159c..2736ff4 100644
--- a/includes/api/ApiCheckToken.php
+++ b/includes/api/ApiCheckToken.php
@@ -66,6 +66,7 @@ class ApiCheckToken extends ApiBase {
'token' => [
ApiBase::PARAM_TYPE => 'string',
ApiBase::PARAM_REQUIRED => true,
+ ApiBase::PARAM_SENSITIVE => true,
],
'maxtokenage' => [
ApiBase::PARAM_TYPE => 'integer',
diff --git a/includes/api/ApiLogin.php b/includes/api/ApiLogin.php
index 9bc0b3a..145d88b 100644
--- a/includes/api/ApiLogin.php
+++ b/includes/api/ApiLogin.php
@@ -268,6 +268,7 @@ class ApiLogin extends ApiBase {
'token' => [
ApiBase::PARAM_TYPE => 'string',
ApiBase::PARAM_REQUIRED => false, // for BC
+ ApiBase::PARAM_SENSITIVE => true,
ApiBase::PARAM_HELP_MSG => [ 'api-help-param-token', 'login' ],
],
];
diff --git a/includes/api/ApiMain.php b/includes/api/ApiMain.php
index 22b079d..c138576 100644
--- a/includes/api/ApiMain.php
+++ b/includes/api/ApiMain.php
@@ -152,6 +152,7 @@ class ApiMain extends ApiBase {
private $mCacheMode = 'private';
private $mCacheControl = [];
private $mParamsUsed = [];
+ private $mParamsSensitive = [];
/** @var bool|null Cached return value from self::lacksSameOriginSecurity() */
private $lacksSameOriginSecurity = null;
@@ -1483,13 +1484,17 @@ class ApiMain extends ApiBase {
" {$logCtx['ip']} " .
"T={$logCtx['timeSpentBackend']}ms";
+ $sensitive = array_flip( $this->getSensitiveParams() );
foreach ( $this->getParamsUsed() as $name ) {
$value = $request->getVal( $name );
if ( $value === null ) {
continue;
}
- if ( strlen( $value ) > 256 ) {
+ if ( isset( $sensitive[$name] ) ) {
+ $value = '[redacted]';
+ $encValue = '[redacted]';
+ } elseif ( strlen( $value ) > 256 ) {
$value = substr( $value, 0, 256 );
$encValue = $this->encodeRequestLogValue( $value ) . '[...]';
} else {
@@ -1540,6 +1545,24 @@ class ApiMain extends ApiBase {
}
/**
+ * Get the request parameters that should be considered sensitive
+ * @since 1.28
+ * @return array
+ */
+ protected function getSensitiveParams() {
+ return array_keys( $this->mParamsSensitive );
+ }
+
+ /**
+ * Mark parameters as sensitive
+ * @since 1.28
+ * @param string|string[] $params
+ */
+ public function markParamsSensitive( $params ) {
+ $this->mParamsSensitive += array_fill_keys( (array)$params, true );
+ }
+
+ /**
* Get a request value, and register the fact that it was used, for logging.
* @param string $name
* @param mixed $default
diff --git a/includes/api/ApiQueryWatchlist.php b/includes/api/ApiQueryWatchlist.php
index e2599d1..08054a8 100644
--- a/includes/api/ApiQueryWatchlist.php
+++ b/includes/api/ApiQueryWatchlist.php
@@ -477,7 +477,8 @@ class ApiQueryWatchlist extends ApiQueryGeneratorBase {
ApiBase::PARAM_TYPE => 'user'
],
'token' => [
- ApiBase::PARAM_TYPE => 'string'
+ ApiBase::PARAM_TYPE => 'string',
+ ApiBase::PARAM_SENSITIVE => true,
],
'continue' => [
ApiBase::PARAM_HELP_MSG => 'api-help-param-continue',
diff --git a/includes/api/ApiQueryWatchlistRaw.php b/includes/api/ApiQueryWatchlistRaw.php
index 806861e..50417ea 100644
--- a/includes/api/ApiQueryWatchlistRaw.php
+++ b/includes/api/ApiQueryWatchlistRaw.php
@@ -170,7 +170,8 @@ class ApiQueryWatchlistRaw extends ApiQueryGeneratorBase {
ApiBase::PARAM_TYPE => 'user'
],
'token' => [
- ApiBase::PARAM_TYPE => 'string'
+ ApiBase::PARAM_TYPE => 'string',
+ ApiBase::PARAM_SENSITIVE => true,
],
'dir' => [
ApiBase::PARAM_DFLT => 'ascending',
--
2.9.3
File Metadata
Details
Attached
Mime Type
text/x-diff
Storage Engine
blob
Storage Format
Raw Data
Storage Handle
3946461
Default Alt Text
T125177.patch (6 KB)
Attached To
Mode
T140591: MediaWiki 1.28.1/1.27.2/1.23.16 security release
Attached
Detach File
T125177: api.log contains passwords in plaintext
Attached
Detach File
Event Timeline
Log In to Comment