From d5f36978cc256f7cdc60a216f8c61383a4539a35 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.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 fac6f7e..6a83c06 100644 --- a/RELEASE-NOTES-1.29 +++ b/RELEASE-NOTES-1.29 @@ -117,6 +117,8 @@ production. 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 5327d7a..28d4f84 100644 --- a/includes/api/ApiAuthManagerHelper.php +++ b/includes/api/ApiAuthManagerHelper.php @@ -169,6 +169,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 b8dd464..6e35dbc 100644 --- a/includes/api/ApiBase.php +++ b/includes/api/ApiBase.php @@ -186,6 +186,13 @@ abstract class ApiBase extends ContextSource { */ 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 = '*'; @@ -1023,6 +1030,10 @@ abstract class ApiBase extends ContextSource { } else { $type = 'NULL'; // allow everything } + + if ( $type == 'password' || !empty( $paramSettings[self::PARAM_SENSITIVE] ) ) { + $this->getMain()->markParamsSensitive( $encParamName ); + } } if ( $type == 'boolean' ) { @@ -2016,6 +2027,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 3cc7a8a..480915e 100644 --- a/includes/api/ApiCheckToken.php +++ b/includes/api/ApiCheckToken.php @@ -73,6 +73,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 6cf1fad..2814660 100644 --- a/includes/api/ApiLogin.php +++ b/includes/api/ApiLogin.php @@ -257,6 +257,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 52f1d95..4b40e46 100644 --- a/includes/api/ApiMain.php +++ b/includes/api/ApiMain.php @@ -159,6 +159,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; @@ -1594,13 +1595,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 { @@ -1651,6 +1656,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 3f59751..72e6252 100644 --- a/includes/api/ApiQueryWatchlist.php +++ b/includes/api/ApiQueryWatchlist.php @@ -475,7 +475,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 a1078a5..56f8d1b 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.1.4