From 26607b9c8858e671846cdf175f0827bd6835f207 Mon Sep 17 00:00:00 2001 From: Brad Jorsch Date: Thu, 18 Aug 2016 13:37:05 -0400 Subject: [PATCH] [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.23 | 4 ++++ includes/api/ApiBase.php | 12 ++++++++++++ includes/api/ApiLogin.php | 1 + includes/api/ApiMain.php | 26 +++++++++++++++++++++++++- includes/api/ApiQueryWatchlist.php | 3 ++- includes/api/ApiQueryWatchlistRaw.php | 3 ++- 6 files changed, 46 insertions(+), 3 deletions(-) diff --git a/RELEASE-NOTES-1.23 b/RELEASE-NOTES-1.23 index 4e1116a..ffa7738 100644 --- a/RELEASE-NOTES-1.23 +++ b/RELEASE-NOTES-1.23 @@ -7,6 +7,8 @@ This is not a release yet! === Changes since 1.23.15 === * (T68404) CSS3 attr() function with url type is no longer allowed in inline styles. +* (T125177) SECURITY: API parameters may now be marked as "sensitive" to keep + their values out of the logs. == MediaWiki 1.23.15 == @@ -504,6 +506,8 @@ release and submitting bug reports. a generator. * (bug 24782) Various modules will now use unique continuation parameters. * (bug 63249) Cache RecentChanges Atom feed in varnish for 15 seconds. +* (T125177) SECURITY: API parameters may now be marked as "sensitive" to keep + their values out of the logs. === Languages updated in 1.23 === diff --git a/includes/api/ApiBase.php b/includes/api/ApiBase.php index cf96ac2..d6e75ab 100644 --- a/includes/api/ApiBase.php +++ b/includes/api/ApiBase.php @@ -65,6 +65,13 @@ abstract class ApiBase extends ContextSource { // Only applies if TYPE='integer' Use with extreme caution const PARAM_RANGE_ENFORCE = 9; + /** + * (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; + // Name of property group that is on the root element of the result, // i.e. not part of a list const PROP_ROOT = 'ROOT'; @@ -668,6 +675,7 @@ abstract class ApiBase extends ContextSource { foreach ( array_keys( $tokenFunctions ) as $token ) { $props[''][$token . 'token'] = array( ApiBase::PROP_TYPE => 'string', + ApiBase::PARAM_SENSITIVE => true, ApiBase::PROP_NULLABLE => true ); } @@ -1027,6 +1035,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' ) { diff --git a/includes/api/ApiLogin.php b/includes/api/ApiLogin.php index f2a9d1a..3a964d6 100644 --- a/includes/api/ApiLogin.php +++ b/includes/api/ApiLogin.php @@ -236,6 +236,7 @@ class ApiLogin extends ApiBase { ), 'token' => array( ApiBase::PROP_TYPE => 'string', + ApiBase::PARAM_SENSITIVE => true, ApiBase::PROP_NULLABLE => true ), 'details' => array( diff --git a/includes/api/ApiMain.php b/includes/api/ApiMain.php index baabaec..b2c5b89 100644 --- a/includes/api/ApiMain.php +++ b/includes/api/ApiMain.php @@ -143,6 +143,7 @@ class ApiMain extends ApiBase { private $mCacheMode = 'private'; private $mCacheControl = array(); private $mParamsUsed = array(); + private $mParamsSensitive = []; /** @var bool|null Cached return value from self::lacksSameOriginSecurity() */ private $lacksSameOriginSecurity = null; @@ -971,13 +972,18 @@ class ApiMain extends ApiBase { ' ' . wfUrlencode( str_replace( ' ', '_', $this->getUser()->getName() ) ) . ' ' . $request->getIP() . ' T=' . $milliseconds . 'ms'; + + $sensitive = array_flip( $this->getSensitiveParams() ); foreach ( $this->getParamsUsed() as $name ) { $value = $request->getVal( $name ); if ( $value === null ) { continue; } $s .= ' ' . $name . '='; - if ( strlen( $value ) > 256 ) { + if ( isset( $sensitive[$name] ) ) { + $value = '[redacted]'; + $encValue = '[redacted]'; + } elseif ( strlen( $value ) > 256 ) { $encValue = $this->encodeRequestLogValue( substr( $value, 0, 256 ) ); $s .= $encValue . '[...]'; } else { @@ -1011,6 +1017,24 @@ class ApiMain extends ApiBase { return array_keys( $this->mParamsUsed ); } + /** + * 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. */ diff --git a/includes/api/ApiQueryWatchlist.php b/includes/api/ApiQueryWatchlist.php index 2e1ce6c..87d2773 100644 --- a/includes/api/ApiQueryWatchlist.php +++ b/includes/api/ApiQueryWatchlist.php @@ -555,7 +555,8 @@ class ApiQueryWatchlist extends ApiQueryGeneratorBase { ApiBase::PARAM_TYPE => 'user' ), 'token' => array( - ApiBase::PARAM_TYPE => 'string' + ApiBase::PARAM_TYPE => 'string', + ApiBase::PARAM_SENSITIVE => true, ), 'continue' => null, ); diff --git a/includes/api/ApiQueryWatchlistRaw.php b/includes/api/ApiQueryWatchlistRaw.php index f45d0e4..0c160be 100644 --- a/includes/api/ApiQueryWatchlistRaw.php +++ b/includes/api/ApiQueryWatchlistRaw.php @@ -160,7 +160,8 @@ class ApiQueryWatchlistRaw extends ApiQueryGeneratorBase { ApiBase::PARAM_TYPE => 'user' ), 'token' => array( - ApiBase::PARAM_TYPE => 'string' + ApiBase::PARAM_TYPE => 'string', + ApiBase::PARAM_SENSITIVE => true, ), 'dir' => array( ApiBase::PARAM_DFLT => 'ascending', -- 2.9.3