search

paragonie / paseto

Platform-Agnostic Security Tokens
https://paseto.io
Other
3.24k stars 108 forks source link

Null salt? #142

Closed aidantwoods closed 3 years ago

aidantwoods commented 3 years ago

Hello again!

Just a quick question, having been a while since I've looked at this reference implementation :)

Is there a reason that $salt (n in docs) is allowed to be null here?

https://github.com/paragonie/paseto/blob/e16113beeaddeee8548a67afb75d288570518a46/src/Keys/SymmetricKey.php#L198-L212

As far as I can see reading the docs, this should have a fixed length of 32 when encrypting (assuming no catastrophic OS CSPRNG failure), and I can't spot any code paths that would seem to allow null to make its way here in any case (PHP docs indicate random_bytes will return a string or throw. When decrypting, it would be possible for this to be a lesser length than 32 (could/should we be failing early in this case based on length?), but again, I can't see a code path that would allow this to be null.

I've only manually examined v3 in detail for this, but psalm seems to be happy with dropping null from all of the key-splitting implementations.

If you agree, then I can raise a PR using https://github.com/aidantwoods/paseto/tree/remove-null-salt ?

paragonie-security commented 3 years ago

We call it a salt, but it's not going into the HKDF salt field. It's being appended to the input.

https://github.com/paragonie/paseto/blob/e16113beeaddeee8548a67afb75d288570518a46/src/Util.php#L118-L124

See https://github.com/paseto-standard/paseto-spec/blob/master/docs/Rationale-V3-V4.md#better-use-of-hkdf-salts-change for information.

paragonie-security commented 3 years ago

I can't spot any code paths that would seem to allow null to make its way here in any case (PHP docs indicate random_bytes will return a string or throw. When decrypting, it would be possible for this to be a lesser length than 32 (could/should we be failing early in this case based on length?), but again, I can't see a code path that would allow this to be null.

Correct, I don't think we ever allow a NULL salt.

aidantwoods commented 3 years ago

We call it a salt, but it's not going into the HKDF salt field. It's being appended to the input.

Yup, I'm just referring to it by the variable name for simplicity :)

Correct, I don't think we ever allow a NULL salt.

I've opened #143 to drop null

aidantwoods commented 3 years ago

Ah, you already merged it while I was typing!

paragonie-security commented 3 years ago

I have a wall-mounted widescreen monitor in portrait mode for reviewing PRs :)