Skip to content

Commit 246b7b1

Browse files
fabiangfabiang
committed
Implement update to downgrade proection (XEP-0474)
1 parent d410630 commit 246b7b1

File tree

12 files changed

+233
-75
lines changed

12 files changed

+233
-75
lines changed

‎.github/workflows/ci.yml

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -70,14 +70,14 @@ jobs:
7070
delay: 60000
7171
log: true
7272

73-
# - name: Wait for second XMPP to become available
74-
# uses: iFaxity/wait-on-action@v1
75-
# with:
76-
# resource: tcp:localhost:25222
77-
# timeout: 1800000
78-
# interval: 10000
79-
# delay: 60000
80-
# log: true
73+
- name: Wait for second XMPP to become available
74+
uses: iFaxity/wait-on-action@v1
75+
with:
76+
resource: tcp:localhost:25222
77+
timeout: 1800000
78+
interval: 10000
79+
delay: 60000
80+
log: true
8181

8282
- name: Run test suite
8383
run: ./vendor/bin/behat

‎behat.yml

Lines changed: 2 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -6,20 +6,13 @@ default:
66
- Fabiang\SASL\Behat\XMPPContext:
77
- localhost
88
- 15222
9+
# Extra test server for https://dyn.eightysoft.de/xeps/xep-0474.html
10+
- 25222
911
- localhost
1012
- testuser
1113
- testpass
1214
- "%paths.base%/tests/log/features/"
1315
- tlsv1.2
14-
# Extra test server for https://dyn.eightysoft.de/xeps/xep-0474.html
15-
# - Fabiang\SASL\Behat\XMPPContext:
16-
# - localhost
17-
# - 25222
18-
# - localhost
19-
# - testuser
20-
# - testpass
21-
# - "%paths.base%/tests/log/features/"
22-
# - tlsv1.3
2316
- Fabiang\SASL\Behat\POP3Context:
2417
- localhost
2518
- 1110

‎docker-compose.yml

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -11,19 +11,19 @@ services:
1111
- CTL_ON_CREATE=register testuser localhost testpass
1212

1313
# Extra test server for https://dyn.eightysoft.de/xeps/xep-0474.html
14-
# xmpp_source:
15-
# build:
16-
# context: https://github.com/processone/docker-ejabberd.git#master:ecs
17-
# args:
18-
# - VERSION=133d52d04023d603283a7796c46bc40ffc7cd3c2
19-
# volumes:
20-
# - "./tests/config/ejabberd/ejabberd.yml:/home/ejabberd/conf/ejabberd.yml"
21-
# - "./tests/config/ejabberd/ejabberd.db:/home/ejabberd/database/ejabberd.db"
22-
# ports:
23-
# - "25222:5222"
24-
# - "25223:5223"
25-
# environment:
26-
# - CTL_ON_CREATE=register testuser localhost testpass
14+
xmpp_source:
15+
build:
16+
context: https://github.com/processone/docker-ejabberd.git#master:ecs
17+
args:
18+
- VERSION=ceee3d3be1fc1053e61bbc81881bd40dbbbc1e89
19+
volumes:
20+
- "./tests/config/ejabberd/ejabberd.yml:/home/ejabberd/conf/ejabberd.yml"
21+
- "./tests/config/ejabberd/ejabberd.db:/home/ejabberd/database/ejabberd.db"
22+
ports:
23+
- "25222:5222"
24+
- "25223:5223"
25+
environment:
26+
- CTL_ON_CREATE=register testuser localhost testpass
2727

2828
mail:
2929
image: dovecot/dovecot:${DOVECOT_VERSION:-2.3.18}

