RustCrypto / formats

Cryptography-related format encoders/decoders: DER, PEM, PKCS, PKIX
228 stars 122 forks source link

Don't fail with InvalidLength when reading nothing at end of data #1387

Closed mpalmer closed 2 months ago

mpalmer commented 2 months ago

This is a curious corner case that can happen when, for example, you're reading the length-value encoded strings in OpenSSH. If the last string in the data is of zero-length, then decode() will be called with an empty buffer, so we don't need to read any bytes. However, since is_finished() was checked unconditionally, the zero-length read (which would be OK) was instead rejected because there was no data to (not) read.

The bug was particularly mischievous because is_finished() will return false if there's = padding, so 2/3 of empty read calls will work normally.

tarcieri commented 2 months ago

Is this really necessary and the best solution? Wouldn’t it be better not to perform a zero length read in the first place?

mpalmer commented 2 months ago

Is this really necessary and the best solution? Wouldn’t it be better not to perform a zero length read in the first place?

If "don't perform zero length reads" is the best solution, that's cool, although in that case it would be better, IMO, if this method always rejected zero length reads. The thing that made this bug so hard to track down was the fact that the zero length read only failed in a very narrow set of circumstances.

tarcieri commented 2 months ago

I think it's fine to always reject zero length reads

mpalmer commented 2 months ago

Righto, I'll rework the PR to go that way.

mpalmer commented 2 months ago

OK, base64ct now errors on zero-length decode requests, with InvalidLength.