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

Rails · xds2000 · Created at · Last by JeskTop Replied at · 409 hits
202

如果你去了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 条回复
202
xds2000 · #1 ·

@dhh 老大发话了,请大家记录之:
Here's the basic pattern we've been using at 37signals to guard against mass-assignment bugs:
https://gist.github.com/1975644

class PostsController < ActionController::Base
  def create
    Post.create(post_params)
  end

  def update
    Post.find(params[:id]).update_attributes!(post_params)
  end

  private
    def post_params
      params[:post].slice(:title, :content)
    end
end
78
ashchan · #2 ·

Learn it, fix it. Go forward.

178
_kaichen · #3 ·

对,这是Github里写这段代码的人代码写差了,这里用attr_accessible也能起到该有的作用。这和Rails没有半毛钱关系,用任何的工具都能写出这种危险的代码。

之前Github还有个信用卡信息没有校验的Bug,可以绕过信用卡信息随意更改Plan。

2
huacnlee · #4 ·

原来是 attr_accessible, attr_protected 的关系
http://huacnlee.com/blog/rails-attr-protected-or-attr-accesible-fields/

2
huacnlee · #5 ·

#3楼 @_kaichen 其实在别的框架里面他们这个风险比较低,因为都是这样的

public string list(){
  Post post = new Post();
  post.setTitle(params[:title]);
  post.setBody(params[:title]);
  post.update();
}

所有字段更新是原始的手动赋值。

101
daqing · #6 ·

忘记写rspec测试了吧。敏感字段要测试的。

96
iwinux · #7 ·

#5楼 @huacnlee 我在 Django 里模仿 Rails 写了个 mass-assignment 的东西,现在有点担心会不会有漏洞 = =

class BaseModel(db.Model):
    protected_fields = []

    def to_dict(self):
        keys = self.fields().keys()
        _dict = {}

        for key in keys:
            _dict[key] = getattr(self, key)

        return _dict

    def from_dict(self, _dict):
        for field in self.protected_fields:
            if field in _dict:
                del _dict[field]

        for key, value in _dict.iteritems():
            setattr(self, key, value)
        self.put()
96
vkill · #8 ·

dhh 说的这种方法在 3.1 之前基本就这么搞,不过 3.1 后我更喜欢 mass assignment

1
rei · #9 ·

#7楼 @iwinux 用白名单而不要用黑名单

96
hhuai · #10 ·

update_attributes,我刚开始学rails的时候就想过这个问题,应该会引起安全问题,尤其是对于有password的user表,不过看了一下,发现rails本身已经有解决办法,attr_protected。

从rails的guide有一章安全来看,rails本身对安全是很重视的,至少比其他的web框架考虑得多,包括xss,csrf。在django中我是没看过安全这章。

这里扯上rails,说是其bug真的太扯了,是github用得不对。

96
23c · #11 ·

编码习惯问题,客户端信息是不可信的。

96
hhuai · #12 ·

#7楼 @iwinux 你写得挺好的,protected_fields都考虑到了。

96
zipme · #13 ·

我觉得用 attr_accessible 会比较好些,不然经常会忘记,比如这里 updated_at 和 created_at 就会被mass assigned.. 虽然是不会照成什么很严重的后果。。

8
hooopo · #14 ·

我觉得他是因为这个issue( https://github.com/rails/rails/issues/5228 )没被rails采纳。。才试图用hack github的方式让开发者重视,或是让rails 采纳他的建议....

我觉得fxn说的很有道理,这是开发者的问题,而不是rails的问题,事实上rails已经提供了很好的解决办法,开发者没有意识到是自己的问题。

昨天还去ruby china看了一下,,发现也有类似问题:https://github.com/huacnlee/ruby-china/blob/master/app/controllers/topics_controller.rb#L94

96
zipme · #15 ·

貌似如果 mass assign Topic 的 suggested_at 的话可以达到右边栏置顶贴的效果?

2
huacnlee · #16 ·

#14楼 @hooopo 这行是没有的, 要看后面句,会被强制设置成当前用户的编号

@topic.user_id = current_user.id
8
hooopo · #17 ·

#16楼 @huacnlee No
可以hack的字段很多,包括updated_at created_at 还有各种标记字段

2
huacnlee · #18 ·

#17楼 @hooopo 我知道,赶紧补上

1
rei · #19 ·

#15楼 @zipme
#17楼 @hooopo

确实有漏洞,现在补上了。大家一定要用 attr_accessible 阿。

16
ywencn · #20 ·

#19楼 @Rei 好吧,我也要从attr_protected党转到attr_accessible党了

96
iwinux · #21 ·

貌似我用了 Rails 那么久一直都是用 attr_accessible,没有用过 attr_protected(我是看 Ruby on Rails Tutorial 这本书入门的,对我的编码风格影响很大)

22
bony · #22 ·

一直用attr_accessible的表示毫无压力。

96
hisea · #23 ·

我觉得homakov的初衷是让rails团队提供一个安全的默认方案。目前的默认方案是不安全的。

一楼dhh自己的解决方案也从侧面反映出当前attr_accessible的方案并不是最优。

96
vkill · #24 ·

@hisea
你说错了,哈哈,attr_accessible + mass_assignment 试试看?

188
fredwu · #25 ·

Yehuda的proposal在这里:https://gist.github.com/1974187

我个人非常赞同safe guarding应该是controller的责任,而不是model的责任。

96
hisea · #26 ·

#24楼 @vkill
我没有说attr_accessible不能解决问题,我是说不是最佳方案,连dhh自己都要另外搞个hack。

另外homakov的意思是,rails默认是没有保护的,这个默认是危险的,为什么不能有一个更安全的默认呢。

196
xdite · #27 ·

37signals 那樣作是有原因的。

  1. 把責任移到 controller
  2. attr_accessible 不是最好的方案。很容易搞死人,而且有時候沒有解決到問題...
8
hooopo · #29 ·

#26楼 @hisea
dhh的这个方案很不DRY呀
I think role based mass assignment protection is enough.

96
hisea · #30 ·

#29楼 @hooopo 所以我把他这个方案叫hack。

也很难简单解决accept_nested_attributes_for.

如果用复杂的办法解决了,我个人不是很喜欢这样的方法存在于controller里面的private区,不能很好的添加测试覆盖。

718
ch3n · #31 ·

dhh的方案不够好,还是把过滤部分写进model更好。

1
rei · #32 ·

我觉得白名单写进 Model 好,不然控制器还得知道 Model 的属性。用了 attr_accessible 之后,控制器知道 role 就行了,相当于打包属性组。

775
nouse · #33 ·

我也觉得DHH还是安安心心地去开赛车的好

558
camel · #34 ·

@homakov竟然是93年的,真是后生可畏啊。
Salut, I am Egor Homakov.
born on 28 apr. 1993 in Russia.

#23楼 @hisea dhh说他喜欢这种'hack'的原因是因为The great thing about post_params is that you can make it dependent on controller state as well
其实我觉得attr_accessor+as完全可以实现相同效果,而且还很dry。
#32楼 @Rei 全部白名单和不写都一样,只是能做个提醒,还真得怪github的程序员偷懒。

188
fredwu · #35 ·

说写进model里好的都应该去复习MVC的基础知识。。。-_-#

8
hooopo · #36 ·

#35楼 @fredwu

Role based mass assignment protection:

class Post < ActiveRecord::Base
  attr_accessible :title, :body
  attr_accessible :title, :body, :user_id, :rank, :as => :admin
end
Post.update_attributes(params[:post], :as => :admin)

哪个角色可以操作model的哪些属性难道不是业务逻辑?写到controller里我还真理解不了:-)

