besser82 / libxcrypt

Extended crypt library for descrypt, md5crypt, bcrypt, and others
GNU Lesser General Public License v2.1
189 stars 52 forks source link

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

Open solardiz opened 5 years ago

solardiz commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

Here are two relevant messages/threads:

https://www.openwall.com/lists/musl/2012/08/08/20 https://www.openwall.com/lists/oss-security/2013/06/12/1

rfc1036 commented 5 years ago

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 commented 5 years ago

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:

solardiz commented 5 years ago

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 commented 5 years ago

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.