3

Many commentators (e.g. ZDNet) have suggested that the weakness in GitHub's case was that the model Homakov discovered was vulnerable had mass assignment enabled for its attributes.

However, I think the problem was not this, but was rather a failure to use a before_filter (or suchlike) in the controller to ensure that any given row in the table he updated could only be updated by an admin or by the user with the ID listed in that row. If such a filter had been in place in the controller, then the table would have been secured from attack even if the model's attributes were mass assignable.

Am I correct?

  • That would work too, but requires designing specific filters that could have leaks. Mass-assignment is really simple and one line solves this basis problem. – Carson Cole Jun 09 '12 at 19:56
  • 1
    Mass assignment is very handy, though. Isn't it more convenient to have mass assignment enabled and some filters in the controller, than to have mass assignment disabled and a whole bunch of workarounds in the controller? –  Jun 09 '12 at 20:17

2 Answers2

3

Yes, I think you're right.

In such situations, you can also use authorization gem, for example: cancan, declarative authorization, heimdallr...

The problem is not that you can use. The problem is how to help do not forget to use it in certain situations. For example cancan have the following method: check_authorization It helps to check authorization for all actions.

About that says Homyakov. Add the protected attribute would be the easiest way. Maybe vulnerability has been discovered just because solution to restrict the authorization was different. In some situations, adding of protected attributes leads to a loss of flexibility in your api.

Mik
  • 4,117
  • 1
  • 25
  • 32
3

As far as I understand this issue has nothing to do with before_filters checking for admins. An attribute existed, in the exemple you cite the user_id on the public_keys model, that was not supposed to be exposed to mass assignment.

The before_filter can work as another "layer" of security as .slice can be also used to ensure that you only get the attributes you supposed to get can also work.

On a last note: I think admin != god, meaning, I find no use to a Github admin having rights to perform the action Homakov was able to perform exploiting this issue.

jvalente
  • 570
  • 7
  • 13