hyper-mesh icon indicating copy to clipboard operation
hyper-mesh copied to clipboard

Security hole - it might be possible to get a list of all id's for a model

Open catmando opened this issue 8 years ago • 7 comments

this https://github.com/ruby-hyperloop/hyper-mesh/blob/ebf661eecc592799b7723bb666c1d42d84551918/lib/reactive_record/server_data_cache.rb#L182

reads every id, but is not protected by the check_permission_with_acting_user method

it should read:

collection = cache_item.value.collect do |record| 
  record.check_permission_with_acting_user(@acting_user, :view_permitted?, :id)
  record.id
end
cache_item.build_new_cache_item(collection, method, method)

Similar to https://github.com/ruby-hyperloop/hyper-mesh/issues/43 .

Hopefully adding that line will not kill performance!

catmando avatar Dec 13 '17 21:12 catmando

Be aware however that any record that has ANY attribute accessible to a channel will automatically be grant access of the id to that channel. This is to allow push synchronization. So for example if my User instance channel has access to the user's name (for example) then each user will also have access to the :id of each record that belongs to them.

I don't think this is an issue FYI... but if it is then we would have to implement an optional encryption of the id before transmission (and decryption after reception.) I would make optional so that if you are not worried about it you don't have to pay for the encrypt/decrypt overhead. Also would probably be handy during development not to encyrpt/decrypt the ids.

catmando avatar Dec 13 '17 22:12 catmando

FYI aggregations might have a similar problem, however i think we should just Make a caveat, as those things are so hard to deal with. I would rather we just had a config switch that turned them off.

catmando avatar Dec 13 '17 22:12 catmando

Thanks for generously assigning this to me. I will immediately put it, in bold, to make sure of its importance, on my hyperloop todo list. To fix it ASAP, i put it at the end of the list, so it will be the first item after all the other items on my list.

janbiedermann avatar Dec 14 '17 09:12 janbiedermann

:-)

catmando avatar Dec 14 '17 13:12 catmando

I'll try to get done as well

catmando avatar Dec 14 '17 13:12 catmando

@janbiedermann sorry abt assigning problem. Fix should be just as shown, but I am concerned about perf impact. You r kind of master if that. If you check it out perf wise I will do test cases etc.

catmando avatar Dec 14 '17 13:12 catmando

in progress. So far:

lib/active_record_base.rb:

  • added def hyper_model_scopes
  • added def server_methods

lib/reactive_record/permissions.rb

  • moved Hyperloop::InternalPolicy to lib/hyperloop/internal_policy.rb
  • moved ReactiveRecord::AccessViolation to lib/reactive_record/access_violation.rb

(hyper-mesh.rb modified accordingly)

lib/hyperloop/internal_policy.rb added:

  • def self.guard_root_fetch_method!(model, method) => true|ReactiveRecord::AccessViolation
  • def self.guard_record_fetch_method!(record, method) => true|ReactiveRecord::AccessViolation
  • def self.guard_relation_fetch_method!(relation, method) => true|ReactiveRecord::AccessViolation
  • def self.raise_access_violation

added in lib/reactive_record/proxy

  • attribute_node.rb class AttributeNode a = ReactiveRecord::Proxy::AttributeNode.new(record, attribute, value) a.value(Rails) param: must pass Rails object to ensure local call => value|ReactiveRecord::AccessViolation
  • record_node.rb r = ReactiveRecord::Proxy::RecordNode.new(record, acting_user)
  • relation_node.rb r = ReactiveRecord::Proxy::RelationNode.new(relation, acting_user)
  • root_node.rb r = ReactiveRecord::Proxy::RootNode.new(model_name, acting_user)

Proxy Interface may change slightly with further progress, maybe.

added lib/reactive_record/graph.rb class ReactiveRecord::Graph < BasicObject class Node < BasicObject internal to Graph interface still in flux

Ill update this to keep track.

janbiedermann avatar Dec 29 '17 13:12 janbiedermann