jenkins-bot has submitted this change and it was merged. (
https://gerrit.wikimedia.org/r/346841 )
Change subject: SECURITY: API: Don't log "sensitive" parameters
......................................................................
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
---
M RELEASE-NOTES-1.29
M includes/api/ApiAuthManagerHelper.php
M includes/api/ApiBase.php
M includes/api/ApiCheckToken.php
M includes/api/ApiLogin.php
M includes/api/ApiMain.php
M includes/api/ApiQueryWatchlist.php
M includes/api/ApiQueryWatchlistRaw.php
8 files changed, 47 insertions(+), 3 deletions(-)
Approvals:
Chad: Looks good to me, approved
jenkins-bot: Verified
diff --git a/RELEASE-NOTES-1.29 b/RELEASE-NOTES-1.29
index a2dbcd5..94bdcf7 100644
--- a/RELEASE-NOTES-1.29
+++ b/RELEASE-NOTES-1.29
@@ -90,6 +90,8 @@
to interwiki links.
* (T144845) SECURITY: XSS in SearchHighlighter::highlightText() when
$wgAdvancedSearchHighlighting is true.
+* (T125177) SECURITY: API parameters may now be marked as "sensitive" to keep
+ their values out of the logs.
=== Action API changes in 1.29 ===
* Submitting sensitive authentication request parameters to action=login,
@@ -150,6 +152,8 @@
various methods now take a module path rather than a module name.
* ApiMessageTrait::getApiCode() now strips 'apierror-' and 'apiwarn-' prefixes
from the message key, and maps some message keys for backwards compatibility.
+* 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 d037c36..8862cc7 100644
--- a/includes/api/ApiAuthManagerHelper.php
+++ b/includes/api/ApiAuthManagerHelper.php
@@ -169,6 +169,7 @@
$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 fec4234..b698cef 100644
--- a/includes/api/ApiBase.php
+++ b/includes/api/ApiBase.php
@@ -188,6 +188,13 @@
*/
const PARAM_EXTRA_NAMESPACES = 18;
+ /*
+ * (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 = 19;
+
/**@}*/
const ALL_DEFAULT_STRING = '*';
@@ -1024,6 +1031,10 @@
$type = gettype( $default );
} else {
$type = 'NULL'; // allow everything
+ }
+
+ if ( $type == 'password' || !empty(
$paramSettings[self::PARAM_SENSITIVE] ) ) {
+ $this->getMain()->markParamsSensitive(
$encParamName );
}
}
@@ -2030,6 +2041,7 @@
$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 3cc7a8a..480915e 100644
--- a/includes/api/ApiCheckToken.php
+++ b/includes/api/ApiCheckToken.php
@@ -73,6 +73,7 @@
'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 e3513da..41bec35 100644
--- a/includes/api/ApiLogin.php
+++ b/includes/api/ApiLogin.php
@@ -250,6 +250,7 @@
'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 a1fac0c..b5eff70 100644
--- a/includes/api/ApiMain.php
+++ b/includes/api/ApiMain.php
@@ -161,6 +161,7 @@
private $mCacheMode = 'private';
private $mCacheControl = [];
private $mParamsUsed = [];
+ private $mParamsSensitive = [];
/** @var bool|null Cached return value from
self::lacksSameOriginSecurity() */
private $lacksSameOriginSecurity = null;
@@ -1602,13 +1603,17 @@
" {$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 {
@@ -1659,6 +1664,24 @@
}
/**
+ * 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 fee0b78..f8f6e7d 100644
--- a/includes/api/ApiQueryWatchlist.php
+++ b/includes/api/ApiQueryWatchlist.php
@@ -475,7 +475,8 @@
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 116f219..b0b1cde 100644
--- a/includes/api/ApiQueryWatchlistRaw.php
+++ b/includes/api/ApiQueryWatchlistRaw.php
@@ -170,7 +170,8 @@
ApiBase::PARAM_TYPE => 'user'
],
'token' => [
- ApiBase::PARAM_TYPE => 'string'
+ ApiBase::PARAM_TYPE => 'string',
+ ApiBase::PARAM_SENSITIVE => true,
],
'dir' => [
ApiBase::PARAM_DFLT => 'ascending',
--
To view, visit https://gerrit.wikimedia.org/r/346841
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I1d61f4dcf792d77401ee2e2988b1afcb2a2ad58f
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/core
Gerrit-Branch: master
Gerrit-Owner: Chad <ch...@wikimedia.org>
Gerrit-Reviewer: Anomie <bjor...@wikimedia.org>
Gerrit-Reviewer: Chad <ch...@wikimedia.org>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
