Fix inconsistent handling of upper and lower cases of email addresses. by dklimpel · Pull Request #7021 · matrix-org/synapse

dklimpel · 2020-03-02T21:34:43Z

Pull Request Checklist

Pull request is based on the develop branch
Pull request includes a changelog file. The entry should:
- Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
- Use markdown where necessary, mostly for code blocks.
- End with either a period (.) or an exclamation mark (!).
- Start with a capital letter.
Pull request includes a sign off
Code style is correct (run the linters)

Signed-off-by: Dirk Klimpel dirk@klimpel.org

babolivier · 2020-03-12T19:56:36Z

Heya, and thanks for this PR :) fyi matrix-org/matrix-spec-proposals#2265 mandates case folding rather than lower case conversion, so if you could update your PR to reflect that it'd be great!

richvdh · 2020-04-21T12:29:26Z

@dklimpel are you still interested in working on this?

dklimpel · 2020-04-29T13:31:04Z

Yes, it is on my to do list.

richvdh

thanks for this; generally it seems like a good idea.

As well as the largely minor points below, please could you update some of the tests to check that the new behaviour is working as expected at the higher level? In particular tests.rest.client.v2_alpha.test_account looks like it tests this sort of area and would be easy to extend to cover the new code.

synapse/util/threepids.py

synapse/handlers/auth.py

synapse/rest/admin/users.py

richvdh · 2020-05-13T11:45:03Z

synapse/rest/client/v1/login.py

-                # For emails, transform the address to lowercase.
-                # We store all email addreses as lowercase in the DB.
-                # (See add_threepid in synapse/handlers/auth.py)
-                address = address.lower()
+                address = canonicalise_email(address)


for the record, I have checked that there are no users on matrix.org that will be locked out by this change (there are no users with non-ascii characters in their email addresses). I'm reasonably happy to assume that if it's not a problem on matrix.org, it's not a problem anywhere.

synapse/rest/client/v2_alpha/account.py

synapse/rest/client/v2_alpha/register.py

better email address validation additional tests in `tests.rest.client.v2_alpha.test_account`

richvdh

thanks very much for this, it's looking better. The new tests in particular look great!

a few comments still though, I'm afraid.

synapse/rest/client/v1/login.py

synapse/rest/client/v2_alpha/account.py

richvdh · 2020-05-15T14:47:55Z

synapse/util/threepids.py

+    Returns:
+        (str) The canonical form of the email address
+    Raises:
+        SynapseError if the address could not be parsed.


I don't think it's correct that utility functions like this - which could be used anywhere - should decide what HTTP error code to return. You should raise a ValueError or similar, and turn it into a SynapseError in the rest layer.

I am not sure if I should catch error in auth.py.
2786638

synapse/util/threepids.py

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

richvdh · 2020-06-02T10:56:27Z

stupid question, but isn't this implementation susceptible to exactly the same CVE that @clokep called out?

changelog.d/7021.bugfix

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

dklimpel · 2020-06-02T14:06:04Z

stupid question, but isn't this implementation susceptible to exactly the same CVE that @clokep called out?

I am not sure. There is a test with email.utils.parseaddr and a check for one @. It is not a full validation. At the moment synapse has no validation.
Would you like a validation like?

https://github.com/django/django/blob/06c8565a4650b359bdfa59f9707eaa0d1dfd7223/tests/validators/tests.py#L43-L91
https://github.com/django/django/blob/06c8565a4650b359bdfa59f9707eaa0d1dfd7223/django/core/validators.py#L155-L221

richvdh · 2020-06-02T14:34:39Z

I am not sure

This is not a reassuring answer :/. I think its important to understand this before changing this code.

I don't think the CVE is anything to do with validation. The point is: suppose example.com allows people to register for email accounts, and that their email addresses are case sensitive. Now suppose you have registered dkimpel@example.com for your synapse account. Now I sign up for DKIMPEL@example.com, and ask for a password reset from synapse. Hurrah, I have taken over your account.

