search

google / fscrypt

Go tool for managing Linux filesystem encryption
Apache License 2.0
886 stars 99 forks source link

clamp uint8 nCPUs variable when nCPUS >= 256 #363

Closed ebenali closed 2 years ago

ebenali commented 2 years ago

Panics on a dual-socket 7742 epyc box with 256 logical cores when a int64 is cast to uint8, inducing an argon2 routine to fail upon encountering a value of 0 (zero).

google-cla[bot] commented 2 years ago

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

ebiggers commented 2 years ago

Great catch!

I'm a bit concerned about systems with more than 256 CPUs. Previously they would have been doing the truncation to 8 bits. If we clamp to 255, then they'd no longer be able to unlock their directories, as the password hash function would have changed.

How about clamping costs.Parallelism to 255 when generating and reading the config file, in combination with using p=1 if uint8(costs.Parallelism) == 0?

ebiggers commented 2 years ago

Actually, the Argon2 algorithm supports p in the range [0, 2**24 - 1]. So the real bug seems to be in the Argon2 library we're using; it needs to be fixed to accept larger values of p.

josephlr commented 2 years ago

I agree with @ebiggers here, we need to do this in such a way that we don't cause data-loss for older users.

To get the best behavior going forward, we could introduce a new TruncationFixed param to HashingCosts. That would indicate that costs.Parallelism actually holds the correct value. Otherwise, if the value is false (the default) we would have the previous behavior, where the parallelism cost is uint8(costs.Parallelism).

josephlr commented 2 years ago

Thanks for the work on this @ebenali, but I'm closing this in favor of #365

It seems like properly fixing this might be a little more involved.