Security issue with policies not applying to counts

[sfcgeorge]

A front end query like Customer.all.count will always succeed regardless of policies. I guess this is because policies apply to instances of models that have an id to check, but a count just returns a number so how can it check who's allowed to see a number?

This is an issue because competitors can get statistics out of your company that they shouldn't have access to (like how many customers do you have? How many transactions have you handled? etc).

It applies to scopes too. While you wouldn't be able to see any of the individual records returned by a scope, you can still call count on it and this metadata could still be damaging, e.g. Product.returned_faulty.count.

It appears average(col) isn't implemented in Hyperloop so I think it's just count that's an issue.

[catmando]

Two things about scopes (and relationships) verses normal attributes.

1. Scopes themselves do not present any data, only their aggregates (like count, avg, etc), so what we are protecting is visibility to the aggregates.
2. All aggregates except count take a column name, so the policy regarding that column name applies to the aggregate. For example if you can read the :age column then you can always compute the average age on the client. Therefore the only policy we really need to understand is around count.

Example

We have an Order model, a User model, and an Admin model.

The Order model has a scope for_vips that returns only orders for vip customers.

Typical count policies might be:

The application (i.e. anybody) can see the count of Orders Only Admin's can use the for_vips scope Admin's can see the count of the User model. User's can see the count of their Orders

  regulate_broadcast(Order) do |policy|
    policy.send_count(:all).to(Application)
    policy.send_count(:for_vips).to(Admin)
  end

  regulate_broadcast(User) do |policy|
    policy.send_count.to(Admin) # :all is assumed
    policy.send_count(:orders).to(Admin, self)
  end

Specifically, #send_count takes a list of method names which are either scopes, has_many or has_many_through relationships. The built in scope :all can also be used, and if no scopes are listed, :all will be assumed.

Note: just like attributes, the policies are defined in terms of broadcasting, but also apply to read access. Thus if you can't broadcast the count, you can't read it either.

[janbiedermann]

Just to clarify: MyModel.super_scope

the vector will look like: [MyModel, [super_scope], *all]

Result will be an aggregation. No column names are called, is the policy applied, which policy is applied?

[janbiedermann]

hit the wrong button

[janbiedermann]

Not sure if i am right with the vector: MyModel.super_scope will it be: [MyModel, [super_scope], *] ? and deliver no data? In that case SDC returns representative, which is the aggregation i suppose.

and MyModel.super_scope.all will it be: [MyModel, [super_scope], *all] ? and will deliver data, calling all on the aggregation?

Now where in SDC is the policy applied to * or *all?

[janbiedermann]

same applies to server_methods.

[catmando]

The only time these policies are applied is when the request is only for the count without any attributes.

Mymodel.all.count # will check ... My model.all.each { |x| x.id } #no need to check

[catmando]

Key to understand what is all vectors return data.

Mymodel.all does nothing

[janbiedermann]

If you accept ipc from the internets its good practice, for security, to guard ALL calls, because you never know, complex systems, etc… So there should be a default policy "deny" and then relax from that. Of course, its a lot of work to relax each possible call, so grouping things, like you suggested is very nice and saves work and probably is safe enough. Of course, in components you would probably not do something like MyModel.super_scope, because the outcome would be useless, you would always do something like MyModel.super_scope.first.name. But as an evil attacker i would try every possible option, looking at the open source code, because then i just want the data, i dont care what it looks like. You say all does nothing, when i look in the code, and maybe i don't understand something here, but i see this:

             elsif method == "*all"
                  cache_item.build_new_cache_item(cache_item.value.collect { |record| record.id }, method, method)

That looks to me like returning an array of ids of valid records. And i am not so much considering here, what the client, when using the RR interface, would do, but i am thinking, what vectors i can create as an evil attacker using my hands in the js console.

[sfcgeorge]

Not sure I'm following 100% but being able to get an array of IDs is basically the same as getting a count. I think I've seen Hyperloop return a bunch of IDs in some situations when I don't think it should / needs to.

So this fix should limit all any other scope, for both count and map IDs.

[catmando]

@janbiedermann is correct that the design must assume that the API is public and will be attacked. Correct use cannot be assumed.