dklimpel · 2020-06-02T15:09:21Z

Ok. Okay. It's clearer now.
And you are right.
The mail is sent to the address provided by the requester. Not to the address from the database.
This needs a litte more work.
E.g. here:

synapse/synapse/rest/client/v2_alpha/account.py

Lines 124 to 132 in 35560eb

    
           else: 
        
               # Send password reset emails from Synapse 
        
               sid = await self.identity_handler.send_threepid_validation( 
        
                   email, 
        
                   client_secret, 
        
                   send_attempt, 
        
                   self.mailer.send_password_reset_mail, 
        
                   next_link, 
        
               )

richvdh

thanks for continuing your work on this @dklimpel !

synapse/rest/client/v2_alpha/account.py

richvdh · 2020-06-17T15:45:27Z

synapse/rest/client/v2_alpha/account.py

+        # Get the email addresses of the user from database
+        # The user can own more than one address. Lookup for the right address.
+        # The email will be sent to the stored address.
+        # It should prevent potential account hijack via password reset form,
+        # if some compare algorithm are not exactly.
+        addresses = await self.account_validity_handler._get_email_addresses_for_user(
+            existing_user_id
+        )
+        for address in addresses:
+            if address == email:
+                email_from_database = address
+        if not email_from_database:
+            raise SynapseError(400, "Email not found", Codes.THREEPID_NOT_FOUND)
+


I'm afraid I've forgotten what we said in previous iterations of this PR, but looking at this again:

surely this is redundant and we can just send the password reset to the canonicalised address email? get_user_id_by_threepid will fail unless it exactly matches the email in the database?

Yes, it's a double check. I'm extra careful and a little paranoid.
Should I remove the double check?

I think it adds complication and confusion without adding much value. Yes, please remove it.

richvdh · 2020-06-17T15:51:14Z

synapse/util/threepids.py

+    parsedAddress = email.utils.parseaddr(address)[1]
+
+    # parseaddr does not find missing "@"
+    regex = r"^[^@]+@[^@]+\.[^@]+$"


this feels like a very complicated way to check that a string contains a single @ character? just do address.split('@') and check the length of the result?

I'm going to change it. The function is to canonicalize and not to validate.

richvdh · 2020-06-17T15:54:13Z

synapse/util/threepids.py

+    address = address.strip()
+    # Validate address
+    # See https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2019-11340
+    parsedAddress = email.utils.parseaddr(address)[1]


what is the object of this? email.utils.parseaddr is not intended as a way to validate addresses, so I think it is redundant.

You use this check in sydent: matrix-org/sydent@4e1cfff
But it does not find a missing @

I have no real idea why sydent does that; I wouldn't recommend cargo-culting it. Suggest removing it.

synapse/util/threepids.py

richvdh

lgtm, will merge when CI completes!

