Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

[vulnerability] The ability to grab any address in the database through the checkout process #53

Closed
AdnanTheExcellent opened this Issue Apr 11, 2013 · 6 comments

Comments

Projects
None yet
3 participants

In the checkouts controller, there is a before_filter called "set_addresses" which decides whether to use the id on a selected address from the address book, or a new address hash from the form. However, there is no check to make sure that the current_user actually owns that address_id.

def set_addresses
    return unless params[:order] && params[:state] == "address"

    if params[:order][:ship_address_id].to_i > 0
      params[:order].delete(:ship_address_attributes)
    else
      params[:order].delete(:ship_address_id)
    end

    if params[:order][:bill_address_id].to_i > 0
      params[:order].delete(:bill_address_attributes)
    else
      params[:order].delete(:bill_address_id)
    end

  end

one could tamper with the radio buttons and permutate through the possible address ids in the selected radio button in the form and continue to payment, which would allow them to be able to grab all the addresses from the database. In the process, all the addresses will now link to that users account, instead of the original users account.

This is a pretty big security risk and should be patched as soon as possible. . My project is heavily modified so i unfortunately don't have a fix for this gem that i can merge. Something like the following should be a good lead to fixing the issue:

def set_addresses
    return unless params[:order] && params[:state] == "address"

    if params[:order][:ship_address_id].to_i > 0
      params[:order].delete(:ship_address_attributes)
      if Spree::Address.find(params[:order][:ship_address_id]).user != spree_current_user
          #raise some error
      end
    else
      params[:order].delete(:ship_address_id)
    end

    if params[:order][:bill_address_id].to_i > 0
      params[:order].delete(:bill_address_attributes)
      if Spree::Address.find(params[:order][:bill_address_id]).user != spree_current_user
          #raise some error
      end
    else
      params[:order].delete(:bill_address_id)
    end

  end

Member

iloveitaly commented Apr 11, 2013

Could you convert your suggested edit into a pull request so the @romul can more easily get this fixed merged in?

@iloveitaly I dont have a fix i can merge. That was really just code that explains what i would do to fix it. I was messing around with this gem in development today and noticed that vulnerability.

Collaborator

jumph4x commented Apr 12, 2013

On it. I'll take care of this.

jumph4x added a commit that referenced this issue Apr 12, 2013

Collaborator

jumph4x commented Apr 12, 2013

I threw up the suggested fix up there as an emergency. No test attached yet. If someone wants to do this, please do.

jumph4x added a commit that referenced this issue Apr 12, 2013

Collaborator

jumph4x commented Apr 12, 2013

Keeping this open as a reminder to write a test. Running into trouble with rake test_app... ugh.

jumph4x added a commit that referenced this issue Apr 12, 2013

@iloveitaly iloveitaly referenced this issue Jul 13, 2014

Open

Future Improvements List #73

0 of 3 tasks complete
Member

iloveitaly commented Jul 13, 2014

Closing this out, moving to #73

@iloveitaly iloveitaly closed this Jul 13, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment