58bf89ba2dfa037971b05d1afb0480a3

一个小Bug引发github安全问题泄露。Shit!

Railsxds2000 • 于发布 • 最后由 JeskTop回复 • 4880次阅读

如果你去了v2ex,一定会看到这个标题: Rails 爆出严重安全漏洞(http://www.v2ex.com/t/28768#reply2), 别激动,v2ex对ruby来说,它是外行社区,看了帖子内容我就意识到都是在看热闹,没说道点子上。

Egor Homakov,这个哥们是一个Rails开发者。报出的问题是mass assignment没保护的问题。这个问题是什么:看这个:
Hackers Love Mass Assignment
#26 Hackers Love Mass Assignment(http://railscasts.com/episodes/26-hackers-love-mass-assignment

github发现后,把这个Homakov的账号给ban住了。这种反映让人不爽,(
http://chrisacky.posterous.com/github-you-have-let-us-all-down
),在危机公关上是个败笔。

最后,好消息,@wycats已经提议强制保护(https://gist.github.com/1974187

后又写了他对attr_accessible (and an analogy to rails_xss
)的安全观点(http://news.ycombinator.com/item?id=3664334
),很快已经有人就把代码写到rails-master分支了(https://github.com/rails/rails/commit/641a4f62405cc2765424320932902ed8076b5d38

共收到 73 条回复
718

@hooopo
其实。。。就算是rails2.3,这种逻辑也应该写在model里面 毕竟还有别的方法 XD

5aec84cd0b5479a0d1d89b6ffa2a9a20

#50楼 @hisea 实现各种逻辑,写在 Controller 和 Model 复杂度都是一样的。我只是赞同写在 model 里而已。

325f379acbce12eb76e0a42a97679127

膜拜了 走夜路要注意安全啊 我就是 只用 attr_private 的人 要改啊

5aec84cd0b5479a0d1d89b6ffa2a9a20

#50楼 @hisea 想了一下,是指 permission 可以指定 model attr accessible 的情形?

类似这种情况?

topic.update_attributes params[:topic].slice(current_user.role.permission[:topic])

那确实写在 controller 和 model 分别不大了。不过我觉得实际应用权限也不需要太灵活,公司网站(一个论坛)实现一个分门别类的权限控制,实际上开发、运营和审核也就几个人而已,用上的权限也就分3、4个够了。

2

其实目前 attr_accessible 这种方式最大的问题是容易被忘掉。哪怕你再牛逼。程序员是“懒惰”的,或者偶尔失误的。
一有这种状况,这种漏洞必然会出现。

2735068c913a072744a799e3c0833b7b

#56楼 @huacnlee 所以要用 config.active_record.whitelist_attributes = true 啊。

85a64fb3375926c0be5c75eccf40e853

文档里( attr_accessible,attr_protected)豁然写着一句

“Note that using Hash#except or Hash#slice in place of attr_accessible to sanitize attributes won’t provide sufficient protection.”

我可以肯定这句文档不是DHH写的....甚至没有看到过,不然他肯定会抹掉.... lol...

5aec84cd0b5479a0d1d89b6ffa2a9a20

#56楼 @huacnlee 强制白名单之后,不设置就写不进,程序员就不会忘了。

2

#57楼 @fredwu
#59楼 @Rei 额,我才知道有这个...

946dbc66d19ba815ffec20d05f138f73

对的, 需要根据不同的controller设置不同的限制. 内部也有无数需要操作model的地方, 限制写在model里面不妥.

针对这个问题改了一上午的代码, 下次写rails的时候就注意到这个问题了.

Fa45f66688b148b999faadb759bcf2dc

#52楼 @Rei 是的,我同意你这个观点,而且复杂的逻辑在Model里面更容易测试。这也是我不喜欢dhh的controller私有方法的原因。

同时,我也同意@fredwu 跟yahuda的观点,我论是role也好,还是current_user的判断也好,authentication和authrization是controller的责任。如果我们不在model里面判断用户是否登陆的话,为什么吧不同role可以更新哪些field的声明牵到model中来呢。

可能很多人会觉得登录和mass assignment不能比较,因为登录是个相对简单很多的问题。

我个人觉得简单是因为登陆已经成为成熟的模式,更重要的是登录有很好的工具完成这一切。而mass assignment处于混沌状态,需要人一斧子劈开混沌找一个成熟的模式,并开发一个易用的工具。我想Yahuda发一个Proposal而不是直接写代码添加到rails里面,也说明rails团队同样也是在摸索,希望得到开发人员的反馈。

我只是觉得现有的方式无论是attr_accessible或者是controller hack都不是最佳方案,虽然我暂时也没想出什么好办法,毕竟mass assignment其实涉及到的很多rails内部机制,是rails提高开发速度的magic之一,限制多了魔法受限,限制少了走火入魔。

8

#62楼 @hisea validation为什么不放在controller呢
current_user和session紧密相关,想放到model里也难

85a64fb3375926c0be5c75eccf40e853

我提几个我认为关键的问题:
1. mass assignment这个问题上,model是否应该信任controller。
2. dhh的做法和model里验证的做法是否冲突?
可不可以把 dhh的做法理解为,filter掉 params里不需要的值。
如果这么理解,dhh的做法是无可厚非的。
3. 把user,role等和当前session相关的东西传给model,是不是一种变相的让model知道当前的状态?

Fa45f66688b148b999faadb759bcf2dc

#63楼 @hooopo validation是validate model自己内部的状态。不取决于外部的条件。
不取决于用户角色,比如post title不能为空,无论谁登录都不能为空,无论是web登录还是api发布都不能为空,即便是console,rake task, seed import,没有controller,没有session保存依然不能为空。这个条件是不为外部条件所控制的。这种自足(self contained)的模式有利于model不依赖外界条件的情况下证明自己的合理性。同时降低了耦合,哪天我们的世界没有了web,没有浏览器,没有了session和controller,这个model依然可一作为一个独立的个体,在不改变代码的情况下继续工作。

validation可以有判断,但这个判断也应该是内部的,比如post的title在slug为空的情况下不能为空,slug同样是post内部的状态。

反过来说,很少有人validation里面是根据其他不相关的外部因素来判断的,例如很少validation牵扯到role或者current_user。为甚么不能在validation中加入current_user的判断呢,看传进来的一个flag,确定是不是登录用户,或者是不是admin,admin就能save,不然validation error就是,"你保存权限不够",这样不也说得过去么。

换一个稍微远一点例子易说明,就是callback跟observer.

我个人的原则就是callback用与自身内部的事务,observer牵扯外部的事务。

比如,如果slug为空,保存之前将title复制到slug,都是内部的事情,用before_save很不错, 用observer有点多此一举。

反过来,如果保存之后需要发一封邮件,访问一个第三方api,然后更新服务器硬盘上的一个文件,这些东西放在after_save里面就怪怪的了。

6a3b78e05f20472aafc29aad3c4a6b4d

#64楼 @poshboytl
个人认为
1、mass assignment 必须信任 controller
2、dhh的做法对于3.1+是多此一举,mass assignment 是3.1加入的功能,3.1 以前确实按dhh那样干过
3、给model只需要传 user_id 就好,不需要session啊。

另外对于 user_id 我的做法是,加个 set_current_user before_filter ,这样request 过来的 user_id 都会被覆盖。model里不对user_id 做权限方面的验证,只管保存就好,是不是admin都在 controller 里判断

  private
    def set_current_user(resource_name=nil, attribute_name="user_id")
      return unless current_user.respond_to?(:id)
      resource_name ||= controller_name.singularize
      params[resource_name] ||= {}
      params[resource_name][attribute_name] = current_user.id
    end
5f694daa215b221099415de42d08f811

因为这个问题提交的补丁,导致论坛当前无法注册新帐号,请@huacnlee 看看。

144314100b686db946ff68c7ae1065d1

#36楼 @hooopo DRY

attr_accessible :title, :body, :as => [:default, :admin]
attr_accessible :user_id, :rank, :as => :admin
5aec84cd0b5479a0d1d89b6ffa2a9a20

#68楼 @narkoz 哇还能这样写,我还想象把基础属性做个数组呢

5aec84cd0b5479a0d1d89b6ffa2a9a20

Strong parameters: Dealing with mass assignment in the controller instead of the model

http://weblog.rubyonrails.org/2012/3/21/strong-parameters/

2735068c913a072744a799e3c0833b7b

那时候我在Twitter上说这个应该gem化,dhh还说直接用Array.slice!就好了。结果还是gem化了。。。嘿嘿嘿~

925b33675ad19949d46f74c66fab1b10

自学半年的新手,看不懂你们说什么。正常吗?

需要 登录 后回复方可回复, 如果你还没有账号你可以 注册 一个帐号。