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

@@ -90,6 +90,8 @@ production.
   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 @@ 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 ===
 

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' );
 		}
 

includes/api/ApiBase.php

@@ -188,6 +188,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 = '*';
@@ -1025,6 +1032,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' ) {
@@ -2030,6 +2041,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(),

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',

includes/api/ApiLogin.php

@@ -250,6 +250,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' ],
 			],
 		];

includes/api/ApiMain.php

@@ -161,6 +161,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;
@@ -1602,13 +1603,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 {
@@ -1658,6 +1663,24 @@ class ApiMain extends ApiBase {
 		$this->mParamsUsed += array_fill_keys( (array)$params, true );
 	}
 
+	/**
+	 * 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

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',

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',