4 ms·

Yes, you're absolutely right. The real bug was sending to the "wrong" matching email. But this is what makes this bug so hard to find. You're looking at "equal"

Yes, you're absolutely right. The real bug was sending to the "wrong" matching email. But this is what makes this bug so hard to find. You're looking at "equal" strings, so why should it make a difference if you pick A or A if A === A ? :)

thaumasiotes 7y ago
Hard to find, but it's the kind of thing that will hopefully come up in code review. Consider this pseudocode with helpful pseudo-hungarian notation: username = request.post_params('username') evil_email = request.post_params('email_address') user = get_user_by_name(username) good_email = user.email_address if good_email != evil_email: # Hackers! else: reset_password(user.id, evil_email) # it's fine; it's the same as user.email_address You can see the problem in the call to reset_password. Sure, it's the same as user.email_address, but if you just used user.email_address you'd be doing the right thing and you wouldn't need the comment about "don't worry; it's fine". Then you start to wonder if that line shouldn't look more like reset_password(user) and from a security perspective, it should. (I assume the reason Github allows the code to specify the email address in the first place is that an account may be associated with multiple addresses.) If it makes no difference which email address you pick, why not pick the one that represents doing something safe instead of the one that represents doing something dangerous?
- jazzabeanie 7y ago
  If they do have multiple email addresses associated with one account, and they don't want to send a password reset to all of them, then you can see how it would happen. ``` if evil_email not in good_email_addresses: # Hackers! else: # just reset with provided email. If I thought there was a potential security issue I would have already addressed it. ``` So easy to be lazy at this point, especially under time pressure.
  - eyegor 7y ago
    Off topic, you need the triple backticks on their own lines for them to render here.
    - codetrotter 7y ago
      Sorry but that is not correct either. Code blocks on HN are created by using two or more leading spaces on each line. Tripple backticks do nothing, and will be shown on the same line as the next line even if put on a separate line, because they are considered as regular text and part of one paragraph of text. See https://news.ycombinator.com/formatdoc https://news.ycombinator.com/formatdoc
- mathewsanders 7y ago
  I’d never heard of Hungarian notation, but searching for info I realize that I’ve seen this pattern so often but never knew it had a name :) TIL https://en.m.wikipedia.org/wiki/Hungarian_notation https://en.m.wikipedia.org/wiki/Hungarian_notation
- LeifCarrotson 7y ago
  The problem is that it requires a particularly detailed way of thinking about strings, much like the popular IEEE 754 floating point number question. If the code were instead written: username = request.post_params('username') email = request.post_params('email_address') user = get_user_by_name(username) if emails_are_equal(email, user.email_address) reset_password(user.id, email) Most people would not feel compelled to use user.email_address in the call to reset_password, because they're guaranteed to be equal by the previous line! The vulnerability was introduced when the author of emails_are_equal() wrote it with conversion to uppercase of the whole thing, and failed to realize that this conversion can collide with Unicode domains. They may also have done things like normalize/remove optional.dots+suffixes@gmail.com, "quoted" or (commented) local parts. There's a lot of magic - and a lot of risk - in an emails_are_equal method.
  - thaumasiotes 7y ago
    > There's a lot of magic - and a lot of risk - in an emails_are_equal method. My whole point is that changing reset_password(user.id, email) to reset_password(user.id, user.email_address) neutralizes that risk. Then it doesn't matter whether emails_are_equal is risky or not, because a failure in emails_are_equal can only cause you to do a safe thing. What's less embarrassing -- "we accidentally sent a password reset for your account to you", or "we accidentally sent a password reset for your account to someone else"? So you might not feel compelled to rewrite reset_password(user.id, email) as reset_password(user.id, user.email_address) -- but you should.
  - nybble41 7y ago
    > They may also have done things like normalize/remove optional.dots+suffixes@gmail.com, "quoted" or (commented) local parts. At least the first two (removing dots and suffixes), along with conversion to lowercase, are strictly invalid transformations which may result in an email address referring to a different account. Sure, most email providers treat the account name as case-insensitive, and the use of '+' as a label/subaccount separator is a common convention, but neither of these is required. The RFCs say that the account name is case-sensitive (unlike the domain name) and '+' is just an ordinary part of the name; any special significance is assigned by the server. Ignoring '.' characters is something specific to Gmail. In the context of a search it's not unreasonable to ignore some of these differences, but at that point the matching names aren't "equal", just "similar". Certainly it should never be assumed that it's safe to send email to the transformed version of the address.

