From ece055d9f7eb8d2ec103ac65f9ed8bcfb67ceab1 Mon Sep 17 00:00:00 2001 From: Brad Jorsch Date: Thu, 18 Aug 2016 13:37:05 -0400 Subject: [PATCH 03/10] 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.27 | 4 ++++ includes/api/ApiAuthManagerHelper.php | 1 + includes/api/ApiBase.php | 13 +++++++++++++ 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, 48 insertions(+), 3 deletions(-) diff --git a/RELEASE-NOTES-1.27 b/RELEASE-NOTES-1.27 index 3d4dfd3..1a054a5 100644 --- a/RELEASE-NOTES-1.27 +++ b/RELEASE-NOTES-1.27 @@ -27,6 +27,8 @@ was released. 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. == MediaWiki 1.27.1 == @@ -341,6 +343,8 @@ The following PHP extensions are strongly recommended: * Added action=changeauthenticationdata. * Added action=removeauthenticationdata. * Added action=resetpassword. +* (T125177) SECURITY: API parameters may now be marked as "sensitive" to keep + their values out of the logs. === Action API internal changes in 1.27 === * ApiQueryORM removed. diff --git a/includes/api/ApiAuthManagerHelper.php b/includes/api/ApiAuthManagerHelper.php index da7c623..bdf1738 100644 --- a/includes/api/ApiAuthManagerHelper.php +++ b/includes/api/ApiAuthManagerHelper.php @@ -171,6 +171,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 40cc90a..7172e4d 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,11 @@ abstract class ApiBase extends ContextSource { $type = 'NULL'; // allow everything } } + + if ( $type == 'password' || !empty( $paramSettings[self::PARAM_SENSITIVE] ) ) { + $this->getMain()->markParamsSensitive( $encParamName ); + } + } if ( $type == 'boolean' ) { @@ -2300,6 +2312,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 1798776..93215ca 100644 --- a/includes/api/ApiLogin.php +++ b/includes/api/ApiLogin.php @@ -264,6 +264,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 7c0fef6..5c9cc86 100644 --- a/includes/api/ApiMain.php +++ b/includes/api/ApiMain.php @@ -145,6 +145,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; @@ -1418,13 +1419,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 { @@ -1474,6 +1479,24 @@ class ApiMain extends ApiBase { $this->mParamsUsed += array_fill_keys( (array)$params, true ); } + /** + * 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 diff --git a/includes/api/ApiQueryWatchlist.php b/includes/api/ApiQueryWatchlist.php index db2cf86..54211e0 100644 --- a/includes/api/ApiQueryWatchlist.php +++ b/includes/api/ApiQueryWatchlist.php @@ -495,7 +495,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 742f8f5..f76b8fb 100644 --- a/includes/api/ApiQueryWatchlistRaw.php +++ b/includes/api/ApiQueryWatchlistRaw.php @@ -185,7 +185,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