diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6a1cd22..5e6c9a1 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -64,7 +64,16 @@ jobs: - name: Wait for XMPP to become available uses: iFaxity/wait-on-action@v1 with: - resource: tcp:localhost:5222 + resource: tcp:localhost:15222 + timeout: 1800000 + interval: 10000 + delay: 60000 + log: true + + - name: Wait for second XMPP to become available + uses: iFaxity/wait-on-action@v1 + with: + resource: tcp:localhost:25222 timeout: 1800000 interval: 10000 delay: 60000 diff --git a/.gitignore b/.gitignore index e0411b4..32786b2 100644 --- a/.gitignore +++ b/.gitignore @@ -3,6 +3,7 @@ build/ phpunit.xml .phpunit.result.cache +.phpunit.cache/ .phpcs-cache *.log codeclimate.json diff --git a/behat.yml b/behat.yml index 518a741..f8c6c7d 100644 --- a/behat.yml +++ b/behat.yml @@ -5,7 +5,9 @@ default: contexts: - Fabiang\Sasl\Behat\XMPPContext: - localhost - - 5222 + - 15222 + # Extra test server for https://dyn.eightysoft.de/xeps/xep-0474.html + - 25222 - localhost - testuser - testpass diff --git a/docker-compose.yml b/docker-compose.yml index 47d9a3c..54260b1 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -6,8 +6,23 @@ services: - "./tests/config/ejabberd/ejabberd.yml:/home/ejabberd/conf/ejabberd.yml" - "./tests/config/ejabberd/ejabberd.db:/home/ejabberd/database/ejabberd.db" ports: - - "5222:5222" - - "5223:5223" + - "15222:5222" + - "15223:5223" + environment: + - CTL_ON_CREATE=register testuser localhost testpass + + # Extra test server for https://dyn.eightysoft.de/xeps/xep-0474.html + xmpp_source: + build: + context: https://github.com/processone/docker-ejabberd.git#master:ecs + args: + - VERSION=ceee3d3be1fc1053e61bbc81881bd40dbbbc1e89 + volumes: + - "./tests/config/ejabberd/ejabberd.yml:/home/ejabberd/conf/ejabberd.yml" + - "./tests/config/ejabberd/ejabberd.db:/home/ejabberd/database/ejabberd.db" + ports: + - "25222:5222" + - "25223:5223" environment: - CTL_ON_CREATE=register testuser localhost testpass diff --git a/src/Authentication/AbstractAuthentication.php b/src/Authentication/AbstractAuthentication.php index b343100..342fb98 100644 --- a/src/Authentication/AbstractAuthentication.php +++ b/src/Authentication/AbstractAuthentication.php @@ -103,9 +103,11 @@ protected function generateCnonce() /** * Generate downgrade protection string * + * @param string $groupDelimiter + * @param string $delimiter * @return string */ - protected function generateDowngradeProtectionVerification() + protected function generateDowngradeProtectionVerification($groupDelimiter, $delimiter) { $downgradeProtectionOptions = $this->options->getDowngradeProtection(); @@ -119,9 +121,9 @@ protected function generateDowngradeProtectionVerification() usort($allowedMechanisms, array($this, 'sortOctetCollation')); usort($allowedChannelBindings, array($this, 'sortOctetCollation')); - $protect = implode(',', $allowedMechanisms); + $protect = implode($delimiter, $allowedMechanisms); if (count($allowedChannelBindings) > 0) { - $protect .= '|' . implode(',', $allowedChannelBindings); + $protect .= $groupDelimiter. implode($delimiter, $allowedChannelBindings); } return $protect; } diff --git a/src/Authentication/SCRAM.php b/src/Authentication/SCRAM.php index 2ad6d09..779c302 100644 --- a/src/Authentication/SCRAM.php +++ b/src/Authentication/SCRAM.php @@ -165,17 +165,18 @@ private function generateResponse($challenge, $password) $serverMessageRegexp = "#^r=(?[\x21-\x2B\x2D-\x7E/]+)" . ",s=(?(?:[A-Za-z0-9/+]{4})*(?:[A-Za-z0-9/+]{3}=|[A-Za-z0-9/+]{2}==)?)" . ",i=(?[0-9]*)" - . "(,(?.*))?$#"; + . "(?(?:,[A-Za-z]=[^,]+)*)$#"; if (!isset($this->cnonce, $this->gs2Header) || !preg_match($serverMessageRegexp, $challenge, $matches)) { return false; } - $additionalAttributes = $this->parseAdditionalAttributes($matches); + $additionalAttributes = $this->parseAdditionalAttributes($matches['additionalAttr']); //forbidden by RFC 5802 - if(isset($additionalAttributes['m'])) + if (isset($additionalAttributes['m'])) { return false; + } $nonce = $matches['nonce']; $salt = base64_decode($matches['salt']); @@ -191,9 +192,14 @@ private function generateResponse($challenge, $password) return false; } - //SSDP hash - if (!empty($additionalAttributes['d'])) { - if (!$this->downgradeProtection($additionalAttributes['d'])) { + if (! empty($additionalAttributes['h'])) { + if (! $this->downgradeProtection($additionalAttributes['h'], "\x1f", "\x1e")) { + return false; + } + } + + if (! empty($additionalAttributes['d'])) { + if (! $this->downgradeProtection($additionalAttributes['d'], '|', ',')) { return false; } } @@ -215,15 +221,22 @@ private function generateResponse($challenge, $password) /** * @param string $expectedDowngradeProtectionHash + * @param string $groupDelimiter + * @param string $delimiter * @return bool */ - private function downgradeProtection($expectedDowngradeProtectionHash) + private function downgradeProtection($expectedDowngradeProtectionHash, $groupDelimiter, $delimiter) { if ($this->options->getDowngradeProtection() === null) { return true; } - $actualDgPHash = base64_encode(call_user_func($this->hash, $this->generateDowngradeProtectionVerification())); + $actualDgPHash = base64_encode( + call_user_func( + $this->hash, + $this->generateDowngradeProtectionVerification($groupDelimiter, $delimiter) + ) + ); return $expectedDowngradeProtectionHash === $actualDgPHash; } @@ -248,19 +261,26 @@ private function hi($str, $salt, $i) /** * This will parse all non-fixed-position additional SCRAM attributes (the optional ones and the m-attribute) - * @param array $matches The array returned by our regex match, MUST contain an 'additionalAttributes' key + * + * @param string $additionalAttributes 'additionalAttributes' string * @return array */ - private function parseAdditionalAttributes($matches) + private function parseAdditionalAttributes($additionalAttributes) { - $additionalAttributes=array(); - $tail=explode(',', $matches['additionalAttributes']); - foreach($tail as $entry) - { - $entry=explode("=", $entry, 2); - $additionalAttributes[$entry[0]] = $entry[1]; + if ($additionalAttributes == "") { + return array(); } - return $additionalAttributes; + + $return = array(); + $tail = explode(',', $additionalAttributes); + + foreach ($tail as $entry) { + $entry = explode("=", $entry, 2); + if (count($entry) > 1) { + $return[$entry[0]] = $entry[1]; + } + } + return $return; } /** @@ -274,7 +294,8 @@ private function parseAdditionalAttributes($matches) */ public function verify($data) { - $verifierRegexp = '#^v=((?:[A-Za-z0-9/+]{4})*(?:[A-Za-z0-9/+]{3}=|[A-Za-z0-9/+]{2}==)?)(,(?.*))?$#'; + $verifierRegexp = '#^v=(?(?:[A-Za-z0-9/+]{4})*(?:[A-Za-z0-9/+]{3}=|[A-Za-z0-9/+]{2}==)?)' + . '(?(?:,[A-Za-z]=[^,]+)*)$#'; $matches = array(); if (!isset($this->saltedPassword, $this->authMessage) || !preg_match($verifierRegexp, $data, $matches)) { @@ -282,13 +303,13 @@ public function verify($data) return false; } - $additionalAttributes = $this->parseAdditionalAttributes($matches); + $additionalAttribute = $this->parseAdditionalAttributes($matches['additionalAttr']); - //forbidden by RFC 5802 - if(isset($additionalAttributes['m'])) + if (isset($additionalAttribute['m'])) { return false; + } - $verifier = $matches[1]; + $verifier = $matches['verifier']; $proposedServerSignature = base64_decode($verifier); $serverKey = call_user_func($this->hmac, $this->saltedPassword, "Server Key", true); $serverSignature = call_user_func($this->hmac, $serverKey, $this->authMessage, true); diff --git a/tests/features/bootstrap/AbstractContext.php b/tests/features/bootstrap/AbstractContext.php index f328cf0..17959b7 100644 --- a/tests/features/bootstrap/AbstractContext.php +++ b/tests/features/bootstrap/AbstractContext.php @@ -56,12 +56,12 @@ abstract class AbstractContext protected $logdir; protected $logfile; - protected function connect() + protected function connect($port) { $errno = null; $errstr = null; - $connectionString = "tcp://{$this->hostname}:{$this->port}"; + $connectionString = "tcp://{$this->hostname}:{$port}"; $context = stream_context_create(array( 'ssl' => array( diff --git a/tests/features/bootstrap/AbstractXMPPContext.php b/tests/features/bootstrap/AbstractXMPPContext.php index e9a454a..3953d30 100644 --- a/tests/features/bootstrap/AbstractXMPPContext.php +++ b/tests/features/bootstrap/AbstractXMPPContext.php @@ -55,6 +55,9 @@ abstract class AbstractXMPPContext extends AbstractContext implements Context, S */ protected $domain; + protected $port1; + protected $port2; + /** * @var string */ @@ -81,7 +84,8 @@ abstract class AbstractXMPPContext extends AbstractContext implements Context, S * context constructor through behat.yml. * * @param string $hostname Hostname for connection - * @param integer $port + * @param integer $port1 + * @param integer $port2 * @param string $domain * @param string $username Domain name of server (important for connecting) * @param string $password @@ -90,7 +94,8 @@ abstract class AbstractXMPPContext extends AbstractContext implements Context, S */ public function __construct( $hostname, - $port, + $port1, + $port2, $domain, $username, $password, @@ -98,7 +103,8 @@ public function __construct( $tlsversion = 'tlsv1.2' ) { $this->hostname = $hostname; - $this->port = (int) $port; + $this->port1 = (int) $port1; + $this->port2 = (int) $port2; $this->domain = $domain; $this->username = $username; $this->password = $password; diff --git a/tests/features/bootstrap/POP3Context.php b/tests/features/bootstrap/POP3Context.php index 8d75fca..246b990 100644 --- a/tests/features/bootstrap/POP3Context.php +++ b/tests/features/bootstrap/POP3Context.php @@ -95,7 +95,7 @@ public function __construct($hostname, $port, $username, $password, $logdir) */ public function connectionToPopServer() { - $this->connect(); + $this->connect($this->port); Assert::assertSame("+OK Dovecot ready.\r\n", $this->read()); } diff --git a/tests/features/bootstrap/XMPPContext.php b/tests/features/bootstrap/XMPPContext.php index 29f6f94..d748afa 100644 --- a/tests/features/bootstrap/XMPPContext.php +++ b/tests/features/bootstrap/XMPPContext.php @@ -112,7 +112,16 @@ private function sendStreamStart() */ public function connectionToXmppServer() { - $this->connect(); + $this->connect($this->port1); + $this->sendStreamStart(); + } + + /** + * @Given Connection to second XMPP server + */ + public function connectionToSecondXmppServer(): void + { + $this->connect($this->port2); $this->sendStreamStart(); } diff --git a/tests/features/downgrade_protection_source.feature b/tests/features/downgrade_protection_source.feature new file mode 100644 index 0000000..b4c9e82 --- /dev/null +++ b/tests/features/downgrade_protection_source.feature @@ -0,0 +1,47 @@ +@xmpp @downgradeProtection +Feature: Authentication with a second XMPP server + + Background: + Given Connection to second XMPP server + And Connection is encrypted by STARTTLS + + @scramsha1 + Scenario: Authenticate with xmpp server through scram-sha-1 authentication with downgrade protection + Given xmpp server supports authentication method "SCRAM-SHA-1" + And Downgrade protection is properly set up + When authenticate with method SCRAM-SHA-1 + And response to challenge for SCRAM-SHA-1 + Then should be authenticated at xmpp server with verification + + @scramsha256 + Scenario: Authenticate with xmpp server through scram-sha-256 authentication with downgrade protection + Given xmpp server supports authentication method "SCRAM-SHA-256" + And Downgrade protection is properly set up + When authenticate with method SCRAM-SHA-256 + And response to challenge for SCRAM-SHA-256 + Then should be authenticated at xmpp server with verification + + @scramsha1 + Scenario: Authenticate with xmpp server through scram-sha-1 authentication with downgrade protection with invalid mechanism + Given xmpp server supports authentication method "SCRAM-SHA-1" + And Downgrade protection is properly set up + And We simulate downgrade protection error by adding an auth mechanism + When authenticate with method SCRAM-SHA-1 + And response to challenge for SCRAM-SHA-1 was invalid + Then Server connection should be closed + + @scramsha1 + Scenario: Authenticate with xmpp server through scram-sha-1 authentication with downgrade protection with invalid channel-binding + Given xmpp server supports authentication method "SCRAM-SHA-1" + And Downgrade protection is properly set up + And We simulate downgrade protection error by adding an unsupported channel-binding + When authenticate with method SCRAM-SHA-1 + And response to challenge for SCRAM-SHA-1 was invalid + Then Server connection should be closed + + @scramsha1 + Scenario: Authenticate with xmpp server through scram-sha-1 authentication with downgrade protection is disabled + Given xmpp server supports authentication method "SCRAM-SHA-1" + When authenticate with method SCRAM-SHA-1 + And response to challenge for SCRAM-SHA-1 + Then should be authenticated at xmpp server with verification