thaumasiotes 7y ago

Hard to find, but it's the kind of thing that will hopefully come up in code review. Consider this pseudocode with helpful pseudo-hungarian notation: username = request.post_params('username') evil_email = request.post_params('email_address') user = get_user_by_name(username) good_email = user.email_address if good_email != evil_email: # Hackers! else: reset_password(user.id, evil_email) # it's fine; it's the same as user.email_address You can see the problem in the call to reset_password. Sure, it's the same as user.email_address, but if you just used user.email_address you'd be doing the right thing and you wouldn't need the comment about "don't worry; it's fine". Then you start to wonder if that line shouldn't look more like reset_password(user) and from a security perspective, it should. (I assume the reason Github allows the code to specify the email address in the first place is that an account may be associated with multiple addresses.) If it makes no difference which email address you pick, why not pick the one that represents doing something safe instead of the one that represents doing something dangerous?

jazzabeanie 7y ago
If they do have multiple email addresses associated with one account, and they don't want to send a password reset to all of them, then you can see how it would happen. ``` if evil_email not in good_email_addresses: # Hackers! else: # just reset with provided email. If I thought there was a potential security issue I would have already addressed it. ``` So easy to be lazy at this point, especially under time pressure.
- eyegor 7y ago
  Off topic, you need the triple backticks on their own lines for them to render here.
  - codetrotter 7y ago
    Sorry but that is not correct either. Code blocks on HN are created by using two or more leading spaces on each line. Tripple backticks do nothing, and will be shown on the same line as the next line even if put on a separate line, because they are considered as regular text and part of one paragraph of text. See https://news.ycombinator.com/formatdoc https://news.ycombinator.com/formatdoc
mathewsanders 7y ago
I’d never heard of Hungarian notation, but searching for info I realize that I’ve seen this pattern so often but never knew it had a name :) TIL https://en.m.wikipedia.org/wiki/Hungarian_notation https://en.m.wikipedia.org/wiki/Hungarian_notation
LeifCarrotson 7y ago
The problem is that it requires a particularly detailed way of thinking about strings, much like the popular IEEE 754 floating point number question. If the code were instead written: username = request.post_params('username') email = request.post_params('email_address') user = get_user_by_name(username) if emails_are_equal(email, user.email_address) reset_password(user.id, email) Most people would not feel compelled to use user.email_address in the call to reset_password, because they're guaranteed to be equal by the previous line! The vulnerability was introduced when the author of emails_are_equal() wrote it with conversion to uppercase of the whole thing, and failed to realize that this conversion can collide with Unicode domains. They may also have done things like normalize/remove optional.dots+suffixes@gmail.com, "quoted" or (commented) local parts. There's a lot of magic - and a lot of risk - in an emails_are_equal method.
- thaumasiotes 7y ago
  > There's a lot of magic - and a lot of risk - in an emails_are_equal method. My whole point is that changing reset_password(user.id, email) to reset_password(user.id, user.email_address) neutralizes that risk. Then it doesn't matter whether emails_are_equal is risky or not, because a failure in emails_are_equal can only cause you to do a safe thing. What's less embarrassing -- "we accidentally sent a password reset for your account to you", or "we accidentally sent a password reset for your account to someone else"? So you might not feel compelled to rewrite reset_password(user.id, email) as reset_password(user.id, user.email_address) -- but you should.
- nybble41 7y ago
  > They may also have done things like normalize/remove optional.dots+suffixes@gmail.com, "quoted" or (commented) local parts. At least the first two (removing dots and suffixes), along with conversion to lowercase, are strictly invalid transformations which may result in an email address referring to a different account. Sure, most email providers treat the account name as case-insensitive, and the use of '+' as a label/subaccount separator is a common convention, but neither of these is required. The RFCs say that the account name is case-sensitive (unlike the domain name) and '+' is just an ordinary part of the name; any special significance is assigned by the server. Ignoring '.' characters is something specific to Gmail. In the context of a search it's not unreasonable to ignore some of these differences, but at that point the matching names aren't "equal", just "similar". Certainly it should never be assumed that it's safe to send email to the transformed version of the address.