Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow for a validation regex for the next_link query parameter #285

Merged
merged 4 commits into from
Mar 23, 2020

Conversation

anoadragon453
Copy link
Member

@anoadragon453 anoadragon453 commented Feb 26, 2020

Allows setting a regex to validate the next_link query parameter on creating and validating 3pid validation sessions.

CI is expected to fail :/

@anoadragon453 anoadragon453 requested a review from a team February 26, 2020 17:04
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.

looks good, though I'm slightly concerned that a regex is a bit hard to use securely. For example, you need to check the link is https? rather than javascript or data; and http://server\.com.* is insecure, because it matches http://server.com.phishing.site/pwnmydataplease. https://github.com/OWASP/CheatSheetSeries/blob/master/cheatsheets/Unvalidated_Redirects_and_Forwards_Cheat_Sheet.md#validating-urls specifically advises against using a regex due to this sort of footgun.

would a whitelist of domains serve the same purpose but be harder to mess up?

sydent/http/servlets/emailservlet.py Outdated Show resolved Hide resolved
sydent/http/servlets/emailservlet.py Outdated Show resolved Hide resolved
sydent/sydent.py Outdated Show resolved Hide resolved
@anoadragon453
Copy link
Member Author

would a whitelist of domains serve the same purpose but be harder to mess up?

This sounds good, thanks for looking into how easy it is to mess this up.

So a whitelist of domains looking something like:

  • example.org
  • bla.company.com

And then building a regex that looks like https?://$domain/.*?

@richvdh
Copy link
Member

richvdh commented Mar 17, 2020

And then building a regex that looks like https?://$domain/.*?

why build a regex? Follow the advice at https://github.com/OWASP/CheatSheetSeries/blob/master/cheatsheets/Unvalidated_Redirects_and_Forwards_Cheat_Sheet.md#validating-urls:

  • parse the link using parse_url or whatever it's called
  • check the scheme is http / https
  • check if the domain matches any of the entries in the whitelist

Obligatory jwz quote:

Some people, when confronted with a problem, think “I know, I'll use regular expressions.” Now they have two problems.

@anoadragon453 anoadragon453 force-pushed the anoa/validate_next_link branch from 6465db3 to 795f85f Compare March 19, 2020 13:50
@anoadragon453 anoadragon453 force-pushed the anoa/validate_next_link branch from 795f85f to 0b1e169 Compare March 19, 2020 13:51
@anoadragon453 anoadragon453 requested a review from richvdh March 19, 2020 13:51
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.

otherwise seems sane I think.

sydent/validators/common.py Outdated Show resolved Hide resolved
sydent/validators/common.py Outdated Show resolved Hide resolved
sydent/sydent.py Outdated Show resolved Hide resolved
@anoadragon453 anoadragon453 merged commit feb0c7b into dinsic Mar 23, 2020
@anoadragon453 anoadragon453 deleted the anoa/validate_next_link branch March 23, 2020 16:58
anoadragon453 added a commit to matrix-org/synapse that referenced this pull request Sep 8, 2020
…omain whitelist (#8275)

This is a config option ported over from DINUM's Sydent: matrix-org/sydent#285

They've switched to validating 3PIDs via Synapse rather than Sydent, and would like to retain this functionality.

This original purpose for this change is phishing prevention. This solution could also potentially be replaced by a similar one to #8004, but across all `*/submit_token` endpoint.

This option may still be useful to enterprise even with that safeguard in place though, if they want to be absolutely sure that their employees don't follow links to other domains.
anoadragon453 added a commit to matrix-org/synapse-dinsic that referenced this pull request Sep 9, 2020
…omain whitelist (#8275)

This is a config option ported over from DINUM's Sydent: matrix-org/sydent#285

They've switched to validating 3PIDs via Synapse rather than Sydent, and would like to retain this functionality.

This original purpose for this change is phishing prevention. This solution could also potentially be replaced by a similar one to matrix-org/synapse#8004, but across all `*/submit_token` endpoint.

This option may still be useful to enterprise even with that safeguard in place though, if they want to be absolutely sure that their employees don't follow links to other domains.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants