Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ability to specify a custom cursor module #9

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

cdunn
Copy link
Contributor

@cdunn cdunn commented Feb 24, 2018

Seems like it might be best practices (at least in certain circumstances where cursor fields are private) to allow encryption of the cursor via a custom cursor module.

Appears there are valid reasons not to directly binary_to_term external input. Also not clear on whether there are any ecto related exploits of specially crafted cursors terms (rails has made me paranoid about nested maps and such) so would be useful to guarantee it is tamper resistant.

I haven't done a tremendous amount of elixir so lmk what you think about the concept and any thoughts on the implementation.

Reference:
Plug.Session.COOKIE
Plug.Crypto.safe_binary_to_term

@stevedomin
Copy link
Member

Hi @cdunn,

Thanks for the PR.

I'm on the fence about this one.

  1. I encrypting cursor is the right thing to do it shouldn't be an option, it should be the default. Can you think of other cases where it would make sense to do this?
  2. While I'm not entirely happy with the binary_to_term/1, I can't really see how it would be an attack vector now. Any proof of concept?

I'll look into it a bit more on my side. Maybe there's a way to do this without using binary_to_term/1.

@cdunn
Copy link
Contributor Author

cdunn commented Feb 26, 2018

I suppose the alternative could be typechecking the decoded values against the db columns but that might get a little messy.

The example issues that come to mind:

  1. Nested functions that could allow for remote code execution (see https://github.com/elixir-plug/plug/blob/4383cd5ddfd1d7896286689bac676bd2bedbc66b/test/plug/crypto_test.exs#L35)
  2. Nested or crafted hashes that get converted to weird SQL via ecto ('OR 1=1' type stuff)
  3. Sorting on a set of cursors that are not public to the user, for example some sort of score/ranking/id heuristic where you dont want the user to be able to decode the cursor value

I'm mostly concerned about 1 and 2, so I need to do a little more research on potential issues of blinding passing user input into ecto and come up with some specifics. I'm mostly just thinking of all the past issues with ActiveRecord in Rails and all the sec issues with converting JSON/YAML from user input.

I like the idea of encrypting by default but it does add a bit to the setup and I definitely see how depending on the use case, these could be acceptable levels of risk for the application.

@stevedomin
Copy link
Member

Hi @cdunn, hope you are well. Sorry for being slow to come back to you on this on, last few days have been hectic.

I'll have a deeper look at it this week.

Base automatically changed from master to main January 26, 2021 17:52
@doorgan
Copy link

doorgan commented Aug 22, 2024

Hi! Is this PR something worth revisiting?
I'm interested in the ability to change the term encoding(I want to solve the same problem as #62), which this PR supports

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants