[PATCH conductor] rm3343: credential: make attributes accessible/protected

Mo Morsi mmorsi at redhat.com
Tue Jun 5 07:46:37 UTC 2012


On 06/04/2012 12:21 PM, Maros Zatko wrote:
> On 06/01/2012 09:23 PM, Matt Wagner wrote:
>> On Fri, Jun 01, 2012 at 04:54:43PM +0200, mzatko at redhat.com wrote:
>>> From: Maros Zatko<mzatko at redhat.com>
>>>
>>> ---
>>>   src/app/models/credential.rb |    3 +++
>>>   1 file changed, 3 insertions(+)
>>>
>>> diff --git a/src/app/models/credential.rb
>>> b/src/app/models/credential.rb
>>> index 4b06a36..a52319c 100644
>>> --- a/src/app/models/credential.rb
>>> +++ b/src/app/models/credential.rb
>>> @@ -34,4 +34,7 @@ class Credential<  ActiveRecord::Base
>>>     validates_presence_of :credential_definition_id
>>>     validates_presence_of :value
>>>     validates_uniqueness_of :credential_definition_id, :scope => 
>>> :provider_account_id
>>> +
>>> +  attr_protected :id, :provider_account_id, :created_at
>>> +  attr_accessible :value, :credential_definition_id
>> This is not a review, but I'm just curious -- how are we deciding what
>> attributes get protected? It's not quite intuitive to me why we're
>> protected :created_at (but not :updated_at), but allowing
>> :credential_definition_id to be mass-assigned. If I were an attacker it
>> seems like there might be an advantage to seeing if I could remap
>> credential_definition_id to point to something else, much moreso than
>> making it look like a credential was created at a different time than it
>> really was.
>>
>> Again, this isn't a patch review; I'm just curious if there's some
>> additional logic I'm missing here.
>>
>> -- Matt
>>
>>>   end
>>> -- 
>>> 1.7.10.2
> Actually, this interests me as well since it has been done by Mo, so
> it's question for him.
> Mo, could you please clarify?
> Thanks
> -maros

Yes, I favor a common sense approach here that leans on the side of
safety. eg while not relying on a 'blacklist everything and whitelist
particular attributes' strategy, we blacklist as much as we can and
assign those attributes explicitly when needed.

Particularily, all id's and timestamps should be blacklisted unless
there is a particular reason not too, as well as all fields which are
used to dictate the flow control of various other code in the
application (such as permission bits) and/or that which should be set to
one of a fixed list of values. All in all, this part of the security
work involves going through and determining if each field should be
allowed to be assigned indiscriminately (using the model analysis on the
wiki [1]  as a central point to track all of this)

I suspect these changes will entail some temporary breakage, as we rely
on various mass assignments in the controller level. We need to track
these down and resolve them as we go along, we can't just ignore these
security flaws for convenience purposes in the controller layer.

  -Mo

[1]
https://www.aeolusproject.org/redmine/projects/aeolus/wiki/Hardening_Conductor_models



More information about the aeolus-devel mailing list