readpass_file: reject a passphrase file with a stray carriage return

libcperciva/util/readpass_file.c

@@ -22,6 +22,7 @@ readpass_file(char ** passwd, const char * filename)
 {
 	FILE * f;
 	char passbuf[MAXPASSLEN];
+	size_t len;
 
 	/* Open the file. */
 	if ((f = fopen(filename, "r")) == NULL) {
@@ -52,8 +53,28 @@ readpass_file(char ** passwd, const char * filename)
 		goto err1;
 	}
 
-	/* Truncate at any newline character. */
-	passbuf[strcspn(passbuf, "\r\n")] = '\0';
+	/*
+	 * Strip a trailing "\n" or "\r\n".  fgets() stops after a "\n", so
+	 * if the file contains one then it is the last character we read.
+	 */
+	len = strlen(passbuf);
+	if ((len > 0) && (passbuf[len - 1] == '\n')) {
+		passbuf[--len] = '\0';
+		if ((len > 0) && (passbuf[len - 1] == '\r'))
+			passbuf[--len] = '\0';
+	}
+
+	/*
+	 * Any "\r" or "\n" left is not a line ending we strip.  We could cut
+	 * the passphrase short there, or keep the character as part of the
+	 * passphrase, but both change the passphrase which an existing file
+	 * yields, and neither is safe to pick silently.  Refuse the file.
+	 */
+	if (strcspn(passbuf, "\r\n") != len) {
+		warn0("passphrase in %s contains a carriage return or newline",
+		    filename);
+		goto err1;
+	}
 
 	/* Copy the password out. */
 	if ((*passwd = strdup(passbuf)) == NULL) {

tests/08-passphrase-file.sh

@@ -10,6 +10,14 @@ decrypted_reference_file="${s_basename}-attempt_reference.txt"
 decrypted_badpass_file="${s_basename}-decrypt-badpass.txt"
 decrypted_badpass_log="${s_basename}-decrypt-badpass.log"
 decrypted_no_file_log="${s_basename}-decrypt-no-file.log"
+decrypted_no_file="${s_basename}-decrypt-no-file.txt"
+cr_passphrase_file="${s_basename}-passphrase-cr.txt"
+decrypted_cr_file="${s_basename}-decrypt-cr.txt"
+decrypted_cr_log="${s_basename}-decrypt-cr.log"
+encrypted_cr_file="${s_basename}-encrypt-cr.enc"
+encrypted_cr_log="${s_basename}-encrypt-cr.log"
+crlf_passphrase_file="${s_basename}-passphrase-crlf.txt"
+decrypted_crlf_file="${s_basename}-decrypt-crlf.txt"
 
 scenario_cmd() {
 	# Create the passphrase file.
@@ -32,7 +40,7 @@ scenario_cmd() {
 	setup_check "scrypt dec file none"
 	${c_valgrind_cmd} "${bindir}/scrypt"				\
 	    dec --passphrase file:THIS_FILE_DOES_NOT_EXIST		\
-	    "${encrypted_reference_file}" "${decrypted_reference_file}"	\
+	    "${encrypted_reference_file}" "${decrypted_no_file}"		\
 	    2> "${decrypted_no_file_log}"
 	expected_exitcode 1 $? > "${c_exitfile}"
 
@@ -44,7 +52,7 @@ scenario_cmd() {
 
 	# We should not have created a file.
 	setup_check "scrypt dec file none no file"
-	test -e "${decrypted_badpass_file}"
+	test -e "${decrypted_no_file}"
 	expected_exitcode 1 $? > "${c_exitfile}"
 
 	# Attempt to decrypt the reference file with an incorrect passphrase.
@@ -53,7 +61,7 @@ scenario_cmd() {
 	echo "bad-pass" > "${bad_passphrase_file}"
 	${c_valgrind_cmd} "${bindir}/scrypt"				\
 	    dec --passphrase file:"${bad_passphrase_file}"		\
-	    "${encrypted_reference_file}" "${decrypted_reference_file}"	\
+	    "${encrypted_reference_file}" "${decrypted_badpass_file}"		\
 	    2> "${decrypted_badpass_log}"
 	expected_exitcode 1 $? > "${c_exitfile}"
 
@@ -66,4 +74,57 @@ scenario_cmd() {
 	setup_check "scrypt dec file bad no file"
 	test -e "${decrypted_badpass_file}"
 	expected_exitcode 1 $? > "${c_exitfile}"
+
+	# A passphrase file containing a carriage return which is not part of
+	# a trailing CRLF must be rejected.  The passphrase used here begins
+	# with the correct passphrase, so accepting it would decrypt the file
+	# with a passphrase which is not the one the file contains.
+	setup_check "scrypt dec file stray CR"
+	printf '%s\rextra\n' "${password}" > "${cr_passphrase_file}"
+	${c_valgrind_cmd} "${bindir}/scrypt"				\
+	    dec --passphrase file:"${cr_passphrase_file}"		\
+	    "${encrypted_reference_file}" "${decrypted_cr_file}"	\
+	    2> "${decrypted_cr_log}"
+	expected_exitcode 1 $? > "${c_exitfile}"
+
+	# We should have received an error message.
+	setup_check "scrypt dec file stray CR error"
+	grep -q "carriage return or newline" "${decrypted_cr_log}"
+	echo "$?" > "${c_exitfile}"
+
+	# We should not have created a file.
+	setup_check "scrypt dec file stray CR no file"
+	test -e "${decrypted_cr_file}"
+	expected_exitcode 1 $? > "${c_exitfile}"
+
+	# Encryption must reject the same malformed passphrase file rather than
+	# silently encrypting with the prefix before the carriage return.
+	setup_check "scrypt enc file stray CR"
+	${c_valgrind_cmd} "${bindir}/scrypt"				\
+	    enc --passphrase file:"${cr_passphrase_file}"		\
+	    "${reference_file}" "${encrypted_cr_file}"		\
+	    2> "${encrypted_cr_log}"
+	expected_exitcode 1 $? > "${c_exitfile}"
+
+	# We should have received the same malformed-passphrase diagnostic.
+	setup_check "scrypt enc file stray CR error"
+	grep -q "carriage return or newline" "${encrypted_cr_log}"
+	echo "$?" > "${c_exitfile}"
+
+	# We should not have created an encrypted output file.
+	setup_check "scrypt enc file stray CR no file"
+	test -e "${encrypted_cr_file}"
+	expected_exitcode 1 $? > "${c_exitfile}"
+
+	# A passphrase file with CRLF line endings must still work.
+	setup_check "scrypt dec file CRLF"
+	printf '%s\r\n' "${password}" > "${crlf_passphrase_file}"
+	${c_valgrind_cmd} "${bindir}/scrypt"				\
+	    dec --passphrase file:"${crlf_passphrase_file}"		\
+	    "${encrypted_reference_file}" "${decrypted_crlf_file}"
+	echo $? > "${c_exitfile}"
+
+	setup_check "scrypt dec file CRLF output against reference"
+	cmp -s "${decrypted_crlf_file}" "${reference_file}"
+	echo $? > "${c_exitfile}"
 }