Page MenuHomePhabricator
Files F4932228

0001-SECURITY-API-Don-t-log-sensitive-parameters.patch
Anomie
Actions

Authored By
Anomie
Dec 1 2016, 7:06 PM
Size
6 KB
Referenced Files
None
Subscribers
None

0001-SECURITY-API-Don-t-log-sensitive-parameters.patch
View Options

From c1f26dd444ae6c823adc7cf3b45aee3113c999af 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.29 | 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.29 b/RELEASE-NOTES-1.29
index 21a94c5..4462831 100644
--- a/RELEASE-NOTES-1.29
+++ b/RELEASE-NOTES-1.29
@@ -39,6 +39,8 @@ production.
* action=clearhasmsg now requires a POST.
=== Action API internal changes in 1.29 ===
+* API parameters may now be marked as "sensitive" to keep their values out of
+ the logs.
=== Languages updated in 1.29 ===
diff --git a/includes/api/ApiAuthManagerHelper.php b/includes/api/ApiAuthManagerHelper.php
index 6fafebf..5527437 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 ) );
$this->module->requirePostedParameters( array_keys( $sensitive ), 'noprefix' );
}
diff --git a/includes/api/ApiBase.php b/includes/api/ApiBase.php
index 0cd46e4..a0266f8 100644
--- a/includes/api/ApiBase.php
+++ b/includes/api/ApiBase.php
@@ -180,6 +180,13 @@ abstract class ApiBase extends ContextSource {
*/
const PARAM_ALL = 17;
+ /**
+ * (boolean) Is the parameter sensitive? Note 'password'-type fields are
+ * always sensitive regardless of the value of this field.
+ * @since 1.29
+ */
+ const PARAM_SENSITIVE = 18;
+
/**@}*/
const ALL_DEFAULT_STRING = '*';
@@ -963,6 +970,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' ) {
@@ -2401,6 +2412,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 dd88b5f..83c8aa1 100644
--- a/includes/api/ApiCheckToken.php
+++ b/includes/api/ApiCheckToken.php
@@ -75,6 +75,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 6ac261d..c4060c5 100644
--- a/includes/api/ApiLogin.php
+++ b/includes/api/ApiLogin.php
@@ -256,6 +256,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 38299b4..a1300df 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;
@@ -1490,13 +1491,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 {
@@ -1547,6 +1552,24 @@ class ApiMain extends ApiBase {
}
/**
+ * Get the request parameters that should be considered sensitive
+ * @since 1.29
+ * @return array
+ */
+ protected function getSensitiveParams() {
+ return array_keys( $this->mParamsSensitive );
+ }
+
+ /**
+ * Mark parameters as sensitive
+ * @since 1.29
+ * @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 42ea55d..7b64675 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.10.2

File Metadata

Mime Type
text/x-diff
Storage Engine
blob
Storage Format
Raw Data
Storage Handle
4175971
Default Alt Text
0001-SECURITY-API-Don-t-log-sensitive-parameters.patch (6 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