#666 ✓resolved
Henry Baragar

User model "secure links" have low security

Reported by Henry Baragar | March 7th, 2010 @ 02:24 AM | in Hobo 1.0X

User accounts are fairly easy to compromise in may hobo applications. Worse yet, the administrator account might be the easiest account to compromise.

###First some background:

  1. Anybody with access to a hobo application can use the "forgot_password" for any user for whom he/she knows the email address.
  2. When the "forgot_password" form is submitted, the hobo application does three things:
    1. it updates the key_timestamp to the current time; and,
    2. it generates a "secure key" taking a SHA1 digest of a concatenation of the user's record id, the key_timestamp and the state (which probably would be "active")
    3. it emails the "secure key" in a specially constructed URL that grants access to the hobo application.
  3. Normally, the user would click on the link sent in the email and reset his/her password.

###The attack vector:

  1. All I need is the email address of an active user of the system. Even better if I know an administrator (and I bet I can guess them pretty easily for any hobo applications being administered by Barquin).
  2. I use the "forgot_password" form to cause the key_timestamp to be reset.
  3. Now I know:
    • the state (i.e. "active")
    • the key_timestamp to within 6 seconds (probably to within 2 seconds)
    • that record ids are generated sequentially and I probably can bet that the record id is less than 10,000 (are they any hobo applications out there with more than 10,000 users?). Furthermore, if I have what I think is a administrator's email address, it likely will be less than 100
    • how to create the special URL using the above information
  4. Now all I have to do is try out the 6 * 10,000 "secure links" I have just created.
    • At 1 attempt a second, I have compromised the account in less than 10 hours on average
    • If the application server's clock is accurate and I have an administrator's email address, one that is within the first 100 users, then I only need to check 200 links, which I can do in less than 4 minutes at one try per second.
    • If I know the email address of the first administrator to be created, the one with record id 1, I need check only 2-6 "secure links"

###The fix:

  1. I believe that all the information currently in the "secure key" is necessary, but clearly not sufficient
  2. To be sufficient, a (preferably random) piece of information that is not easily obtained by a user needs to be appended three values before the SHA1 digest is computed. The problem is that the application needs to remember this piece of information.
  3. Fortunately, Rails has such a piece of information that is used for protecting the session information. It is:
    ActionController::Base.session_options[:secret] See "nonce" and "secret_key" at http://api.rubyonrails.org/classes/ActionController/HttpAuthenticat...

Comments and changes to this ticket

  • Betelgeuse

    Betelgeuse March 7th, 2010 @ 05:41 AM

    The link should probably also expire at some point. Hobo already records a salt into the database for password purposes so it could use the same salt for this one.

  • Owen

    Owen March 8th, 2010 @ 12:36 PM

    • State changed from “new” to “investigating”
    • Assigned user set to “Tom Locke”
    • Milestone set to “Beyond Hobo 1.0”

    Passing to Tom for comments

  • Tom Locke

    Tom Locke March 8th, 2010 @ 01:11 PM

    • Assigned user cleared.

    Obviously we want this fixed ASAP. Henry - would you consider submitting a patch?

  • Tom Locke

    Tom Locke March 8th, 2010 @ 05:40 PM

    Thanks very much for this contribution Henry.

    I thought we had code in there already to prevent the same key from being used more than once? If not that's obviously a problem. I'm not too keen on the idea of having #valid_key? become side-effecting though. Is there somewhere else we can hook in to this? In the controller maybe?

  • Betelgeuse

    Betelgeuse March 8th, 2010 @ 06:36 PM

    Use cases for using the link more than once shouldn't be that uncommon (I have one at least) so there should remain a possibility to reuse it.

  • Tom Locke

    Tom Locke March 8th, 2010 @ 07:09 PM

    The side effect I meant was @key_was_used = true in #valid_key?

    Thinking about it, a change of state should always invalidate the key I believe. How about

    def clear_key
  record.write_attribute key_timestamp_field, nil
