diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 1c93bc6ed..fbcbe87c0 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -463,6 +463,9 @@ Protect private information, in particular passwords and email addresses. Avoid mechanisms that could be used for tracking where possible (we do need to verify people are logged in for some operations), and ensure that third parties can't use interactions for tracking. +When sending an email to an existing account, use the original account +email not the claimed email address sent now; for why, see +[Hacking GitHub with Unicode's dotless 'i'](https://eng.getwisdom.io/hacking-github-with-unicode-dotless-i/). For more about security, see [security](doc/security.md). We want the software to have decent performance for typical users. diff --git a/app/controllers/password_resets_controller.rb b/app/controllers/password_resets_controller.rb index 925c0b192..8cd27fca1 100644 --- a/app/controllers/password_resets_controller.rb +++ b/app/controllers/password_resets_controller.rb @@ -14,6 +14,12 @@ def new; end def create @user = User.find_by(email: params[:password_reset][:email]) if @user + # Note: We send the password reset to the email address originally + # created by the *original* user, and *not* to the requester + # (who may be attacking the original user's account). This prevents + # attacks where the finding system is "overly generous" and matches + # the "wrong" email address (e.g., exploiting dotless i). See: + # https://eng.getwisdom.io/hacking-github-with-unicode-dotless-i/ reset_password(@user) else flash.now[:danger] = t('password_resets.email_not_found') diff --git a/doc/security.md b/doc/security.md index 3c8e9d2ba..f4d53b1a6 100644 --- a/doc/security.md +++ b/doc/security.md @@ -538,6 +538,19 @@ raw passwords and unencrypted email addresses). These are considered additional hardening measures, and so are discussed further in the section on hardening. +Password reset requests (for local users) trigger an email, but that +email is sent to the address as provided by the original account; +emails are *not* sent to whatever email address is provided by the +reset requestor (who might be an attacker). +These email addresses match in the sense of `find_by`, which is a +case-insensitive match, but since it is sometimes possible for an attacker +to create another email account that "matches" in a case-insensitive way +to an existing account, we always use the known-correct email address. +You can verify this by reviewing +`app/controllers/password_resets_controller.rb`. +This approach completely counters the attack described in +[Hacking GitHub with Unicode's dotless 'i'](https://eng.getwisdom.io/hacking-github-with-unicode-dotless-i/). + #### HTTPS HTTPS (specifically the TLS protocol)