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

Consider introducing limits on resource usage by maybe-rogue hash encodings #54

Open
solardiz opened this issue Nov 1, 2018 · 6 comments
Labels
documentation Primarily an issue with the documentation. enhancement Requests a new feature or improvement. Without "need more information", we agree it's desirable.

Comments

@solardiz
Copy link
Collaborator

solardiz commented Nov 1, 2018

Historically, hashes were fixed-cost and thus OK for semi-trusted users to be able to specify directly e.g. in Apache httpd .htpasswd files. With tunable-cost hashes, this changes - a semi-trusted user could DoS the service for its other users, and if no adequate resource limits are configured on the service then also DoS the system. musl chose to impose some hard-coded sanity limits on those hashes. With libxcrypt's support for hashes tunable not only for time, but also for memory (scrypt, yescrypt), this may be even more of an issue.

I am undecided on whether we should merely document the issue or also impose limits, and if so whether they should be hard-coded or configurable, and if so compile- or run-time configurable. To avoid duplicate parsing, some logic might need to be added to upstream yescrypt tree and made use of by libxcrypt.

At least we should make an informed decision. Hence opening this issue.

@zackw
Copy link
Collaborator

zackw commented Nov 1, 2018

The crypt.conf format I proposed in #26 could easily be extended to support run-time configurable upper limits.

I think we should certainly document the issue, and I think there's a strong case for doing more than that, but I am not sure I know what that is. Do you have links to the discussion that happened when musl added limits?

@solardiz
Copy link
Collaborator Author

solardiz commented Nov 1, 2018

@rfc1036
Copy link

rfc1036 commented Nov 2, 2018

If there is a consensus that the problem is worth solving then it must be solved in libxcrypt: since there is no standard scheme to specify the hash parameters then every interested program would have to implement parsing every hash format, thus negating the generality of libxcrypt.

@zackw
Copy link
Collaborator

zackw commented Nov 8, 2018

I just revised the crypt.conf spec so that there are now defcost= and maxcost= keyval arguments for each hashing method. We'd need to add more knobs if we ever allow tuning memory separately from time parameters, but I think this will do for the time being.

Unresolved questions:

  • Does the documentation I wrote explain the issue clearly enough?
  • What should the default upper time limit used by crypt-tune-costs be? One second? Ten?
  • Should there be a mincost= separate from defcost? Right now, the documented behavior is that you can specify costs lower than defcost when calling crypt_gensalt but the result will be flagged as "too cheap" by crypt_checksalt. It might make sense to allow these to be controlled separately; it might also make sense to disallow costs lower than defcost in crypt_gensalt, I am undecided.

@solardiz
Copy link
Collaborator Author

solardiz commented Nov 8, 2018

I haven't reviewed this carefully yet, but conceptually I think a single numeric maxcost= can't fully/directly address the issue of malicious hash encodings manually put into .htpasswd files and the like for hash types that have more than one parameter specified via the setting strings (in current libxcrypt that's scrypt and yescrypt). So perhaps we need something else, or we need some reverse translation layer (a hack) that would convert arbitrary setting strings into single numeric cost values that are at least as resource-demanding as those setting strings would be in terms of max(time, memory) consumption. Besides being a hack, this notion might be difficult for us to explain and for users to grasp.

@zackw
Copy link
Collaborator

zackw commented Nov 11, 2018

conceptually I think a single numeric maxcost= can't fully/directly address the issue of malicious hash encodings manually put into .htpasswd files and the like for hash types that have more than one parameter specified via the setting strings (in current libxcrypt that's scrypt and yescrypt).

You're right. I was thinking only in terms of what can be expressed via the crypt_gensalt interface, not what can be expressed by manual construction of setting strings.

Abstractly what we want is maxcpu= and maxtime= knobs that apply across the board, but I don't see a good way to implement that short of having libxcrypt fork off a child process on every call to crypt so it can apply OS-level resource limits. Maybe that's not a totally crazy idea, tbh, but we'd have to worry about all the hair that comes with calling fork in a library. It feels more of an authentication daemon feature.

@besser82 besser82 added enhancement Requests a new feature or improvement. Without "need more information", we agree it's desirable. documentation Primarily an issue with the documentation. labels Nov 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Primarily an issue with the documentation. enhancement Requests a new feature or improvement. Without "need more information", we agree it's desirable.
Projects
None yet
Development

No branches or pull requests

4 participants