Skip to content

Commit

Permalink
Document that we send emails to correct place (#1401)
Browse files Browse the repository at this point in the history
There's a sneaky attack described in the article by John Gracey titled
"Hacking GitHub with Unicode's dotless 'i'" (Nov 28, 2019),
https://eng.getwisdom.io/hacking-github-with-unicode-dotless-i/
Basically, if you do case-insensitive matches, but then use the
email address provided by an *attacker* to send the email, you
might send it to the wrong place.

We have *never* been vulnerable to this attack.
Still, someone might wonder if we *are* vulnerable to it.
Clearly document that we aren't vulnerable to it, and add
additional comments to ensure that things stay this way.

Signed-off-by: David A. Wheeler <dwheeler@dwheeler.com>
  • Loading branch information
david-a-wheeler committed Feb 26, 2020
1 parent ba3b30f commit 29f09ae
Show file tree
Hide file tree
Showing 3 changed files with 22 additions and 0 deletions.
3 changes: 3 additions & 0 deletions CONTRIBUTING.md
Expand Up @@ -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.
Expand Down
6 changes: 6 additions & 0 deletions app/controllers/password_resets_controller.rb
Expand Up @@ -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

This comment has been minimized.

Copy link
@ciibot

ciibot Sep 30, 2021

Collaborator

Style/CommentAnnotation: Annotation keywords like Note should be all upper case, followed by a colon, and a space, then a note describing the problem. (https://rubystyle.guide#annotate-keywords)

# 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')
Expand Down
13 changes: 13 additions & 0 deletions doc/security.md
Expand Up @@ -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)
Expand Down

0 comments on commit 29f09ae

Please sign in to comment.