‎src/Authentication/AbstractAuthentication.php

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ protected function generateCnonce(): string
9494
/**
9595
* Generate downgrade protection string
9696
*/
97-
protected function generateDowngradeProtectionVerification(): string
97+
protected function generateDowngradeProtectionVerification(string $groupDelimiter, string $delimiter): string
9898
{
9999
$downgradeProtectionOptions = $this->options->getDowngradeProtection();
100100
if ($downgradeProtectionOptions === null) {
@@ -108,12 +108,12 @@ protected function generateDowngradeProtectionVerification(): string
108108
return '';
109109
}
110110

111-
usort($allowedMechanisms, [$this, 'sortOctetCollation']);
112-
usort($allowedChannelBindings, [$this, 'sortOctetCollation']);
111+
usort($allowedMechanisms, $this->sortOctetCollation(...));
112+
usort($allowedChannelBindings, $this->sortOctetCollation(...));
113113

114-
$protect = implode(',', $allowedMechanisms);
114+
$protect = implode($delimiter, $allowedMechanisms);
115115
if (count($allowedChannelBindings) > 0) {
116-
$protect .= '|' . implode(',', $allowedChannelBindings);
116+
$protect .= $groupDelimiter . implode($delimiter, $allowedChannelBindings);
117117
}
118118
return $protect;
119119
}

‎src/Authentication/SCRAM.php

