Bug 46021 - mailPrimaryAddress and mailAlternativeAddress syntax does not require a local part
mailPrimaryAddress and mailAlternativeAddress syntax does not require a local...
Status: CLOSED FIXED
Product: UCS
Classification: Unclassified
Component: UMC - Mail
UCS 4.2
Other Linux
: P5 normal (vote)
: UCS 4.2-3-errata
Assigned To: Sönke Schwardt-Krummrich
Daniel Tröder
:
Depends on:
Blocks: 46599
  Show dependency treegraph
 
Reported: 2018-01-12 16:07 CET by Stefan Gohmann
Modified: 2018-03-12 13:31 CET (History)
3 users (show)

See Also:
What kind of report is it?: Bug Report
What type of bug is this?: 5: Major Usability: Impairs usability in key scenarios
Who will be affected by this bug?: 1: Will affect a very few installed domains
How will those affected feel about the bug?: 5: Blocking further progress on the daily work
User Pain: 0.143
Enterprise Customer affected?:
School Customer affected?: Yes
ISV affected?:
Waiting Support:
Ticket number: 2018010921000712
Bug group (optional):
Max CVSS v3 score:
best: Patch_Available+


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Stefan Gohmann univentionstaff 2018-01-12 16:07:19 CET
It is possible to add a mailPrimaryAddress via UDM which does not contain a local part which is recognized as a catchall address.
Comment 1 Stefan Gohmann univentionstaff 2018-01-12 16:08:11 CET
Should be fixed as erratum
Comment 2 Florian Best univentionstaff 2018-01-12 16:42:01 CET
diff --git a/management/univention-directory-manager-modules/modules/univention/admin/syntax.py b/management/univention-directory-manager-modules/modules/univention/admin/syntax.py
index 197b672..a4cfb1b 100644
--- a/management/univention-directory-manager-modules/modules/univention/admin/syntax.py
+++ b/management/univention-directory-manager-modules/modules/univention/admin/syntax.py
@@ -1246,7 +1246,7 @@ class emailAddress(simple):
 
 »   @classmethod
 »   def parse(self, text):
-»   »   if '@' not in text:
+»   »   if '@' not in text or text.startswith('@'):
 »   »   »   raise univention.admin.uexceptions.valueError(_('Not a valid email address! (No "@"-character to separate local-part and domain-part)'))
 »   »   return text
 
@@ -1254,7 +1254,7 @@ def parse(self, text):
 class emailAddressTemplate(simple):
 »   min_length = 4
 »   max_length = 0
-»   _re = re.compile("^.*@.*$")
+»   _re = re.compile("^[^@]+@.*$")
 
 »   @classmethod
 »   def parse(self, text):
Comment 3 Florian Best univentionstaff 2018-01-12 18:59:44 CET
Patch has been commited.

univention-directory-manager-modules (12.0.18-19)
3f8bd1061187 | Bug #46021: prevent empty local part in mail addresses
7a7a8efc0071 | Bug #46021: prevent empty local part in mail addresses

univention-directory-manager-modules.yaml
3f8bd1061187 | Bug #46021: prevent empty local part in mail addresses

Merged also to UCS 4.3.
Comment 5 Daniel Tröder univentionstaff 2018-01-24 11:48:41 CET
OK: syntax.py modifcation
OK: manual functional test: modification and creation of users with either mailPrimaryAddress or mailAlternativeAddress without local part is not possible anymore
OK advisory (added build number in 5f9b778edda)

Almost verified, except that the mail maintainers have decided that all mail bugs must be tested and documented now. No documentation is needed for this bug,  but:

REOPEN: please write a ucs-test that tries to
a1) create and
a2) modify users by setting
b1) a mailPrimaryAddress
b2) a mailAlternativeAddress
c1) with local part
c2) without local part
d1) only '@'
d2) with domain
d3) without domain

Full matrix please: 2*2*2*3 UDM calls, of which only a*+b*+c1+d2 should succeed.
Comment 6 Sönke Schwardt-Krummrich univentionstaff 2018-01-25 13:24:29 CET
REOPENED:

