Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

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

Merged
merged 15 commits into from
Jul 3, 2020

Conversation

dklimpel
Copy link
Contributor

@dklimpel dklimpel commented Mar 2, 2020

Fix #7016

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
Copy link
Contributor

babolivier commented Mar 12, 2020

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!

@anoadragon453 anoadragon453 removed the request for review from a team March 31, 2020 14:31
@clokep clokep added the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Apr 2, 2020
@richvdh
Copy link
Member

richvdh commented Apr 21, 2020

@dklimpel are you still interested in working on this?

@dklimpel
Copy link
Contributor Author

Yes, it is on my to do list.

@dklimpel dklimpel force-pushed the fix_case_sensitive_mail_address branch 3 times, most recently from 3fb29e4 to 199c1b0 Compare May 2, 2020 18:09
@dklimpel dklimpel force-pushed the fix_case_sensitive_mail_address branch from de4b38a to 61ee8a4 Compare May 2, 2020 19:14
@clokep clokep requested a review from a team May 6, 2020 19:49
@richvdh richvdh removed the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label May 12, 2020
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
synapse/handlers/auth.py Outdated Show resolved Hide resolved
synapse/rest/admin/users.py Outdated Show resolved Hide resolved
Comment on lines 195 to 196
# 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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
synapse/rest/client/v2_alpha/register.py Outdated Show resolved Hide resolved
dklimpel added 3 commits May 13, 2020 23:37
better email address validation
additional tests in `tests.rest.client.v2_alpha.test_account`
@richvdh richvdh self-requested a review May 14, 2020 09:34
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
synapse/rest/client/v2_alpha/account.py Outdated Show resolved Hide resolved
Returns:
(str) The canonical form of the email address
Raises:
SynapseError if the address could not be parsed.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

synapse/util/threepids.py Outdated Show resolved Hide resolved
synapse/util/threepids.py Outdated Show resolved Hide resolved
synapse/util/threepids.py Outdated Show resolved Hide resolved
dklimpel and others added 3 commits May 15, 2020 17:15
@dklimpel dklimpel requested a review from richvdh May 17, 2020 20:00
@richvdh
Copy link
Member

richvdh commented Jun 2, 2020

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

changelog.d/7021.bugfix Outdated Show resolved Hide resolved
Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
@dklimpel
Copy link
Contributor Author

dklimpel commented Jun 2, 2020

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
Copy link
Member

richvdh commented Jun 2, 2020

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
Copy link
Contributor Author

dklimpel commented Jun 2, 2020

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:

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 richvdh added the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Jun 5, 2020
@dklimpel dklimpel requested a review from richvdh June 17, 2020 10:09
@clokep clokep removed the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Jun 17, 2020
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for continuing your work on this @dklimpel !

synapse/rest/client/v2_alpha/account.py Outdated Show resolved Hide resolved
Comment on lines 119 to 132
# 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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

parsedAddress = email.utils.parseaddr(address)[1]

# parseaddr does not find missing "@"
regex = r"^[^@]+@[^@]+\.[^@]+$"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@richvdh richvdh self-requested a review July 2, 2020 15:48
synapse/util/threepids.py Outdated Show resolved Hide resolved
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, will merge when CI completes!

@richvdh richvdh merged commit 21a212f into matrix-org:develop Jul 3, 2020
@dklimpel dklimpel deleted the fix_case_sensitive_mail_address branch July 3, 2020 14:06
babolivier pushed a commit that referenced this pull request Sep 1, 2021
* 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
@DMRobertson DMRobertson linked an issue Jun 15, 2022 that may be closed by this pull request
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handling of upper and lower case of e-mail addresses
4 participants