Skip to content

Commit 9a6718e

Browse files
pimliepimlie
authored
fix: correctly check smtp.auth authentication results (#71)
test: improve & add some more tests for recent changes
1 parent 90248f2 commit 9a6718e

File tree

5 files changed

+164
-50
lines changed

5 files changed

+164
-50
lines changed

‎.gitignore

+1
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
.git
2+
.phpunit*
23
*.bak
34
composer.lock
45
vendor

‎authres_status.php

+77-37
Original file line numberDiff line numberDiff line change
@@ -336,56 +336,96 @@ public function get_authentication_status($headers, $show_statuses = 0, $uid = 0
336336
if (($results = ($headers->others['x-dkim-authentication-results'] ?? '')) && strpos($results, 'none') !== false) {
337337
$status = self::STATUS_NOSIG;
338338
} else {
339-
if ($headers->others['authentication-results'] ?? null) {
339+
$hasAuthenticationResultHeaders = (bool)$headers->others['authentication-results'];
340+
if ($hasAuthenticationResultHeaders) {
340341
$results = $this->rfc5451_extract_authresheader($headers->others['authentication-results']);
341342
$status = 0;
342343
$title = '';
343344

344345
foreach ($results as $result) {
345346
$status = $status | (isset(self::$RFC5451_authentication_results[$result['result']]) ? self::$RFC5451_authentication_results[$result['result']] : self::STATUS_FAIL);
346-
347347
$title .= ($title ? '; ' : '') . $result['title'];
348348
}
349349

350-
if ($status == self::STATUS_PASS) {
351-
/* Verify if its an author's domain signature or a third party
352-
*/
353-
if (preg_match("/[@]([a-zA-Z0-9]+([.][a-zA-Z0-9]+)?\.[a-zA-Z]{2,4})/", $headers->from, $m)) {
350+
if ($status === self::STATUS_PASS || $status === self::STATUS_THIRD) {
351+
// Extract emailaddress from From: header
352+
$authorEmail = '';
353+
$authorDomain = '';
354+
355+
if (preg_match('/<([^>]+)>/', $headers->from, $m)) {
356+
$authorEmail = $m[1];
357+
}
358+
359+
if (str_contains($authorEmail, '@')) {
360+
$authorDomain = explode('@', $authorEmail, 2)[1];
361+
}
362+
363+
if ($authorDomain) {
354364
$title = '';
355-
$authorDomain = $m[1];
356365
$authorDomainFound = false;
357366

367+
/* Verify if its an author's domain signature or a third party
368+
*/
358369
foreach ($results as $result) {
359-
if (in_array($result['method'], array('dkim', 'dmarc', 'domainkeys'))) {
360-
if (is_array($result['props']) && isset($result['props']['header'])) {
361-
$pvalue = '';
362-
363-
// d is required, but still not always present
364-
if (isset($result['props']['header']['d'])) {
365-
$pvalue = $result['props']['header']['d'];
366-
} elseif (isset($result['props']['header']['i'])) {
367-
$pvalue = substr($result['props']['header']['i'], strpos($result['props']['header']['i'], '@') + 1);
368-
} elseif (isset($result['props']['header']['from'])) {
369-
$pvalue = $result['props']['header']['from'];
370-
}
371-
372-
if ($pvalue == $authorDomain || substr($authorDomain, -1 * strlen($pvalue)) == $pvalue) {
373-
$authorDomainFound = true;
374-
375-
if ($status != self::STATUS_PASS) {
376-
$status = self::STATUS_PASS;
377-
$title = $result['title'];
378-
} else {
379-
$title.= ($title ? '; ' : '') . $result['title'];
380-
}
381-
} else {
382-
if ($status == self::STATUS_THIRD) {
383-
$title .= '; ' . $this->gettext('for') . ' ' . $pvalue . ' ' . $this->gettext('by') . ' ' . $result['title'];
384-
} elseif (!$authorDomainFound) {
385-
$status = self::STATUS_THIRD;
386-
$title = $pvalue . ' ' . $this->gettext('by') . ' ' . $result['title'];
387-
}
388-
}
370+
if (
371+
!in_array($result['method'], array('auth', 'dkim', 'dmarc', 'domainkeys'))
372+
|| !is_array($result['props'])
373+
) {
374+
continue;
375+
}
376+
377+
if ($result['method'] === 'auth') {
378+
$method_props = $result['props']['smtp'];
379+
} else {
380+
$method_props = $result['props']['header'];
381+
}
382+
383+
if (!isset($method_props)) {
384+
continue;
385+
}
386+
387+
$pvalue = ''; // pvalue refers to the definition in RFC-5451 (p10)
388+
389+
if (isset($method_props['d'])) {
390+
// d is required, but still not always present
391+
$pvalue = $method_props['d'];
392+
} elseif (isset($method_props['i'])) {
393+
$pvalue = substr($method_props['i'], strpos($method_props['i'], '@') + 1);
394+
} elseif ($result['method'] === 'dmarc' && isset($method_props['from'])) {
395+
// from is used with dmarc results
396+
$pvalue = $method_props['from'];
397+
} elseif ($result['method'] === 'auth' && isset($method_props['mailfrom'])) {
398+
// mailfrom is used with smtp.auth results (RFC-4954)
399+
$pvalue = $method_props['mailfrom'];
400+
}
401+
402+
$isAuthorValid = false;
403+
if ($result['method'] === 'auth') {
404+
$isAuthorValid = $pvalue === $authorEmail;
405+
} else {
406+
$isAuthorValid = $pvalue === $authorDomain;
407+
408+
if (!$isAuthorValid) {
409+
// Check if authorDomain is a subdomain of the signee
410+
$isAuthorValid = str_ends_with($authorDomain, '.' . str_replace('@', '', $pvalue));
411+
}
412+
}
413+
414+
if ($isAuthorValid) {
415+
$authorDomainFound = true;
416+
417+
if ($status != self::STATUS_PASS) {
418+
$status = self::STATUS_PASS;
419+
$title = $result['title'];
420+
} else {
421+
$title.= ($title ? '; ' : '') . $result['title'];
422+
}
423+
} else {
424+
if ($status == self::STATUS_THIRD) {
425+
$title .= '; ' . $this->gettext('for') . ' ' . $pvalue . ' ' . $this->gettext('by') . ' ' . $result['title'];
426+
} elseif (!$authorDomainFound) {
427+
$status = self::STATUS_THIRD;
428+
$title = $pvalue . ' ' . $this->gettext('by') . ' ' . $result['title'];
389429
}
390430
}
391431
}

‎composer.json

+6-1
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@
1818
"pimlie/php-dkim": ">=0.2.2"
1919
},
2020
"require-dev": {
21-
"phpunit/phpunit": "^6.1",
21+
"phpunit/phpunit": "^9.0",
2222
"squizlabs/php_codesniffer": "3.*"
2323
},
2424
"autoload-dev": {
@@ -41,5 +41,10 @@
4141
"scripts": {
4242
"test": "vendor/bin/phpunit",
4343
"cs": "vendor/bin/phpcs ./"
44+
},
45+
"config": {
46+
"allow-plugins": {
47+
"roundcube/plugin-installer": true
48+
}
4449
}
4550
}

‎phpunit.xml

-7
Original file line numberDiff line numberDiff line change
@@ -8,17 +8,10 @@
88
convertWarningsToExceptions="true"
99
processIsolation="false"
1010
stopOnFailure="false"
11-
syntaxCheck="false"
1211
>
1312
<testsuites>
1413
<testsuite name="Package Test Suite">
1514
<directory suffix=".php">tests/</directory>
1615
</testsuite>
1716
</testsuites>
18-
19-
<filter>
20-
<whitelist>
21-
<directory suffix=".php">./</directory>
22-
</whitelist>
23-
</filter>
2417
</phpunit>

‎tests/DomainStatusTest.php

+80-5
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,10 @@ class DomainStatusTest extends TestCase
99
{
1010
public function test_pass_from_subdomain()
1111
{
12-
$headers = $this->create_header_object('mail.domain.net; dkim=pass (1024-bit key; secure) header.d=email.com [email protected] header.b=XXXXXXXX; dkim-atps=neutral', 'Test <[email protected]>');
12+
$headers = $this->create_header_object(
13+
'mail.domain.net; dkim=pass (1024-bit key; secure) header.d=email.com [email protected] header.b=XXXXXXXX; dkim-atps=neutral',
14+
'Test <[email protected]>'
15+
);
1316

1417
$plugin = new authres_status();
1518
$result = $plugin->get_authentication_status($headers);
@@ -23,7 +26,9 @@ public function test_pass_from_subdomain()
2326

2427
public function test_pass_single_signature()
2528
{
26-
$headers = $this->create_header_object('mail.domain.net; dkim=pass (1024-bit key; secure) header.d=email.com [email protected] header.b=XXXXXXXX; dkim-atps=neutral');
29+
$headers = $this->create_header_object(
30+
'mail.domain.net; dkim=pass (1024-bit key; secure) header.d=email.com [email protected] header.b=XXXXXXXX; dkim-atps=neutral'
31+
);
2732

2833
$plugin = new authres_status();
2934
$result = $plugin->get_authentication_status($headers);
@@ -63,21 +68,49 @@ public function test_third_signature()
6368
$this->assertEquals($expected, $result);
6469
}
6570

66-
public function test_smtp_auth_signature()
71+
public function test_smtp_auth_valid_signature()
72+
{
73+
$headers = $this->create_header_object('auth=pass smtp.auth=sendonly [email protected]');
74+
75+
$plugin = new authres_status();
76+
$result = $plugin->get_authentication_status($headers);
77+
78+
$expected = <<<EOT
79+
<img src="plugins/authres_status/images/status_pass.png" alt="signaturepass" title="Valid signature(s) from the sender's domain. verified by auth=pass" class="authres-status-img" />
80+
EOT;
81+
82+
$this->assertEquals($expected, $result);
83+
}
84+
85+
public function test_smtp_auth_thirdparty_signature()
6786
{
6887
$headers = $this->create_header_object('auth=pass smtp.auth=sendonly [email protected]');
6988

7089
$plugin = new authres_status();
7190
$result = $plugin->get_authentication_status($headers);
7291

7392
$expected = <<<EOT
74-
<img src="plugins/authres_status/images/status_pass.png" alt="signaturepass" title="Valid signature(s) from the sender's domain. verified by " class="authres-status-img" />
93+
<img src="plugins/authres_status/images/status_third.png" alt="thirdparty" title="Signed by third party, signature is present but for different domain than from address. verified for [email protected] by auth=pass" class="authres-status-img" />
7594
EOT;
7695

7796
$this->assertEquals($expected, $result);
7897
}
7998

80-
public function test_arc_header()
99+
public function test_dmarc_signature()
100+
{
101+
$headers = $this->create_header_object('dmarc=pass (p=none dis=none) header.from=email.com');
102+
103+
$plugin = new authres_status();
104+
$result = $plugin->get_authentication_status($headers);
105+
106+
$expected = <<<EOT
107+
<img src="plugins/authres_status/images/status_pass.png" alt="signaturepass" title="Valid signature(s) from the sender's domain. verified by dmarc=pass (p=none dis=none)" class="authres-status-img" />
108+
EOT;
109+
110+
$this->assertEquals($expected, $result);
111+
}
112+
113+
public function test_arc_failed_header()
81114
{
82115
$headers = $this->create_header_object('arc=fail (signature failed)');
83116

@@ -91,6 +124,48 @@ public function test_arc_header()
91124
$this->assertEquals($expected, $result);
92125
}
93126

127+
public function test_arc_none_header()
128+
{
129+
$headers = $this->create_header_object('arc=none');
130+
131+
$plugin = new authres_status();
132+
$result = $plugin->get_authentication_status($headers);
133+
134+
$expected = <<<EOT
135+
<img src="plugins/authres_status/images/status_nosig.png" alt="nosignature" title="No signature informationarc=none" class="authres-status-img" />
136+
EOT;
137+
138+
$this->assertEquals($expected, $result);
139+
}
140+
141+
public function test_dkim_with_arc_none_header()
142+
{
143+
$headers = $this->create_header_object('mail.domain.net; dkim=pass (1024-bit key; secure) header.d=email.com [email protected] header.b=XXXXXXXX; dkim-atps=neutral; arc=none');
144+
145+
$plugin = new authres_status();
146+
$result = $plugin->get_authentication_status($headers);
147+
148+
$expected = <<<EOT
149+
<img src="plugins/authres_status/images/status_partial_pass.png" alt="partialpass" title="Some signatures are invalid but at least one is valid for the sender's domain. verified by dkim=pass (1024-bit key; secure); arc=none" class="authres-status-img" />
150+
EOT;
151+
152+
$this->assertEquals($expected, $result);
153+
}
154+
155+
public function test_dkim_ok_but_dmarc_fail()
156+
{
157+
$headers = $this->create_header_object('mail.domain.net; dkim=pass header.d=email.com [email protected]; dmarc=fail');
158+
159+
$plugin = new authres_status();
160+
$result = $plugin->get_authentication_status($headers);
161+
162+
$expected = <<<EOT
163+
<img src="plugins/authres_status/images/status_partial_pass.png" alt="partialpass" title="Some signatures are invalid but at least one is valid for the sender's domain. verified by dkim=pass; dmarc=fail" class="authres-status-img" />
164+
EOT;
165+
166+
$this->assertEquals($expected, $result);
167+
}
168+
94169
protected function create_header_object($authres_header, $from = 'Test <[email protected]>')
95170
{
96171
$headers = new stdClass;

0 commit comments

Comments
 (0)