1. default policy is to deny access but the generator overrides this but only for dev and test (not production)
2. Re: record.id I think you are right, and that is a security hole. I added https://github.com/ruby-hyperloop/hyper-mesh/issues/45 to cover this, and because no good deed goes unpunished, I assigned it to you :-)

[janbiedermann]

in progress, see issue #45

[catmando]

okay this is easier (I hope) than I thought. System never actually broadcasts count (i.e. client has to ask for it) So the semantics can be based on acting_user rather than a channel, which is much easier!

I am not quite sure what policy syntax will work best, but for now I will implement a lower level method called count_permitted?(acting_user, method) at the class level, and count_permitted?(method) at the instance level. By default these methods will return true for all models all the time.

You can override them in ApplicationRecord, and/or whatever models you want to control access.

For example:

class ApplicationRecord < ActiveRecord::Base
  def self.count_permitted?(acting_user, method) # note its a class method for scopes
    false
  end
  def count_permitted?(method) # and its an instance method for relationships
    # like the other low level permission methods the acting_user is defined on the instance already
    false
  end
end

class Order < ApplicationRecord
  def self.count_permitted?(acting_user, method)
    case method
    when :all
      true # everybody can see the total count of orders placed
    when :for_vips
      acting_user.admin?  # only admins can see the count of orders for vips
    end
  end
end

class User < ApplicationRecord
    def self.count_permitted?(acting_user, method)
       acting_user.admin? # only admins can see details of number of users
    end
    def count_permitted?(method)
       acting_user.admin? || # admins can see anything
       (acting_user == self && method == :orders) # I can count my own orders
    end
 end

Probably sensible defaults in the ApplicationRecord would be something like this:

class ApplicationRecord < ActiveRecord::Base
  def self.count_permitted?(acting_user, method) # note its a class method for scopes
    acting_user.admin?
  end
  def count_permitted?(method) 
    # admins can get counts of all relationships
    # the user model can get counts of all relationships owned by the user and
    # if the model is belongs to a user, then that user can also see all relationships
    acting_user.admin? || self == acting_user || respond_to?(:user) && user == self
  end
end

Once we get some experience we can create some higher level regulation methods to simplify things.

Perhaps in this case it makes more sense to attach the policy to the model itself, in which case we could add features to the scope and relationship methods like this:

class User < ApplicationRecord
  has_many :orders, allow_count: -> { acting_user == self }
end

class Order < ApplicationRecord
  scope :for_vips, :allow_count -> { admin? }
end

I don't know... but its closer...

[catmando]

how about a permit_count method that takes a list of methods, and an optional block. If block returns true count is permitted.

class User < ApplicationRecord
  permit_count :orders { |method| acting_user == self } # object is record to which orders is being applied
end
class Order < ApplicationRecord
  permit_scope_count :for_vips { |acting_user, method| acting_user.admin? }
end

You need to have both method types since one applies to class objects and the other to instance objects

You can then build extensions to has_many and scope using these methods, but still have these methods for more general cases, and also to make code more readable.

Scope chaining creates a problem. Consider this case:

class Order < ApplicationRecord
  permit_scope_count :with_positive_ratings # assumes true if no block given
  permit_scope_count :active { |acting_user| false / true ? what? }
  permit_scope_count :for_vips { |acting_user| acting_user.admin? }
end

There is no right answer for active... this would be okay some_user.orders.active (assuming active_user == some_user) this might not: Order.active unless acting_user.admin? how about this: Order.with_positive_ratings.active ? this probably would be okay Order.for_vips.active if acting_user.admin?

This is officially hard :-)

So is it that each permission returns a no/yes/maybe you might be able to make that work for scopes, but what about some_user.orders.active?

[catmando]

Thinking the above might not be permit_count but just permit_relationship and permit_scope. i.e. does count even have to be involved?

[catmando]

Another way to look at this is to say does acting_user have permission to look at the set of id's in the current scope.

You could in fact implement count that way, but it would be slow.

So maybe the check just needs to be on count.

def count_permitted?(collection, acting_user)
  collection.each { |item| item.check_permission_with_acting_user(:acting_user, :view_permitted?, :id) } rescue nil
