crypt_des: fail if salt/session is too short

[brooks]

Summary

Check that the string is long enough before accessing it. With sufficent
optimization the code would unconditionally load two byte from the
string even if it was an empty string.

For old-style hashes, retain support for 1-byte salts. For new-style
hashes, require 9 non-NUL bytes. In both cases, don't check for
termination beyond that point as a programmer might have passed the
whole hash or an unterminated byte array.

Found with: CHERI
Sponsored by: Innovate UK

[brooks]

secure/lib/libcrypt/crypt-des.c

618

I'm a little unclear if these should be < # to allow for sloppy code that passes the whole based password.

[brooks]

I found this due to running the tests on CheriBSD and some of the tests pass "" as an invalid salt.

secure/lib/libcrypt/crypt-des.c

618

s/based/hashed/

[jrtc27]

secure/lib/libcrypt/crypt-des.c

671

I don't think this is right? It pads with setting[0] not setting[1] when setting is length 1. Which you've now made invalid input, which may or may not be correct to do, given the code went out of its way to handle that... see 49de41577e22f26f71caed8f4791bdb8b6839db0 from 1994, which I think "safely" handles settings short by 1 byte for both styles, but unsafely handles any shorter by more bytes.

[brooks]

secure/lib/libcrypt/crypt-des.c

671

I guess I should be more conservative and continue to handle the 1-character string case. 5f521d7ba72145092ea23ff6081d8791ad6c1f9d seems to have dropped the handling in the above commit without explanation.

[brooks]

Retain support for one character salts.

[jrtc27]

secure/lib/libcrypt/crypt-des.c

671

I think the handling for 1-character is still there, but it's botched too-long new-style strings because it no longer makes sure to terminate?

[brooks]

Allow new-style salts to be longer than 9 characters.

[brooks]

For newly added reviewers, I'm aiming to fix a narrow set of UB triggered by the test suite passing "" as the session argument when running lib/libcrypt/crypt_test:crypt_salts. On CHERI the test crashes and ends up marked as broken because "" has only one accessible byte. I've chosen to take a fairly loose approach to acceptable input since that seems to be traditional here.