On Thu, Aug 09, 2018 at 08:45:03AM +0200, Theo Buehler wrote:
> 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.
Right after sending I noticed that I had the checks in the wrong order.
Sorry about that.
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 07:08:20 -0000
@@ -101,17 +101,20 @@ EVP_read_pw_string_min(char *buf, int mi
char buff[BUFSIZ];
UI *ui;
+ if (len > BUFSIZ)
+ len = BUFSIZ;
+ if (min < 0 || len - 1 < min)
+ return -1;
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);