* commit '5cdca53aa': Merge different Resource implementation classes (#7732) Fix inconsistent handling of upper and lower cases of email addresses. (#7021) Allow YAML config file to contain None (#7779) Fix a typo. Move 1.15.2 after 1.16.0rc2. 1.16.0rc2 Remove an extraneous space. Add links to the fixes. Fix tense in the release notes. Hack to add push priority to push notifications (#7765) Add early returns to `_check_for_soft_fail` (#7769) Use symbolic names for replication stream names (#7768) Type checking for `FederationHandler` (#7770) Fix new metric where we used ms instead of seconds (#7771) Fix incorrect error message when database CTYPE was set incorrectly. (#7760) Pin link in CHANGES.md Fixes to CHANGES.md

Fix inconsistent handling of upper and lower cases of email addresses.

ced5716

clokep mentioned this pull request Mar 9, 2020

Add validation of format for 3pid and add validation of 3pid in admin api #7022

Closed

4 tasks

richvdh requested a review from a team March 12, 2020 18:34

anoadragon453 assigned dklimpel Mar 23, 2020

anoadragon453 removed the request for review from a team March 31, 2020 14:31

clokep added the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Apr 2, 2020

dklimpel force-pushed the fix_case_sensitive_mail_address branch 3 times, most recently from 3fb29e4 to 199c1b0 Compare May 2, 2020 18:09

Change validation of mail address from 'lower()' to 'casefold'

61ee8a4

dklimpel force-pushed the fix_case_sensitive_mail_address branch from de4b38a to 61ee8a4 Compare May 2, 2020 19:14

Merge branch 'develop' into fix_case_sensitive_mail_address

b9e25f7

clokep requested a review from a team May 6, 2020 19:49

richvdh removed the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label May 12, 2020

richvdh suggested changes May 13, 2020

View reviewed changes

dklimpel added 3 commits May 13, 2020 23:37

update after review

288d2db

better email address validation additional tests in `tests.rest.client.v2_alpha.test_account`

code style

790c0df

Merge branch 'develop' into fix_case_sensitive_mail_address

590bca8

richvdh self-requested a review May 14, 2020 09:34

richvdh suggested changes May 15, 2020

View reviewed changes

dklimpel and others added 3 commits May 15, 2020 17:15

Apply suggestions from code review

5cd421b

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

Update error handling after review to ValueError

8aecb7f

Revert error handling in auth.py

2786638

dklimpel requested a review from richvdh May 17, 2020 20:00

richvdh reviewed Jun 2, 2020

View reviewed changes

changelog.d/7021.bugfix Outdated Show resolved Hide resolved

Apply suggestions from code review

35560eb

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

richvdh added the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Jun 5, 2020

Add tests, comments, password reset with mail from database

19218d1

dklimpel requested a review from richvdh June 17, 2020 10:09

clokep removed the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Jun 17, 2020

richvdh suggested changes Jun 17, 2020

View reviewed changes

dklimpel added 3 commits June 17, 2020 19:43

Apply suggestions from code review

f418a20

minor correction

4b2aff9

Merge branch 'develop' into fix_case_sensitive_mail_address

c702ab9

richvdh self-requested a review July 2, 2020 15:48

richvdh reviewed Jul 3, 2020

View reviewed changes

synapse/util/threepids.py Outdated Show resolved Hide resolved

Update synapse/util/threepids.py

6e634e9

richvdh approved these changes Jul 3, 2020

View reviewed changes

richvdh merged commit 21a212f into matrix-org:develop Jul 3, 2020

dklimpel deleted the fix_case_sensitive_mail_address branch July 3, 2020 14:06

dklimpel mentioned this pull request Jul 3, 2020

Threepid whitespace is not trimmed before inserting to database #6398

Closed

dklimpel mentioned this pull request Oct 12, 2020

Email matching for invitations is case-sensitive #8522

Closed

psrpinto mentioned this pull request Oct 13, 2021

Password reset handles Upper Case Characters in email addresses brokenly element-hq/element-web#9556

Closed

clokep mentioned this pull request Dec 9, 2021

Ensure emails are canonicalized before fetching associated user. #11547

Merged

DMRobertson linked an issue Jun 15, 2022 that may be closed by this pull request

Email matching for invitations is case-sensitive #8522

Closed

DMRobertson removed a link to an issue Jun 15, 2022

Email matching for invitations is case-sensitive #8522

Closed

Uh oh!

Conversation

dklimpel commented Mar 2, 2020

Pull Request Checklist

Uh oh!

babolivier commented Mar 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

richvdh commented Apr 21, 2020

Uh oh!

dklimpel commented Apr 29, 2020

Uh oh!

richvdh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

richvdh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

richvdh commented Jun 2, 2020

Uh oh!

Uh oh!

dklimpel commented Jun 2, 2020

Uh oh!

richvdh commented Jun 2, 2020

Uh oh!

dklimpel commented Jun 2, 2020

Uh oh!

richvdh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

richvdh left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

babolivier commented Mar 12, 2020 •

edited

Loading