188
fredwu · #37 ·

#36楼 @hooopo 因为操作model是controller的责任啊……

96
anklos · #38 ·

#35楼 @hooopo 这里的arre_accessible不是用来表示model之间的关系,而是post安全机制上的保护。在controller这一层更make sense.

8
hooopo · #39 ·

#37楼 @fredwu
#38楼 @anklos
我认为这个写在哪里都讲的通的。和权限验证比较类似。贯穿整个mvc。yahuda的例子里面,view也是需要的,只不过一般时候起不到作用。

96
pokkalee · #40 ·

我觉得arre_accessible无需想到什么谁的责任什么的问题,arre_accessible正是对对象属性的一种描述,controller的责任不是操作model或者谁,而是接收业务请求跟着role走

718
ch3n · #42 ·

@fredwu
验证数据作为业务逻辑的一部分,当然是model的事情.
而且,也许不止PostsController一个地方会更新Post,写入model,其他地方也一样调用.不然,若是某天允许修改Post的其他属性,要改无数个地方.

96
hisea · #43 ·

不同的controller可能需要不同的attr_accessible, 我想这也是@fredwu说操作model是controller的责任。

如果只给最少的权限,controller中将会有大量的.

@post.att1=123
@post.att2=234
...
....

586
edokeh · #44 ·

又是一个典型的易用性和安全性的冲突问题,说实话,虽然这个不能称之为rails的bug,但是rails在这上面的处理确实不妥,导致不少“中低”水平的开发者做出的应用暗藏隐患
另外参考rails3对于xss的处理方式,我觉得rails确实需要在mass assign上做一些工作,safe by default对框架来说还是很重要的

1
rei · #45 ·

#43楼 @hisea attr_accessible 有 role

8
hooopo · #46 ·

#45楼 @Rei 我觉得如果是rails2.3,dhh的方法还不错~
加accessable以后后台管理不方便了

188
fredwu · #47 ·

把AngelNest的mass assignment漏洞给修补了,嘿嘿:https://github.com/fredwu/angel_nest/commit/ab636d677295cca9c42e525bc737dc6978e1a663

96
anklos · #48 ·

#47楼@fredwu 话说你还是直接在model里解决了阿o。0

188
fredwu · #49 ·

#48楼 @anklos 因为AngelNest我不作大的更新了。新的项目的话我正琢磨着是不是自己写个gem来解决。。。

96
hisea · #50 ·

#45楼 @Rei 你是说:as么?这个并不是很灵活啊,比如一个系统有10个以上的role的时?
而且很多ACL系统是动态管理的,系统管理人员可以添加group/role/permission.

718
ch3n · #51 ·

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

1
rei · #52 ·

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

686
jhjguxin · #53 ·

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

1
rei · #54 ·

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

类似这种情况?

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

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

2
huacnlee · #56 ·

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

188
fredwu · #57 ·

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

4

文档里( 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...

1
rei · #59 ·

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

2
huacnlee · #60 ·

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

64

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

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

96
hisea · #62 ·

#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
hooopo · #63 ·

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

4

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

96
hisea · #65 ·

#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里面就怪怪的了。

96
vkill · #66 ·

#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
96
Guest · #67 ·

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

96
narkoz · #68 ·

#36楼 @hooopo DRY

attr_accessible :title, :body, :as => [:default, :admin]
attr_accessible :user_id, :rank, :as => :admin
1
rei · #69 ·

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

1
rei · #71 ·

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

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

188
fredwu · #72 ·

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

1164
jesktop · #73 ·

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

需要 Sign In 后方可回复, 如果你还没有账号请点击这里 Sign Up