From ec76a4614bf2a4e7d9f30ecdd89974b7392df2e9 Mon Sep 17 00:00:00 2001 From: Brad Jorsch 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 | 4 ++++ 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, 47 insertions(+), 3 deletions(-) diff --git a/RELEASE-NOTES-1.28 b/RELEASE-NOTES-1.28 index 3ee3f0509a..338dadbf28 100644 --- a/RELEASE-NOTES-1.28 +++ b/RELEASE-NOTES-1.28 @@ -11,6 +11,8 @@ This is not a release yet! * (T154670) A missing method causing the MySQL installer to fatal in rare circumstances was restored. * (T154672) Un-deprecate ArticleAfterFetchContentObject hook. +* (T125177) SECURITY: API parameters may now be marked as "sensitive" to keep + their values out of the logs. == MediaWiki 1.28 == @@ -201,6 +203,8 @@ This is not a release yet! these hooks by passing an array for $hookData to ApiQueryBase::select() and by calling ApiQueryBase->processRow() before adding a row's data to the result. +* (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 1a42ccce9e..32910d103d 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 506ff73a41..6dfd425664 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' ) { @@ -2366,6 +2377,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 3d2159cf50..2736ff4b2a 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 6ac261dd3a..c4060c5264 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 c8f4460e9f..b464fbf6f5 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; @@ -1491,13 +1492,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 { @@ -1548,6 +1553,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 c30f0cf467..c6b3f0475d 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 806861e800..50417eac13 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.11.0