According to https://tools.ietf.org/html/rfc3696 the local part may contain "@" characters. Examples:
Abc\@def@example.com
"Abc@def"@example.com

We should also check if the domain part is empty. Suggestion for a simplier test without regexp:
- len(addr) >= 3    → at least 3 characters → e.g. a@b
- not(addr.startswith('@'))  → does not start with '@' → has to be quoted
- '@' in addr[1:-1] → at least one @ in the middle part (without first and 
  last character)
- bool(addr.rsplit('@', 1)[-1]) → the domain part after last '@' is non-empty
Comment 7 Daniel Tröder univentionstaff 2018-01-25 13:39:30 CET
(In reply to Sönke Schwardt-Krummrich from comment #6)
> REOPENED:
> 
> According to https://tools.ietf.org/html/rfc3696 the local part may contain
> "@" characters. Examples:
> Abc\@def@example.com
> "Abc@def"@example.com
Yes - almost everything (incl white space!) is allowed if it is escaped.
IMHO we should not allow this kind of address, because 1) it's most certainly a user error and 2) not all email server software will work properly with that.
That's why public email providers don't allow them.

> We should also check if the domain part is empty. Suggestion for a simplier
> test without regexp:
> - len(addr) >= 3    → at least 3 characters → e.g. a@b
> - not(addr.startswith('@'))  → does not start with '@' → has to be quoted
> - '@' in addr[1:-1] → at least one @ in the middle part (without first and 
>   last character)
> - bool(addr.rsplit('@', 1)[-1]) → the domain part after last '@' is non-empty
IMHO a regex is more expressive (and fast enough if precompiled):
r'^[^@]+@[^@]+\.[^@]+$' would require an email address of form 'a@b.c' which IMHO is sensible. It will not allow >1 '@' and the '@' must not be at front of end.
Comment 8 Daniel Tröder univentionstaff 2018-01-25 13:40:27 CET
/of end/or end/
Comment 9 Sönke Schwardt-Krummrich univentionstaff 2018-01-25 14:38:43 CET
As discussed, the syntax emailAddress will be fixed and emailAddressTemplate will become a subclass of it.

The check will now prevent leading or trailing '@' characters and requires a '@' somewhere within the string.

univention-directory-manager-modules (12.0.18-20)
6fabe4d1b141 | Bug #46021: prevent empty local part or domain part in mail addresses
dbf712f0a548 | Bug #46021: add changelog entry

ucs-test (7.0.23-114)
fee5e908cef9 | Bug #46021: add check for emailAddress and emailAddressTemplate
29fe108e11f1 | Bug #46021: stop CLI server after cleanup
16adc4b31cfb | Bug #46021: add changelog entry

univention-directory-manager-modules.yaml
e402bc7b6440 | Bug #46021: update advisory

Package: univention-directory-manager-modules
Version: 12.0.18-20A~4.2.0.201801251423
Branch: ucs_4.2-0
Scope: errata4.2-3

Package: ucs-test
Version: 7.0.23-114A~4.2.0.201801251431
Branch: ucs_4.2-0
Scope: errata4.2-3

Package: univention-directory-manager-modules
Version: 13.0.4-1A~4.3.0.201801251422
Branch: ucs_4.3-0

Package: ucs-test
Version: 8.0.20-1A~4.3.0.201801251432
Branch: ucs_4.3-0


→ Waiting for jenkins results
Comment 10 Sönke Schwardt-Krummrich univentionstaff 2018-02-06 15:15:50 CET
No problems found in regular jenkins job results that may be caused by this bug/modification.
Comment 11 Daniel Tröder univentionstaff 2018-02-13 09:27:11 CET
OK: code change
OK: advisory
OK: ucs-test
OK: no additional docu
OK: stable jenkins results
OK: merge to 4.3
Comment 12 Arvid Requate univentionstaff 2018-02-14 13:31:47 CET
<http://errata.software-univention.de/ucs/4.2/287.html>