On Wed, Aug 08, 2018 at 02:45:29PM +0100, Ricardo Mestre wrote:
> Hi,
>
> When openssl(1) passwd is invoked without passing in the `password' as
> argument
> , meaning interactively, and if the password is 10 or more characters it will
> show the following memory corruption error, and using -crypt which is the
> default:
>
> openssl(43025) in free(): chunck canary corrupted 0x13a8dc4a1bb0 0xa@0xa
>
> pw_len is set to 8, then passwd_malloc_size is set to pw_len + 2 in order to
> be
> able to warn the user that the password will be truncated then it calls
> EVP_read_pw_string(3) which allocates the space size of the input buffer, in
> this case password_malloc_size plus 1 for the NUL-termination character
> through
> strlcpy(3).
>
> When we finally call free(password_malloc) the sizes will differ and the
> memory
> will be corrupted, in order to solve this when we allocate memory for the
> input
> buffer we need to add plus 1 for the NUL-termination character.
>
> Comments? OK?
Nice find. I think this is an off-by-one in EVP_read_pw_string_min()'s
usage of UI_add_input_string() (and UI_add_verify_string()). The man
page explicitly says that NUL is *not* counted in minlen and maxlen.
UI_add_input_string() and UI_add_verify_string() add a prompt to ui, as
well as flags and a result buffer and the desired minimum and maximum
sizes of the result, not counting the final NUL character.
Notice that EVP_read_pw_string(buf, sizeof buf, ...) is a common enough
idiom. All these are subject to a similar buffer overrun. On the other
hand, everywhere else our tree has the correct usage of
UI_add_input_string(ui, prompt, flags, buf, 0, sizeof(buf) - 1),
so I believe we should fix this in EVP_read_pw_string_min() as follows:
Index: evp/evp_key.c
===================================================================
RCS file: /var/cvs/src/lib/libcrypto/evp/evp_key.c,v
retrieving revision 1.24
diff -u -p -r1.24 evp_key.c
--- evp/evp_key.c 29 Jan 2017 17:49:23 -0000 1.24
+++ evp/evp_key.c 8 Aug 2018 15:23:32 -0000
@@ -107,11 +107,11 @@ EVP_read_pw_string_min(char *buf, int mi
if (ui == NULL)
return -1;
if (UI_add_input_string(ui, prompt, 0, buf, min,
- (len >= BUFSIZ) ? BUFSIZ - 1 : len) < 0)
+ (len >= BUFSIZ) ? BUFSIZ - 1 : len - 1) < 0)
return -1;
if (verify) {
if (UI_add_verify_string(ui, prompt, 0, buff, min,
- (len >= BUFSIZ) ? BUFSIZ - 1 : len, buf) < 0)
+ (len >= BUFSIZ) ? BUFSIZ - 1 : len - 1, buf) < 0)
return -1;
}
ret = UI_process(ui);