end

This actually I think always works, but it would be way too slow.

It also does not handle the case of a scope like for_vips which we might not want some user to be able to filter on in any case.

Which means that indeed the permissions are not even associated with count necessarily but with applying the scope.

The problem is that if applying the scope fails then of course the whole thing fails, but allowing the scope does not necessarily mean counting the id's of the scope should pass.

and oh btw the above "checking every record" method is not secure, since you by counting some subset, and then adding another item to the subset, you can effectively ask who is in each scope.

[catmando]

Some examples

query	acting_user == some_user	acting_user.admin?	acting_user.nil?
some_user.orders.all	good	good	bad
some_user.orders	good	good	bad
Order.all	bad	good	bad
Order.for_user(some_user).all	good	good	bad
some_user.orders.for_vips	bad	good	bad
Order.active	bad	good	bad
some_user.orders.active	good	good	bad
Order.with_postive_ratings	good	good	good
Order.active.with_positive_rating	??	??	??
Order.with_positive_rating.active	??	??	??

[catmando]

Fundamental to how relationships and scopes work is being able to chain them.

For example, we might have the following expression:

Order.for_vips.with_positive_ratings

We cannot always tell if we have permission to apply an aggregate or enumerator method until we get to the end of the chain and attempt to apply the method. For instance, scoping with_positive_ratings may be allowed for to any user (even nil), but for_vips might be only accessible for admins.

And we have to keep in mind that the order of scoping does not matter, so the above is the same as

Order.with_positive_ratings.for_vips

And while saying

Order.with_positive_ratings.for_vips.count

might be illegal unless the acting user is an admin, saying

Order.with_positive_ratings.count

might be legal for any user. So we cannot decide what is allowed until the aggregate or enumerator is actually applied.

Thus we have a kind of tri-state logic where each scope (or relationship) may either

• give permission
• deny permission
• not care

and these must be and'ed together with the following tri-state logic:

tri-state-and	granted	denied	not determined
granted	granted	denied	granted
denied	denied	denied	denied
not determined	granted	denied	not determined

An aggregator or enumerator method can be applied to chain only if permission has been granted. This means:

• at least one element of the chain granted permission
• and no element of the chain denied permission.

In other words, if any element denied permission then no aggregator or enumerator can be applied to the chain.

Likewise if every element in the chain failed to make a decision, then the final decision is also no.

To support this logic the relationship and scope regulations can return a truthy value meaning permission is granted, a falsey value, meaning not determined, or deny permission by using the denied! method (which will simply raise an error, and abort processing.)

class ApplicationRecord < ActiveRecord::Base
  # Admins can see everything, otherwise permission will be granted by other scopes
  regulate_scope :all { acting_user.admin? } 
end

• self is the current relationship or the Model
• acting_user is a method on self that returns the current acting user
• the result of running the block is either a truthy value meaning permission granted, or
• a falsey value meaning permission is indeterminate
• if no regulation is defined for a scope the permission is not determined
• regulate_scope can take multiple names which will share the block

class User < ApplicationRecord
  # grant permission if acting user is an admin or if acting user is this user, otherwise
  # permission is indeterminate
  regulate_relationship :orders { acting_user.admin? || acting_user == self  }
end

• self is the current record for relationship regulations
• acting_user is a method on self
• regulations (like the one defined for all) are inherited by child classes

class Order < ApplicationRecord
  regulate_scope :for_user { |user| acting_user == user || acting_user.admin? }
  regulate_scope :for_vips { denied! unless acting_user.admin? }
  regulate_scope :active { acting_user.admin? }
  regulate_scope :with_positive_ratings # same as returning true from the block
end

• If the scope takes args (like for_user), the same args will be passed to the block
• like acting_user, denied! is a method on self.
• the active scope regulation while adding clarity, is redundant because .all is implicitly applied to all scope changes, and its behavior is inherited from ApplicationRecord

We also allow the regulations to be directly attached to the scopes and regulations using the permit_when option to ActiveRecords has_many and scope macros:

class User < Application::Base
  has_many :orders, permit_when: -> { acting_user == self }
  # or even
  has_many :orders, permit_when: :acting_user? # can be the name of method as well
