Coherence Security Issues

[GriffinMB]

Thanks again for taking the time to talk with me. I'm opening an issue as per our conversation:

The Coherence library has "Mass Assignment"-like vulnerabilities. In particular, "registration"
endpoints (like creating, editing, updating), allow users to update any coherence_fields. This
means that, among other issues, users can automatically confirm their accounts by sending the confirmed_at parameter with their registration request. Further, the library design and documentation encourages insecure functionality by default.

For example:

• The "store" demo allows registering users to add themselves as admin:
  https://github.com/smpallen99/store/blob/master/web/models/coherence/user.ex

Due to these issues, I would consider officially "retiring" the current version of Coherence in hex.

[GriffinMB]

Hi @newdayrising! It looks like you misunderstood my suggestion.

It is possible to retire specific versions of libraries on Hex.pm. My recommendation is that, once a new release is made, this specific version be retired so that people do not inadvertently use an insecure dependency.

[newdayrising]

I personally don't like the tone that I took myself. I'll just go ahead and delete that comment :D

[joshprice]

I'm running sobelow 0.6.2 and getting this error:

Known Vulnerable Dependency - Coherence v0.5.1

Is this the same version you are referring to in this issue @GriffinMB ?

[GriffinMB]

@joshprice - Yes, it is.

[joshprice]

@GriffinMB Thanks! Given the large surface area of Coherence and the fact that we don't actually implement any user registration functionality (thus avoiding the described vulnerability) it's probably worth indicating in sobelow what the vulnerability actually is along with any known mitigation strategies.

[GriffinMB]

@joshprice - No problem! If you run the command with the -v flag (e.g. mix sobelow -v), there is a link to this issue. I could do a better job documenting the flag, but, in general, that's what you'll want to use if you're looking for more information!

It is also worth noting that the issue is not limited to registration, though certainly not all applications using Coherence will be critically vulnerable. At any rate, I don't want to derail the discussion in this issue, so if you want to continue the conversation feel free to open an issue on sobelow!

[joshprice]

@GriffinMB Good to know about -v, I found this issue by looking through the issues in Coherence manually.

If the issues aren't limited to user registration could you please expand on which other areas are affected so that anyone following the link in sobelow can be sure they are or aren't affected?

Thanks!

[GriffinMB]

@joshprice - I will update the original issue to include additional information in the next week or so. But, at a high level (for anyone reading this in the mean time), anything in the user's changeset method can be updated by any of the library's user-update functions. That could be registration, or updating the user profile, etc.

For example:

  def changeset(model, params \\ %{}) do
    model
    |> cast(params, [:your, :params] ++ coherence_fields)
    |> validate_required([:name, :email])
    |> unique_constraint(:email)
    |> validate_coherence(params)
  end

Here, :your, :params, and any coherence_fields can be set by a user updating an account, or registering. If you have any sensitive fields exposed in the changeset function, or are relying on any of the default fields (like confirmed_at) not being user-editable, your application is vulnerable to this issue. If you only allow login, but not registration or account updates, this issue may not be exploitable within your app.

I need to refresh my memory on the full scope of the vulnerability, but I will update the original issue with this information and more when time permits!

[acrolink]

Correct me if I am wrong, but this issue is found in any User model exposing a path to update parameters. What is needed is a flag that some fields are editable by user with id = 1 or something similar.

[GriffinMB]

Correct me if I am wrong, but this issue is found in any User model exposing a path to update parameters.

This is correct! Typically, you would avoid this by having different changeset functions for different user-types and different actions (create, update, etc). You would also want to have separate functions for generating values for columns like created_at rather than passing them to the changeset function directly.

[smpallen99]

@GriffinMB I don't understand the discussion regarding created_at. First, the default field in Ecto (and Coherence) is :inserted_at. Second, these fields are not updatable through the changeset. For example:

iex(4)> User.changeset(u, %{inserted_at: NaiveDateTime.utc_now})
#Ecto.Changeset<action: nil, changes: %{}, errors: [],
 data: #SamplePhx.Coherence.User<>, valid?: true>

I understand and agree with the other fields....

[GriffinMB]

@smpallen99 It was only an example of a possible field that you would generate a value for, not one that necessarily exists.

[seanculver]

In the meantime, while this is getting resolved, what are the steps needed to patch this?

[smpallen99]

Got it. Thanks...

[seanculver]

What I've done is created a separate registration changeset that only has

    field :name, :string
    field :email, :string
    field :password
    field :password_confirmation, :string, virtual: true

I'm not using the routes for view / update / edit for registration_controller and have instead rolled my own. I'm not using coherence_schema or coherence_fields.

If I'm understanding the vulnerability then I should have essentially worked around it, right?

[GriffinMB]

@seanculver - As long as you aren't using coherence_schema, coherence_fields, or the default changeset & associated endpoints, you should be fine as far as this issue is concerned.

[seanculver]

@GriffinMB That is the case, thanks!

I did notice some other issues with ProjectName.Coherence.Schema aka project_name/coherence/schemas.ex when I ran Sobelow, I haven't tried removing that file, I Imagine it is being used throughout coherence. Thoughts on those items?

[GriffinMB]

@seanculver - No problem!

I haven't looked much into those issues, but I believe they are false-positives. It looks like it's flagging a bunch of String.to_atom calls used in some meta-programming functions. Those shouldn't be an issue because they are only called a compile-time and won't touch user input.

[newpain01]

Is this issue still not resolved? What are the recommended steps to avoid these security issues?

[GriffinMB]

Hi @newpain01! To my knowledge, these issues are not resolved. Avoiding these issues is difficult if you are fully using this library, but if you are only using bits of it @seanculver's comment above has the right idea.

[NicoleG25]

Hi @smpallen99 , was this issue ever resolved?
I'm doing some security research and it appears that CVE-2018-20301 was assigned to this issue.
Could you point me to the fix if there is one ? Thanks !