Lines changed: 55 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,6 @@
5555
class SCRAM extends AbstractAuthentication implements ChallengeAuthenticationInterface, VerificationInterface
5656
{
5757
private string $hashAlgo;
58-
5958
private ?string $gs2Header = null;
6059
private ?string $cnonce = null;
6160
private ?string $firstMessageBare = null;
@@ -80,7 +79,9 @@ public function __construct(Options $options, string $hash)
8079
// For instance "sha1" is accepted, while the registered hash name should be "SHA-1".
8180
$replaced = preg_replace('#^sha-(\d+)#i', 'sha\1', $hash);
8281
if ($replaced === null) {
82+
// @codeCoverageIgnoreStart
8383
throw new InvalidArgumentException("Could not normalize hash '$hash'");
84+
// @codeCoverageIgnoreEnd
8485
}
8586
$normalizedHash = strtolower($replaced);
8687

@@ -176,22 +177,32 @@ private function generateResponse(
176177
/* @psalm-var array<int,string> $matches */
177178
$matches = [];
178179

180+
$downGradeProtectionRegexp = '(?:[A-Za-z0-9/+]{4})*(?:[A-Za-z0-9/+]{3}=|[A-Za-z0-9/+]{2}==)';
181+
179182
$serverMessageRegexp = "#^r=(?<nonce>[\x21-\x2B\x2D-\x7E/]+)"
180183
. ",s=(?<salt>(?:[A-Za-z0-9/+]{4})*(?:[A-Za-z0-9/+]{3}=|[A-Za-z0-9/+]{2}==)?)"
181184
. ",i=(?<iteration>[0-9]*)"
182-
. "(?:,d=(?<downgradeProtection>(?:[A-Za-z0-9/+]{4})*(?:[A-Za-z0-9/+]{3}=|[A-Za-z0-9/+]{2}==)))?"
183-
. "(,[A-Za-z]=[^,])*$#";
185+
. "(?:,d=(?<downgradeProtection>$downGradeProtectionRegexp))?"
186+
. "(?:,h=(?<downgradeProtectionSecure>$downGradeProtectionRegexp))?"
187+
. "(?<additionalAttr>(?:,[A-Za-z]=[^,]+)*)$#";
184188

185189
if ($this->cnonce === null ||
186190
$this->gs2Header === null ||
187191
$this->firstMessageBare === null ||
188-
!preg_match($serverMessageRegexp, $challenge, $matches)) {
192+
! preg_match($serverMessageRegexp, $challenge, $matches)) {
193+
return false;
194+
}
195+
196+
$additionalAttribute = $this->parseAdditionalAttributes($matches['additionalAttr']);
197+
198+
if (isset($additionalAttribute['m'])) {
189199
return false;
190200
}
191201

192202
$nonce = $matches['nonce'];
193203
$salt = base64_decode($matches['salt']);
194-
if (!$salt) {
204+
205+
if (! $salt) {
195206
// Invalid Base64.
196207
return false;
197208
}
@@ -203,8 +214,14 @@ private function generateResponse(
203214
return false;
204215
}
205216

206-
if ($matches['downgradeProtection'] !== '') {
207-
if (!$this->downgradeProtection($matches['downgradeProtection'])) {
217+
if (! empty($matches['downgradeProtectionSecure'])) {
218+
if (! $this->downgradeProtection($matches['downgradeProtectionSecure'], "\x1f", "\x1e")) {
219+
return false;
220+
}
221+
}
222+
223+
if (! empty($matches['downgradeProtection'])) {
224+
if (! $this->downgradeProtection($matches['downgradeProtection'], '|', ',')) {
208225
return false;
209226
}
210227
}
@@ -224,13 +241,20 @@ private function generateResponse(
224241
return $finalMessage . $proof;
225242
}
226243

227-
private function downgradeProtection(string $expectedDowngradeProtectionHash): bool
228-
{
244+
private function downgradeProtection(
245+
string $expectedDowngradeProtectionHash,
246+
string $groupDelimiter,
247+
string $delimiter
248+
): bool {
229249
if ($this->options->getDowngradeProtection() === null) {
230250
return true;
231251
}
232252

233-
$actualDgPHash = base64_encode($this->hash($this->generateDowngradeProtectionVerification()));
253+
$actualDgPHash = base64_encode(
254+
$this->hash(
255+
$this->generateDowngradeProtectionVerification($groupDelimiter, $delimiter)
256+
)
257+
);
234258
return $expectedDowngradeProtectionHash === $actualDgPHash;
235259
}
236260

@@ -268,16 +292,23 @@ private function hi(
268292
*/
269293
public function verify(string $data): bool
270294
{
271-
$verifierRegexp = '#^v=(?<verifier>(?:[A-Za-z0-9/+]{4})*(?:[A-Za-z0-9/+]{3}=|[A-Za-z0-9/+]{2}==)?)$#';
295+
$verifierRegexp = '#^v=(?<verifier>(?:[A-Za-z0-9/+]{4})*(?:[A-Za-z0-9/+]{3}=|[A-Za-z0-9/+]{2}==)?)'
296+
. '(?<additionalAttr>(?:,[A-Za-z]=[^,]+)*)$#';
272297

273298
$matches = [];
274299
if (empty($this->saltedSecret) ||
275300
$this->authMessage === null ||
276-
!preg_match($verifierRegexp, $data, $matches)) {
301+
! preg_match($verifierRegexp, $data, $matches)) {
277302
// This cannot be an outcome, you never sent the challenge's response.
278303
return false;
279304
}
280305

306+
$additionalAttribute = $this->parseAdditionalAttributes($matches['additionalAttr']);
307+
308+
if (isset($additionalAttribute['m'])) {
309+
return false;
310+
}
311+
281312
$saltedSecret = $this->saltedSecret->getValue();
282313

283314
$verifier = $matches['verifier'];
@@ -288,6 +319,18 @@ public function verify(string $data): bool
288319
return $proposedServerSignature === $serverSignature;
289320
}
290321

322+
private function parseAdditionalAttributes(string $addAttr): array
323+
{
324+
return array_column(
325+
array_map(
326+
fn($v) => explode('=', trim($v), 2),
327+
array_filter(explode(',', $addAttr))
328+
),
329+
1,
330+
0
331+
);
332+
}
333+
291334
private function hash(string $data): string
292335
{
293336
return hash($this->hashAlgo, $data, true);

‎src/Options.php

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -98,15 +98,4 @@ public function getDowngradeProtection(): ?DowngradeProtectionOptions
9898
{
9999
return $this->downgradeProtection;
100100
}
101-
102-
public function toArray(): array
103-
{
104-
return [
105-
'authcid' => $this->getAuthcid(),
106-
'secret' => $this->getSecret(),
107-
'authzid' => $this->getAuthzid(),
108-
'service' => $this->getService(),
109-
'hostname' => $this->getHostname(),
110-
];
111-
}
112101
}

‎tests/features/bootstrap/AbstractContext.php

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -52,20 +52,21 @@
5252
abstract class AbstractContext
5353
{
5454
protected string $hostname;
55-
protected int $port;
55+
protected int $port1;
56+
protected int $port2;
5657
protected string $username;
5758
protected string $password;
5859

5960
protected string $logdir;
6061
protected $stream;
6162
protected $logfile;
6263

63-
protected function connect(): void
64+
protected function connect(int $port): void
6465
{
6566
$errno = null;
6667
$errstr = null;
6768

68-
$connectionString = "tcp://{$this->hostname}:{$this->port}";
69+
$connectionString = "tcp://{$this->hostname}:{$port}";
6970

7071
$context = stream_context_create([
7172
'ssl' => [

‎tests/features/bootstrap/POP3Context.php

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ class POP3Context extends AbstractContext implements Context, SnippetAcceptingCo
7171
public function __construct(string $hostname, string $port, string $username, string $password, string $logdir)
7272
{
7373
$this->hostname = $hostname;
74-
$this->port = (int) $port;
74+
$this->port1 = (int) $port;
7575
$this->username = $username;
7676
$this->password = $password;
7777

@@ -85,7 +85,7 @@ public function __construct(string $hostname, string $port, string $username, st
8585
#[Given('Connection to pop3 server')]
8686
public function connectionToPopServer(): void
8787
{
88-
$this->connect();
88+
$this->connect($this->port1);
8989
Assert::assertSame("+OK Dovecot ready.\r\n", $this->read());
9090
}
9191

‎tests/features/bootstrap/XMPPContext.php

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,8 @@ class XMPPContext extends AbstractContext implements Context, SnippetAcceptingCo
7272
* context constructor through behat.yml.
7373
*
7474
* @param string $hostname Hostname for connection
75-
* @param integer $port
75+
* @param integer $port1
76+
* @param integer $port2
7677
* @param string $domain
7778
* @param string $username Domain name of server (important for connecting)
7879
* @param string $password
@@ -81,15 +82,17 @@ class XMPPContext extends AbstractContext implements Context, SnippetAcceptingCo
8182
*/
8283
public function __construct(
8384
string $hostname,
84-
string $port,
85+
string $port1,
86+
string $port2,
8587
string $domain,
8688
string $username,
8789
string $password,
8890
string $logdir,
8991
string $tlsversion = 'tlsv1.2'
9092
) {
9193
$this->hostname = $hostname;
92-
$this->port = (int) $port;
94+
$this->port1 = (int) $port1;
95+
$this->port2 = (int) $port2;
9396
$this->domain = $domain;
9497
$this->username = $username;
9598
$this->password = $password;
@@ -115,10 +118,17 @@ private function getOptions(): Options
115118
);
116119
}
117120

118-
#[Given('Connection to xmpp server')]
119-
public function connectionToXmppServer(): void
121+
#[Given('Connection to XMPP server')]
122+
public function connectionToXMPPServer(): void
120123
{
121-
$this->connect();
124+
$this->connect($this->port1);
125+
$this->sendStreamStart();
126+
}
127+
128+
#[Given('Connection to second XMPP server')]
129+
public function connectionToSecondXMPPServer(): void
130+
{
131+
$this->connect($this->port2);
122132
$this->sendStreamStart();
123133
}
124134

‎tests/features/downgrade_protection.feature

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
11
@xmpp @downgradeProtection
2-
Feature: Authentication with a xmpp server
2+
Feature: Authentication with a XMPP server
33

44
Background:
5-
Given Connection to xmpp server
5+
Given Connection to XMPP server
66
And Connection is encrypted by STARTTLS
77

88
@scramsha1

0 commit comments

Comments
 (0)