On Wed, Aug 08, 2018 at 05:53:56PM +0200, Theo Buehler wrote:
> 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:
I got some oks on this, but here's a diff that I like better since it's
more defensive.
First, ints min and len only make sense if they're non-negative, so
let's check for that. Second, let's check for the maxlen < minlen
situation as the UI_* family of functions doesn't seem to do that.
Third, it seems easier to cap len at BUFSIZ instead of repeating the use
of the ternary operator.
I forgot to mention that this diff also aligns EVP_read_pw_string() with
the documented behavior whereas currently buf would need to be of size
length+1 to avoid the buffer overrun.
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 9 Aug 2018 06:13:53 -0000
@@ -101,17 +101,20 @@ EVP_read_pw_string_min(char *buf, int mi
char buff[BUFSIZ];
UI *ui;
+ if (min < 0 || len - 1 < min)
+ return -1;
+ if (len > BUFSIZ)
+ len = BUFSIZ;
if ((prompt == NULL) && (prompt_string[0] != '\0'))
prompt = prompt_string;
ui = UI_new();
if (ui == NULL)
return -1;
- if (UI_add_input_string(ui, prompt, 0, buf, min,
- (len >= BUFSIZ) ? BUFSIZ - 1 : len) < 0)
+ if (UI_add_input_string(ui, prompt, 0, buf, min, len - 1) < 0)
return -1;
if (verify) {
- if (UI_add_verify_string(ui, prompt, 0, buff, min,
- (len >= BUFSIZ) ? BUFSIZ - 1 : len, buf) < 0)
+ if (UI_add_verify_string(ui, prompt, 0, buff, min, len - 1, buf)
+ < 0)
return -1;
}
ret = UI_process(ui);
