readpass_file: reject a passphrase file with a stray carriage return - #432
Open
woahwhattheheck wants to merge 2 commits into
Open
readpass_file: reject a passphrase file with a stray carriage return#432woahwhattheheck wants to merge 2 commits into
woahwhattheheck wants to merge 2 commits into
Conversation
readpass_file() finished with passbuf[strcspn(passbuf, "\r\n")] = '\0'; fgets() stops after a "\n", and the fgetc() check above rejects anything which follows it, so a "\n" in the buffer is always the last character. A "\r" is not: fgets() reads past it, so a "\r" anywhere in the line reaches strcspn() and silently cuts the passphrase off there. "--passphrase file:pw" with pw holding "hunter2\rextra" therefore used "hunter2". It round-trips, since encryption truncates the same way, so nothing showed until the passphrase was supplied by another route. Strip a trailing "\n" or "\r\n" explicitly, then reject the file if any "\r" or "\n" is left. Keeping the character as passphrase data would also change the passphrase an existing file yields, so neither answer is safe to pick silently. readpass.h already documented this behaviour. Extend tests/08-passphrase-file.sh: a file holding "hunter2\rextra" must be rejected -- it begins with the correct passphrase, so before this change that decryption succeeded -- and a CRLF file must still work.
Use distinct output paths for the missing-file and incorrect-passphrase cases, and check the path used by each command. Preserve the existing implementation and upstream submission.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #431.
readpass_file()ends withfgets()stops after a\n, and thefgetc(f) != EOFcheck above rejectsanything following it, so a
\nin the buffer is always the last character.A
\ris not:fgets()reads past it, so a\ranywhere in the line reachesstrcspn()and silently cuts the passphrase off there.--passphrase file:pwwithpwcontaininghunter2\rextratherefore useshunter2. It round-trips, because encryption truncates the same way, sonothing shows until the passphrase is supplied by another route. The header
in
libcperciva/util/readpass.halready says a file like that should berejected:
The change
Strip a trailing
\nor\r\nexplicitly, then reject the file if any\ror
\nis left. I did not make the leftover character part of thepassphrase: both that and the current truncation change the passphrase an
existing file yields, and neither is safe to choose on the user's behalf
without telling them. No documentation change — this makes the code match
what
readpass.halready describes.Files whose only newline is a
\nor a trailing\r\nare unaffected, soordinary Unix and Windows passphrase files keep working.
Test
tests/08-passphrase-file.shgains four checks rather than a new test file:hunter2\rextramust be rejected, with theerror message, and must not create an output file. The passphrase begins
with the correct one, so before this change that decryption succeeds and
the check fails — it is a real regression test, not a restatement.
file correctly.
Note
This is
libcpercivacode, so it also lives in tarsnap and kivaloo; I haveonly touched the copy in this repository.