openssl(1) passwd chunck canary corrupted

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

openssl(1) passwd chunck canary corrupted

Ricardo Mestre-2
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?

Index: passwd.c
===================================================================
RCS file: /cvs/src/usr.bin/openssl/passwd.c,v
retrieving revision 1.9
diff -u -p -u -r1.9 passwd.c
--- passwd.c 7 Feb 2018 05:47:55 -0000 1.9
+++ passwd.c 8 Aug 2018 13:00:40 -0000
@@ -213,7 +213,7 @@ passwd_main(int argc, char **argv)
 
  passwd_malloc_size = pw_maxlen + 2;
  /* longer than necessary so that we can warn about truncation */
- passwd = passwd_malloc = malloc(passwd_malloc_size);
+ passwd = passwd_malloc = malloc(passwd_malloc_size + 1);
  if (passwd_malloc == NULL)
  goto err;
  }

Reply | Threaded
Open this post in threaded view
|

Re: openssl(1) passwd chunck canary corrupted

Theo Buehler-5
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);

Reply | Threaded
Open this post in threaded view
|

Re: openssl(1) passwd chunck canary corrupted

Theo Buehler-5
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);

Reply | Threaded
Open this post in threaded view
|

Re: openssl(1) passwd chunck canary corrupted

Theo Buehler-5
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);