5 ms·

For folks who wanna see what led to this exploit in a Rails codebase, here’s the commit where it’s fixed: https://gitlab.com/gitlab-org/gitlab/-/commit/c571840

• sethammons 3y ago
  as a non-rubiest, can you point to the error?
  • Rapzid 3y ago
    Ruby. I kid, but also I don't. Initially a single email could be passed into the API/form call and they would look it up. If found they would send a recovery to that email but it was the email the user supplied not what was in the DB. Oh, no problem we looked it up so they are the same! But then the ability to look up accounts from a list of emails was added. If any email matches the account lookup would succeed. Then they sent the reset link to that same user supplied value but OH NOEHS IT'S AN ARRAY NOW AND SOME MIGHT NOT HAVE MATCHED ACCOUNT EMAILS! So they ended up sending out reset links to a tainted list of emails. Rails "concerns" are the worst IMHO anyway, but looks like they aren't using strong params here either which is even worse. Also someone thought it was more elegant to reuse the tainted value which is par for the RoR course.
  • szundi 3y ago
    Omg is this fix had to be this bloated?
  • thedanbob 3y ago
    This is actually a follow-up refactor, the fix is here: https://gitlab.com/gitlab-org/gitlab/-/commit/abe79e4ec437988cf16534a9dbba81b98a2e7f18 https://gitlab.com/gitlab-org/gitlab/-/commit/abe79e4ec43798...
• taspeotis 3y ago
  # Concern that overrides the Devise methods # to send reset password instructions to any verified user email module RecoverableByAnyEmail So it was a feature?? Anyway, in the fixed version it's still called RecoverableByAnyEmail. Do people not read the code around what they are changing??
  • Rapzid 3y ago
    It does say any email doesn't it? Not verified, any.
  • BandButcher 3y ago
    haha the first thing i would've caught in the initial PR was the file name... and the default setting of `confirmed: true`... seems like a big oversight or possibly an inside job (if im being conspiratorial)
  • zx8080 3y ago
    > "RecoverableByAnyEmail" Added 8 months ago [1]. And then one month later: > "password_reset_any_verified_email" Was removed. 7 months ago [2], *note* __verified__ word here. No blaming or conspiracy intended in this post, just listing links to relevant commits. 1 - https://gitlab.com/gitlab-org/gitlab/-/commit/94069d38c9cd63202cc21e7febfaaad596052c0d https://gitlab.com/gitlab-org/gitlab/-/commit/94069d38c9cd63... 2 - https://gitlab.com/gitlab-org/gitlab/-/commit/a935d28f3decf8f6873d5b0a6f405f2687d0f99f https://gitlab.com/gitlab-org/gitlab/-/commit/a935d28f3decf8...
• thedanbob 3y ago
  This doesn't look like the actual fix but rather a follow-up refactor. I believe the fix is here: https://gitlab.com/gitlab-org/gitlab/-/commit/abe79e4ec437988cf16534a9dbba81b98a2e7f18 https://gitlab.com/gitlab-org/gitlab/-/commit/abe79e4ec43798... - recoverable.send_reset_password_instructions(to: email) if recoverable&.persisted? + recoverable.send_reset_password_instructions if recoverable&.persisted?
  • alexpls 3y ago
    Oh yeah, good pickup thanks!
  • progval 3y ago
    and making send_reset_password_instructions get the email addresses itself from the "recoverable" object.
  • 1oooqooq 3y ago
    on GitHub, the fix would be adding a regex to ensure there was no list on the user supplied email.