jenkins-bot has submitted this change and it was merged.
Change subject: Improved the VERP generation by cutting down the hmac hash
......................................................................
Improved the VERP generation by cutting down the hmac hash
The $local_part of a standard email should be < 64 which was not
taken care of earlier, and there was high chances of crossing it.
Cutting down securely the hmac hash looks fine, as per security
experts.
Added a prefix such as 'wiki-' so that the email-id looks good, and
easy to parse by exim.
Added documentation.
Change-Id: I167c018be679f6f1535824e94d5aa380dda069a7
---
M BounceHandler.php
M BounceHandlerHooks.php
M includes/ProcessBounceEmails.php
M includes/VerpAddressGenerator.php
M tests/VERPEncodeDecodeTest.php
5 files changed, 33 insertions(+), 17 deletions(-)
Approvals:
Legoktm: Looks good to me, approved
jenkins-bot: Verified
diff --git a/BounceHandler.php b/BounceHandler.php
index 9e34cd2..f3f6b0a 100644
--- a/BounceHandler.php
+++ b/BounceHandler.php
@@ -49,11 +49,12 @@
/**
* VERP Configurations
- * wgEnableVERP - Engales VERP for bounce handling
- * wgVERPalgo - Algorithm to hash the return path address.Possible algorithms
are
+ * wgVERPprefix - The prefix of the VERP address.
+ * wgVERPalgorithm - Algorithm to hash the return path address.Possible
algorithms are
* md2. md4, md5, sha1, sha224, sha256, sha384, ripemd128, ripemd160,
whirlpool and more.
* wgVERPsecret - The secret key to hash the return path address
*/
+$wgVERPprefix = 'wiki';
$wgVERPalgorithm = 'md5';
$wgVERPsecret = 'MediawikiVERP';
$wgVERPAcceptTime = 259200; //3 days time
diff --git a/BounceHandlerHooks.php b/BounceHandlerHooks.php
index 46d2acd..cec0d1a 100644
--- a/BounceHandlerHooks.php
+++ b/BounceHandlerHooks.php
@@ -29,11 +29,11 @@
* Process a given $to address and return its VERP return path
*
* @param MailAddress $to
- * @param string returnPath return-path address
+ * @param string $returnPath return-path address
* @return bool true
*/
public static function generateVerp( MailAddress $to, &$returnPath ) {
- global $wgVERPalgorithm, $wgVERPsecret, $wgServer, $wgSMTP;
+ global $wgVERPprefix, $wgVERPalgorithm, $wgVERPsecret,
$wgServer, $wgSMTP;
$user = User::newFromName( $to->name );
if ( !$user ) {
return true;
@@ -44,7 +44,7 @@
} else {
return true;
}
- $verpAddress = new VerpAddressGenerator( $wgVERPalgorithm,
$wgVERPsecret, $wgServer, $wgSMTP );
+ $verpAddress = new VerpAddressGenerator( $wgVERPprefix,
$wgVERPalgorithm, $wgVERPsecret, $wgServer, $wgSMTP );
$returnPath = $verpAddress->generateVERP( $uid );
return true;
}
diff --git a/includes/ProcessBounceEmails.php b/includes/ProcessBounceEmails.php
index ffbff15..315c76a 100644
--- a/includes/ProcessBounceEmails.php
+++ b/includes/ProcessBounceEmails.php
@@ -90,12 +90,12 @@
$failedUser = array();
preg_match( '~(.*?)@~', $hashedEmail, $hashedPart );
$hashedVERPPart = explode( '-', $hashedPart[1] );
- $hashedData = $hashedVERPPart[0]. '-'. $hashedVERPPart[1]. '-'.
$hashedVERPPart[2];
- $bounceTime = base_convert( $hashedVERPPart[2], 36, 10 );
- if ( base64_encode( hash_hmac( $wgVERPalgorithm, $hashedData,
$wgVERPsecret, true ) ) === $hashedVERPPart[3] &&
- $currentTime - $bounceTime < $wgVERPAcceptTime ) {
- $failedUser['wikiId'] = str_replace( '.', '-',
$hashedVERPPart[0] );
- $failedUser['rawUserId'] = base_convert(
$hashedVERPPart[1], 36, 10 );
+ $hashedData = $hashedVERPPart[0]. '-'. $hashedVERPPart[1]. '-'.
$hashedVERPPart[2]. '-'. $hashedVERPPart[3];
+ $bounceTime = base_convert( $hashedVERPPart[3], 36, 10 );
+ if ( base64_encode( substr( hash_hmac( $wgVERPalgorithm,
$hashedData, $wgVERPsecret, true ), 0, 12 ) ) === $hashedVERPPart[4]
+ && $currentTime - $bounceTime < $wgVERPAcceptTime ) {
+ $failedUser['wikiId'] = str_replace( '.', '-',
$hashedVERPPart[1] );
+ $failedUser['rawUserId'] = base_convert(
$hashedVERPPart[2], 36, 10 );
$failedUser['rawEmail'] = self::getOriginalEmail(
$failedUser );
$failedUser['bounceTime'] = $bounceTime;
} else {
diff --git a/includes/VerpAddressGenerator.php
b/includes/VerpAddressGenerator.php
index db76ebc..461ca85 100644
--- a/includes/VerpAddressGenerator.php
+++ b/includes/VerpAddressGenerator.php
@@ -33,6 +33,10 @@
/**
* @var string
*/
+ protected $prefix;
+ /**
+ * @var string
+ */
protected $algorithm;
/**
@@ -51,12 +55,14 @@
protected $smtp;
/**
+ * @param string $prefix
* @param string $algorithm
* @param string $secretKey
* @param string $server
* @param array $smtp The SMTP setting configurations
*/
- public function __construct( $algorithm, $secretKey, $server, $smtp ) {
+ public function __construct( $prefix, $algorithm, $secretKey, $server,
$smtp ) {
+ $this->prefix = $prefix;
$this->algorithm = $algorithm;
$this->secretKey = $secretKey;
$this->server = $server;
@@ -65,9 +71,16 @@
/**
* Generate VERP address
+ * The generated hash is cut down to 12 ( 96 bits ) instead of the full
120 bits.
+ * For attacks attempting to recover the hmac key, this makes the
attackers job harder by giving them less information to work from.
+ * This makes brute force attacks easier. An attacker would be able to
brute force the signature by
+ * sending an average of 2^95 emails to us. We would (hopefully) notice
that.
+ * This would make finding a collision slightly easier if the secret
key was known,
+ * but the constraints on each segment (wiki id must be valid,
timestamp needs to be within a certain limit),
+ * combined with the difficulty of finding collisions when the key is
unknown, makes this virtually impossible.
*
- * @param string recipient email
- * @return string ReturnPath address
+ * @param int $uid user-id of the failing user
+ * @return string $ReturnPath address
*/
public function generateVERP( $uid ) {
// Get the time in Unix timestamp to compare with seconds
@@ -83,8 +96,8 @@
// wikiId-base36( $UserID )-base36( $Timestamp )-hash(
$algorithm, $key, $prefix )@$email_domain
// We dont want repeating '-' in our WikiId
$wikiId = str_replace( '-', '.', wfWikiID() );
- $email_prefix = $wikiId. '-'. base_convert( $uid, 10, 36). '-'.
base_convert( $timeNow, 10, 36);
- $verp_hash = base64_encode( hash_hmac( $this->algorithm,
$email_prefix, $this->secretKey, true ) );
+ $email_prefix = $this->prefix. '-'. $wikiId. '-'. base_convert(
$uid, 10, 36). '-'. base_convert( $timeNow, 10, 36);
+ $verp_hash = base64_encode( substr( hash_hmac(
$this->algorithm, $email_prefix, $this->secretKey, true ), 0, 12 ) );
$returnPath = $email_prefix. '-' .$verp_hash. '@'
.$email_domain;
return $returnPath;
}
diff --git a/tests/VERPEncodeDecodeTest.php b/tests/VERPEncodeDecodeTest.php
index 2d4555b..ea5299b 100644
--- a/tests/VERPEncodeDecodeTest.php
+++ b/tests/VERPEncodeDecodeTest.php
@@ -18,12 +18,14 @@
$uid = $user->getId();
+ $prefix = 'wiki';
$algorithm = 'md5';
$secretKey = 'mySecret';
$server = 'http://testwiki.org';
$smtp = array();
$this->setMwGlobals( array(
+ 'wgVERPprefix' => $prefix,
'wgVERPalgorithm' => $algorithm,
'wgVERPsecret' => $secretKey,
'wgServer' => $server,
@@ -32,7 +34,7 @@
);
$this->setMwGlobals( 'wgVERPAcceptTime', 259200 );
- $encodeVERP = new VerpAddressGenerator( $algorithm, $secretKey,
$server, $smtp );
+ $encodeVERP = new VerpAddressGenerator( $prefix, $algorithm,
$secretKey, $server, $smtp );
$encodedAddress = $encodeVERP->generateVERP( $uid );
$decodeVERPwithRegex = new ProcessBounceWithRegex();
--
To view, visit https://gerrit.wikimedia.org/r/153450
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I167c018be679f6f1535824e94d5aa380dda069a7
Gerrit-PatchSet: 3
Gerrit-Project: mediawiki/extensions/BounceHandler
Gerrit-Branch: master
Gerrit-Owner: 01tonythomas <[email protected]>
Gerrit-Reviewer: 01tonythomas <[email protected]>
Gerrit-Reviewer: CSteipp <[email protected]>
Gerrit-Reviewer: Hoo man <[email protected]>
Gerrit-Reviewer: Jgreen <[email protected]>
Gerrit-Reviewer: Legoktm <[email protected]>
Gerrit-Reviewer: Reedy <[email protected]>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