end

    And always call that in #become

    Want to give that a try?

    Betelgeuse - using the same key more than once sounds wrong to me. Can you give an example of when you need this

  • Owen

    Owen March 8th, 2010 @ 07:13 PM

    Thanks again, Henry for your willingness to dive in...

    -Owen

  • Betelgeuse

    Betelgeuse March 8th, 2010 @ 07:23 PM

    I use the key to identify a visitor coming in and they can use the link multiple times.

  • Tom Locke

    Tom Locke March 8th, 2010 @ 07:38 PM

    Hmm, In terms of what I originally intended I would have to call that a bug (that you can use the link twice like that), even though it was a feature for you. The idea was that the link would take you from one state to another, and so wouldn't be useful again once you'd entered the new state.

    I think we might have to sacrifice that bug/feature in the name of security.

  • Betelgeuse

    Betelgeuse March 8th, 2010 @ 09:09 PM

    Yeah but lifecycles allow the source and target state to be the same so I think it should be quite easy to support both with a configuration option.

  • Tom Locke

    Tom Locke March 9th, 2010 @ 09:53 AM

    Rather than adding to the API (with dont_clear_key) would it make sense for the rule to be "clear the key if and only if the state has changed"?

  • Bryan Larsen

    Bryan Larsen March 9th, 2010 @ 01:48 PM

    • State changed from “investigating” to “open”
    • Tag set to “defect, lifecycles, security”
    • Milestone changed from “Beyond Hobo 1.0” to “Hobo 1.1”

    I've stayed out of this conversation because it's been progressing fruitfully. The patch looks fine to me, except that you've inserted tabs instead of spaces. I don't mind fixing that up at my end, but if you fix that up, "git blame" will properly credit you.

    As far as documentation goes, the "right" way to do that is to fork tablatom/hobocookbook and edit manual/lifecycles.markdown. But I'd be so thankful to actually receive doc updates that I'd accept it in pretty much any form.

    Re: Tom's comment, I'm cool with any of the alternatives, including always changing the key without any mechanism to avoid doing so.

    BTW, good catch, Henry. Much appreciated.

  • Bryan Larsen

    Bryan Larsen March 9th, 2010 @ 01:48 PM

    reviewers: this new github feature is awesome: http://github.com/hbaragar/hobo/compare/44ded614...master

  • Owen

    Owen March 9th, 2010 @ 03:05 PM

    Thanks, much Henry!

    -Owen

  • Tom Locke

    Tom Locke March 9th, 2010 @ 03:43 PM

    Henry:

    > The state might not change for a reset_password: consider the { :active => :active } transition.

    Good point - ok that idea is out of the window then.

    I still think we can clean this up a bit though. How about:

    • call record.clear_key from Transition#change_state instead of from Lifecycle#become

    • Then we can not call it if the transition has :new_key => true

    • We can add a similar transition option :keep_key => true for multiple-use secure links

    Makes sense?

  • Tom Locke

    Tom Locke March 9th, 2010 @ 03:50 PM

    p.s. forgot to mention - the point being to get rid of dont_clear_key

  • Tom Locke

    Tom Locke March 11th, 2010 @ 09:30 AM

    OK I'll pull your changes in to a branch and have a go at finishing it off.

  • Owen

    Owen May 29th, 2010 @ 12:57 AM

    • Milestone changed from “Hobo 1.1” to “Hobo 1.0X”
    • Assigned user set to “Tom Locke”

    Quick win.

  • Bryan Larsen

    Bryan Larsen November 15th, 2010 @ 06:05 PM

    • Tag changed from “defect, lifecycles, security” to “defect, high, lifecycles, security”
    • Milestone order changed from “0” to “0”
  • Bryan Larsen

    Bryan Larsen November 20th, 2010 @ 08:13 PM

    • Milestone changed from “Hobo 1.0X” to “Hobo 1.3 (Rails 3)”
    • Assigned user changed from “Tom Locke” to “Domizio Demichelis”
    • Milestone order changed from “1” to “0”

    Domizio, this fix has been applied to 1-0-stable and master. Please change the milestone back to 1.0X and state to resolved once it has been applied to your branch. Thanks. The changes you want are 071aff4^..8336de94

  • Domizio Demichelis

    Domizio Demichelis November 22nd, 2010 @ 07:05 PM

    • State changed from “open” to “resolved”

    (from [132137bfcd9bd045ee411dc322e078a81939f194]) Improved the security of "secure links" (manually included changes 071aff4^..8336de94 from 1-0-stable) [#666 state:resolved] https://github.com/tablatom/hobo/commit/132137bfcd9bd045ee411dc322e...

  • Domizio Demichelis

    Domizio Demichelis November 22nd, 2010 @ 07:07 PM

    • Milestone changed from “Hobo 1.3 (Rails 3)” to “Hobo 1.0X”
    • Milestone order changed from “28” to “0”
  • Domizio Demichelis

    Domizio Demichelis November 27th, 2010 @ 09:40 PM

    (from [b181477dfb9a9655f1d44c3ea096260a40889fa9]) fix for private method :write_attribute call [#666] (credit: Tomoaki Hayasaka) https://github.com/tablatom/hobo/commit/b181477dfb9a9655f1d44c3ea09...

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile ยป

People watching this ticket

Tags

Referenced by

Pages