nccgroup / sobelow

Security-focused static analysis for the Phoenix Framework
Apache License 2.0
1.67k stars 92 forks source link

casting foreign keys in ecto changesets #29

Open ghost opened 6 years ago

ghost commented 6 years ago

👋

Not particularly phoenix related, but is it possible to catch if foreign keys can be set from params passed in by a user through a controller action? Abusing different ecto casts down the line (up the stack?) allows attackers to modify resources that don't belong to them.

I've seen many phoenix projects do something like this

# controller
def some_action(conn, params) do
  # ...
  Resources.create(params) # or update, or delete
  # ...
end

# schema / changeset
schema "resources" do
  # ...
  belongs_to(:something, Something)
  # ...
end

def changeset(resource, attrs) do
  resource
  |> cast(attrs, [:something_id, ...])
  # or cast_assoc(:something)
  # or put_assoc(:something)
end

# resources context 
def create(attrs) do
  %Resource{}
  |> Resource.changeset(attrs)
  |> Repo.insert()
end

As a possible solution / suggestion:

# don't cast foreign keys in Resource.changeset 
# but pass them in Resources.create

@spec create(attrs, for: pos_integer) :: {:ok, Resource.t} | {:error, Ecto.Changeset.t}
def create(attrs, for: something_id) do
  %Resource{something_id: something_id}
  |> Resource.changeset(attrs)
  |> Repo.insert()
end

# or

@spec create(attrs, for: Something.t) :: {:ok, Resource.t} | {:error, Ecto.Changeset.t}
def create(attrs, for: %Something{id: something_id}) do
  %Resource{something_id: something_id}
  |> Resource.changeset(attrs)
  |> Repo.insert()
end

# controller action then becomes
def some_action(conn, params) do
  # something or something_id are set by some plug (possibly based on user id)
  something = conn.assigns.something
  # or something_id = conn.assigns.something_id
  Resources.create(params, for: something)
  # ...
end
GriffinMB commented 6 years ago

Hi! Thank you for the feature request. I'm not sure how soon it will happen, but something like this will definitely be added in the (hopefully) near future!

I think this general class of vulnerability is one of the more common issues in Phoenix applications.

ryanwinchester commented 3 years ago

This would be useful, but only if you can ignore specific changesets somehow.