end

class Order < ApplicationRecord
  scope :for_user, permit_when: -> (user) { acting_user == user || acting_user.admin? } do |user| 
    where(user: user)
  end
  scope :with_positive_ratings, permit_when: true do # short for -> () { true }
  ...
  end
end

[janbiedermann]

I like that, but i also like 'secure by default' and i see 2 items that i think don't contribute well:

1. in the Table i would prefer: granted && not determined --> denied
2. and regulate_scope :with_positive_ratings # same as denied, because undetermined

Why did you prefer otherwise?

[catmando]

Note that the regulations ONLY apply to requests from the client.

Internal requests (such as arising from Controllers, or ServerOps) are as normal "unprotected"

This means that probably the default regulation should be to deny permission. This prevents any scope from being used unless you have explicitly written some regulation for it.

This way its very easy to lock down your scopes, and relationships: You can build a small set of scopes for the specific sets of data your client needs.

[janbiedermann]

i think this is maybe useful, this way also undetermined can be mitigated:

class Order < ApplicationRecord
  regulate_scope_default { |user| acting_user == user || acting_user.admin? }
  # applies to all scopes as default value, is overwritten by a
  regulate_scope :active { acting_user.admin? } # or a
  scope :for_user, permit_when: -> (user) { acting_user == user || acting_user.admin? } do |user| 
    where(user: user)
  end
  regulate_relationship_default { |user| acting_user == user || acting_user.admin? }
  # applies to all relationships as default, is overwritten by a
  has_many :orders, permit_when: -> { acting_user == self }
  regulate_relationship :top_orders { acting_user.admin? || acting_user == self  }
end

[catmando]

@janbiedermann good points.

I had already realized that by default we should assume that every scope is denied! this means that unless the developer has decided on some rule, we are going to assume no!

next point: granted && not-determined --> not-determined Is necessary, to allow chaining of scopes in any order, and to make thinking about the logic practical. You can still get what you want in any case by flipping the logic:

Lets say your regulation looks like this { acting_user == self } this means we are going to grant if acting_user == self, and be indeterminate other wise. But if for whatever reason you want to lock this down completely, you can say { denied! unless acting_user == self }

next point: Well that is just the syntax. saying regulate_scope :with_positive_ratings means the same as regulate_scope :with_positive_ratings { true } if that is not the logic then write what you want. This is especially necessary if we are going to be default make all regulations by default do this: { denied! }

[catmando]

I like the default idea, cause it solves your new user problem nicely, and also makes the overall system easier to explain.

The name should be default_scope_regulation. It can just be a method that is inherited, and it can work like this.

class ActiveRecord::Base
  # The base definitions.  These can be overridden if desired in ApplicationRecord
  # or individual models as needed.

  # If a scope does not have at least one regulation defined, 
  # the default_scope_regulation will be called.
  def default_scope_regulation
    denied! if Rails.env.production?
  end

  # If a has_many relationship does not have at least one regulation defined, 
  # the default_relationship_regulation will be called.
  def default_relationship_regulation
    denied! if Rails.env.production?
  end
end

[sfcgeorge]

Does Hyperloop work with UUIDs? Rails used to use sequential database IDs as primary keys, but now I believe it defaults to UUIDs for primary keys if you use Postgres. This solves enumeration attacks and data leaking of totals in URLs. AKA:

• If I sign up as a new employer and my ID is 1234 I now know there are 1234 employers signed up to this site.
• I can also enumerate records; for example I can scrape the URLs /vacancies/1, /vacancies/2, /vacancies/3 etc.

Those are long standing data leaks most web frameworks suffered from for years. The new Rails PG UUID default mitigates those.

I don't think switching to UUIDs negates any of this issues in this issue as we'd still need to restrict count and scopes and not return huge lists of IDs/UUIDs. But as they're the new Rails default for new projects Hyperloop should at least support them (if it doesn't already).

[catmando]

Added https://github.com/ruby-hyperloop/hyper-mesh/issues/56 to cover UUID case

[sfcgeorge]

Added #59 to cover possibly related send_only being ignored on create.