libressl: crash in DES_fcrypt

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

libressl: crash in DES_fcrypt

Jan Engelhardt-2

libressl-2.6.2 is susceptible to an out-of-bounds read:

#include <openssl/des.h>
int main(void) {
        char salt[3] = {0xf8, 0xd0, 0x00};
        char out[32];
        DES_fcrypt("foo", salt, out);
}

Place in libressl's fcrypt.c:
        x=ret[0]=((salt[0] == '\0')?'A':salt[0]);
        Eswap0=con_salt[x]<<2;  // boom

ASM: => 0x00007ffff77a6fa8 <+56>:    movzbl (%rcx,%rdx,1),%ebp
        rcx = con_salt
        rdx = 0xfffffff8


Because salt[0] is -8, x will be 0xfffffff8 due to
type promotion and conversion. con_salt[0xfffffff8]
is then evaluted, which bombs out.

openssl 1.1.x has it fixed (but 1.0.2l does not!) - their commit
seems to be 6493e4801e9edbe1ad1e256d4ce9cd55c8aa2242 in
https://github.com/openssl/openssl .

Reply | Threaded
Open this post in threaded view
|

Re: libressl: crash in DES_fcrypt

Brent Cook
Thank you Jan. This is a good thing to fix, but I had a hard time envisioning a security issue with it. Will see about backporting it though.

Regards
 - Brent

> On Oct 26, 2017, at 6:50 PM, Jan Engelhardt <[hidden email]> wrote:
>
>
> libressl-2.6.2 is susceptible to an out-of-bounds read:
>
> #include <openssl/des.h>
> int main(void) {
>        char salt[3] = {0xf8, 0xd0, 0x00};
>        char out[32];
>        DES_fcrypt("foo", salt, out);
> }
>
> Place in libressl's fcrypt.c:
>        x=ret[0]=((salt[0] == '\0')?'A':salt[0]);
>        Eswap0=con_salt[x]<<2;  // boom
>
> ASM: => 0x00007ffff77a6fa8 <+56>:    movzbl (%rcx,%rdx,1),%ebp
> rcx = con_salt
> rdx = 0xfffffff8
>
>
> Because salt[0] is -8, x will be 0xfffffff8 due to
> type promotion and conversion. con_salt[0xfffffff8]
> is then evaluted, which bombs out.
>
> openssl 1.1.x has it fixed (but 1.0.2l does not!) - their commit
> seems to be 6493e4801e9edbe1ad1e256d4ce9cd55c8aa2242 in
> https://github.com/openssl/openssl .
>

Reply | Threaded
Open this post in threaded view
|

Re: libressl: crash in DES_fcrypt

Alexander Bluhm
In reply to this post by Jan Engelhardt-2
On Fri, Oct 27, 2017 at 01:50:26AM +0200, Jan Engelhardt wrote:
> #include <openssl/des.h>
> int main(void) {
>         char salt[3] = {0xf8, 0xd0, 0x00};
>         char out[32];
>         DES_fcrypt("foo", salt, out);
> }

This program produces a Segmentation fault in OpenBSD current.

> openssl 1.1.x has it fixed (but 1.0.2l does not!) - their commit
> seems to be 6493e4801e9edbe1ad1e256d4ce9cd55c8aa2242 in
> https://github.com/openssl/openssl .

Their fix changes the semantics of DES_crypt() and DES_fcrypt().
They may fail and return NULL now.  This was never the case before
so we may expect that programs do not check it.  With DES_fcrypt()
it is very likely that some uninitilaized content of the return
string is used.

So I have extended the workaround that was there already.  Read the
comment above the fix.  If the salt does not consist of two ascii
characters, replace it with "AA".  This gives a safe result in all
cases.

ok?

bluhm

Index: lib/libcrypto/des/fcrypt.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/lib/libcrypto/des/fcrypt.c,v
retrieving revision 1.12
diff -u -p -r1.12 fcrypt.c
--- lib/libcrypto/des/fcrypt.c 26 Dec 2016 21:30:10 -0000 1.12
+++ lib/libcrypto/des/fcrypt.c 5 Dec 2017 16:03:57 -0000
@@ -78,9 +78,15 @@ char *DES_fcrypt(const char *buf, const
  * crypt to "*".  This was found when replacing the crypt in
  * our shared libraries.  People found that the disabled
  * accounts effectively had no passwd :-(. */
- x=ret[0]=((salt[0] == '\0')?'A':salt[0]);
+ x = salt[0];
+ if (x == 0 || x >= sizeof(con_salt))
+ x = 'A';
+ ret[0] = x;
  Eswap0=con_salt[x]<<2;
- x=ret[1]=((salt[1] == '\0')?'A':salt[1]);
+ x = (salt[0] == '\0') ? 'A' : salt[1];
+ if (x == 0 || x >= sizeof(con_salt))
+ x = 'A';
+ ret[1] = x;
  Eswap1=con_salt[x]<<6;
 /* EAY
 r=strlen(buf);
Index: regress/lib/libcrypto/des/destest.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/regress/lib/libcrypto/des/destest.c,v
retrieving revision 1.3
diff -u -p -r1.3 destest.c
--- regress/lib/libcrypto/des/destest.c 30 Oct 2015 15:58:40 -0000 1.3
+++ regress/lib/libcrypto/des/destest.c 5 Dec 2017 16:02:18 -0000
@@ -749,18 +749,38 @@ plain[8+4], plain[8+5], plain[8+6], plai
  }
  printf("\n");
  printf("fast crypt test ");
- str=crypt("testing","ef");
+ str=DES_crypt("testing","ef");
  if (strcmp("efGnQx2725bI2",str) != 0)
  {
  printf("fast crypt error, %s should be efGnQx2725bI2\n",str);
  err=1;
  }
- str=crypt("bca76;23","yA");
+ str=DES_crypt("bca76;23","yA");
  if (strcmp("yA1Rp/1hZXIJk",str) != 0)
  {
  printf("fast crypt error, %s should be yA1Rp/1hZXIJk\n",str);
  err=1;
  }
+ str = DES_crypt("testing", "\202B");
+ if (strncmp("AB",str,2) != 0) {
+ printf("salt %s first non ascii not replaced with A\n", str);
+ err = 1;
+ }
+ str = DES_crypt("testing", "B\202");
+ if (strncmp("BA",str,2) != 0) {
+ printf("salt %s second non ascii not replaced with A\n", str);
+ err = 1;
+ }
+ str = DES_crypt("testing", "\0B");
+ if (strncmp("AA",str,2) != 0) {
+ printf("salt %s first NUL not replaced with AA\n", str);
+ err = 1;
+ }
+ str = DES_crypt("testing", "B");
+ if (strncmp("BA",str,2) != 0) {
+ printf("salt %s second NUL not replaced with A\n", str);
+ err = 1;
+ }
  printf("\n");
  return(err);
  }

Reply | Threaded
Open this post in threaded view
|

Re: libressl: crash in DES_fcrypt

Alexander Bluhm
Hi,

I would like to commit this fix.  I tried to avoid the crash in
libcrypto with least possible impact for the DES_fcrypt() API.

ok?

bluhm

On Tue, Dec 05, 2017 at 05:20:49PM +0100, Alexander Bluhm wrote:

> On Fri, Oct 27, 2017 at 01:50:26AM +0200, Jan Engelhardt wrote:
> > #include <openssl/des.h>
> > int main(void) {
> >         char salt[3] = {0xf8, 0xd0, 0x00};
> >         char out[32];
> >         DES_fcrypt("foo", salt, out);
> > }
>
> This program produces a Segmentation fault in OpenBSD current.
>
> > openssl 1.1.x has it fixed (but 1.0.2l does not!) - their commit
> > seems to be 6493e4801e9edbe1ad1e256d4ce9cd55c8aa2242 in
> > https://github.com/openssl/openssl .
>
> Their fix changes the semantics of DES_crypt() and DES_fcrypt().
> They may fail and return NULL now.  This was never the case before
> so we may expect that programs do not check it.  With DES_fcrypt()
> it is very likely that some uninitilaized content of the return
> string is used.
>
> So I have extended the workaround that was there already.  Read the
> comment above the fix.  If the salt does not consist of two ascii
> characters, replace it with "AA".  This gives a safe result in all
> cases.
>
> ok?
>
> bluhm
>
> Index: lib/libcrypto/des/fcrypt.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/lib/libcrypto/des/fcrypt.c,v
> retrieving revision 1.12
> diff -u -p -r1.12 fcrypt.c
> --- lib/libcrypto/des/fcrypt.c 26 Dec 2016 21:30:10 -0000 1.12
> +++ lib/libcrypto/des/fcrypt.c 5 Dec 2017 16:03:57 -0000
> @@ -78,9 +78,15 @@ char *DES_fcrypt(const char *buf, const
>   * crypt to "*".  This was found when replacing the crypt in
>   * our shared libraries.  People found that the disabled
>   * accounts effectively had no passwd :-(. */
> - x=ret[0]=((salt[0] == '\0')?'A':salt[0]);
> + x = salt[0];
> + if (x == 0 || x >= sizeof(con_salt))
> + x = 'A';
> + ret[0] = x;
>   Eswap0=con_salt[x]<<2;
> - x=ret[1]=((salt[1] == '\0')?'A':salt[1]);
> + x = (salt[0] == '\0') ? 'A' : salt[1];
> + if (x == 0 || x >= sizeof(con_salt))
> + x = 'A';
> + ret[1] = x;
>   Eswap1=con_salt[x]<<6;
>  /* EAY
>  r=strlen(buf);
> Index: regress/lib/libcrypto/des/destest.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/regress/lib/libcrypto/des/destest.c,v
> retrieving revision 1.3
> diff -u -p -r1.3 destest.c
> --- regress/lib/libcrypto/des/destest.c 30 Oct 2015 15:58:40 -0000 1.3
> +++ regress/lib/libcrypto/des/destest.c 5 Dec 2017 16:02:18 -0000
> @@ -749,18 +749,38 @@ plain[8+4], plain[8+5], plain[8+6], plai
>   }
>   printf("\n");
>   printf("fast crypt test ");
> - str=crypt("testing","ef");
> + str=DES_crypt("testing","ef");
>   if (strcmp("efGnQx2725bI2",str) != 0)
>   {
>   printf("fast crypt error, %s should be efGnQx2725bI2\n",str);
>   err=1;
>   }
> - str=crypt("bca76;23","yA");
> + str=DES_crypt("bca76;23","yA");
>   if (strcmp("yA1Rp/1hZXIJk",str) != 0)
>   {
>   printf("fast crypt error, %s should be yA1Rp/1hZXIJk\n",str);
>   err=1;
>   }
> + str = DES_crypt("testing", "\202B");
> + if (strncmp("AB",str,2) != 0) {
> + printf("salt %s first non ascii not replaced with A\n", str);
> + err = 1;
> + }
> + str = DES_crypt("testing", "B\202");
> + if (strncmp("BA",str,2) != 0) {
> + printf("salt %s second non ascii not replaced with A\n", str);
> + err = 1;
> + }
> + str = DES_crypt("testing", "\0B");
> + if (strncmp("AA",str,2) != 0) {
> + printf("salt %s first NUL not replaced with AA\n", str);
> + err = 1;
> + }
> + str = DES_crypt("testing", "B");
> + if (strncmp("BA",str,2) != 0) {
> + printf("salt %s second NUL not replaced with A\n", str);
> + err = 1;
> + }
>   printf("\n");
>   return(err);
>   }

Reply | Threaded
Open this post in threaded view
|

Re: libressl: crash in DES_fcrypt

Theo de Raadt-2
I still don't understand.

This feels like fail-open.

> I would like to commit this fix.  I tried to avoid the crash in
> libcrypto with least possible impact for the DES_fcrypt() API.
>
> ok?
>
> bluhm
>
> On Tue, Dec 05, 2017 at 05:20:49PM +0100, Alexander Bluhm wrote:
> > On Fri, Oct 27, 2017 at 01:50:26AM +0200, Jan Engelhardt wrote:
> > > #include <openssl/des.h>
> > > int main(void) {
> > >         char salt[3] = {0xf8, 0xd0, 0x00};
> > >         char out[32];
> > >         DES_fcrypt("foo", salt, out);
> > > }
> >
> > This program produces a Segmentation fault in OpenBSD current.
> >
> > > openssl 1.1.x has it fixed (but 1.0.2l does not!) - their commit
> > > seems to be 6493e4801e9edbe1ad1e256d4ce9cd55c8aa2242 in
> > > https://github.com/openssl/openssl .
> >
> > Their fix changes the semantics of DES_crypt() and DES_fcrypt().
> > They may fail and return NULL now.  This was never the case before
> > so we may expect that programs do not check it.  With DES_fcrypt()
> > it is very likely that some uninitilaized content of the return
> > string is used.
> >
> > So I have extended the workaround that was there already.  Read the
> > comment above the fix.  If the salt does not consist of two ascii
> > characters, replace it with "AA".  This gives a safe result in all
> > cases.
> >
> > ok?
> >
> > bluhm
> >
> > Index: lib/libcrypto/des/fcrypt.c
> > ===================================================================
> > RCS file: /data/mirror/openbsd/cvs/src/lib/libcrypto/des/fcrypt.c,v
> > retrieving revision 1.12
> > diff -u -p -r1.12 fcrypt.c
> > --- lib/libcrypto/des/fcrypt.c 26 Dec 2016 21:30:10 -0000 1.12
> > +++ lib/libcrypto/des/fcrypt.c 5 Dec 2017 16:03:57 -0000
> > @@ -78,9 +78,15 @@ char *DES_fcrypt(const char *buf, const
> >   * crypt to "*".  This was found when replacing the crypt in
> >   * our shared libraries.  People found that the disabled
> >   * accounts effectively had no passwd :-(. */
> > - x=ret[0]=((salt[0] == '\0')?'A':salt[0]);
> > + x = salt[0];
> > + if (x == 0 || x >= sizeof(con_salt))
> > + x = 'A';
> > + ret[0] = x;
> >   Eswap0=con_salt[x]<<2;
> > - x=ret[1]=((salt[1] == '\0')?'A':salt[1]);
> > + x = (salt[0] == '\0') ? 'A' : salt[1];
> > + if (x == 0 || x >= sizeof(con_salt))
> > + x = 'A';
> > + ret[1] = x;
> >   Eswap1=con_salt[x]<<6;
> >  /* EAY
> >  r=strlen(buf);
> > Index: regress/lib/libcrypto/des/destest.c
> > ===================================================================
> > RCS file: /data/mirror/openbsd/cvs/src/regress/lib/libcrypto/des/destest.c,v
> > retrieving revision 1.3
> > diff -u -p -r1.3 destest.c
> > --- regress/lib/libcrypto/des/destest.c 30 Oct 2015 15:58:40 -0000 1.3
> > +++ regress/lib/libcrypto/des/destest.c 5 Dec 2017 16:02:18 -0000
> > @@ -749,18 +749,38 @@ plain[8+4], plain[8+5], plain[8+6], plai
> >   }
> >   printf("\n");
> >   printf("fast crypt test ");
> > - str=crypt("testing","ef");
> > + str=DES_crypt("testing","ef");
> >   if (strcmp("efGnQx2725bI2",str) != 0)
> >   {
> >   printf("fast crypt error, %s should be efGnQx2725bI2\n",str);
> >   err=1;
> >   }
> > - str=crypt("bca76;23","yA");
> > + str=DES_crypt("bca76;23","yA");
> >   if (strcmp("yA1Rp/1hZXIJk",str) != 0)
> >   {
> >   printf("fast crypt error, %s should be yA1Rp/1hZXIJk\n",str);
> >   err=1;
> >   }
> > + str = DES_crypt("testing", "\202B");
> > + if (strncmp("AB",str,2) != 0) {
> > + printf("salt %s first non ascii not replaced with A\n", str);
> > + err = 1;
> > + }
> > + str = DES_crypt("testing", "B\202");
> > + if (strncmp("BA",str,2) != 0) {
> > + printf("salt %s second non ascii not replaced with A\n", str);
> > + err = 1;
> > + }
> > + str = DES_crypt("testing", "\0B");
> > + if (strncmp("AA",str,2) != 0) {
> > + printf("salt %s first NUL not replaced with AA\n", str);
> > + err = 1;
> > + }
> > + str = DES_crypt("testing", "B");
> > + if (strncmp("BA",str,2) != 0) {
> > + printf("salt %s second NUL not replaced with A\n", str);
> > + err = 1;
> > + }
> >   printf("\n");
> >   return(err);
> >   }
>

Reply | Threaded
Open this post in threaded view
|

Re: libressl: crash in DES_fcrypt

Bob Beck-3
why AA?  why not just choose two random ascii salt chars at that point?  or
since this is effectively a failure case encrypt a random ascii salt and
random string?

using AA will produce a usable result based on the original string.
 encrypting a random string with a random salt means the failure return
wont be accidentally used

On Wed, Dec 13, 2017 at 06:51 Theo de Raadt <[hidden email]> wrote:

> I still don't understand.
>
> This feels like fail-open.
>
> > I would like to commit this fix.  I tried to avoid the crash in
> > libcrypto with least possible impact for the DES_fcrypt() API.
> >
> > ok?
> >
> > bluhm
> >
> > On Tue, Dec 05, 2017 at 05:20:49PM +0100, Alexander Bluhm wrote:
> > > On Fri, Oct 27, 2017 at 01:50:26AM +0200, Jan Engelhardt wrote:
> > > > #include <openssl/des.h>
> > > > int main(void) {
> > > >         char salt[3] = {0xf8, 0xd0, 0x00};
> > > >         char out[32];
> > > >         DES_fcrypt("foo", salt, out);
> > > > }
> > >
> > > This program produces a Segmentation fault in OpenBSD current.
> > >
> > > > openssl 1.1.x has it fixed (but 1.0.2l does not!) - their commit
> > > > seems to be 6493e4801e9edbe1ad1e256d4ce9cd55c8aa2242 in
> > > > https://github.com/openssl/openssl .
> > >
> > > Their fix changes the semantics of DES_crypt() and DES_fcrypt().
> > > They may fail and return NULL now.  This was never the case before
> > > so we may expect that programs do not check it.  With DES_fcrypt()
> > > it is very likely that some uninitilaized content of the return
> > > string is used.
> > >
> > > So I have extended the workaround that was there already.  Read the
> > > comment above the fix.  If the salt does not consist of two ascii
> > > characters, replace it with "AA".  This gives a safe result in all
> > > cases.
> > >
> > > ok?
> > >
> > > bluhm
> > >
> > > Index: lib/libcrypto/des/fcrypt.c
> > > ===================================================================
> > > RCS file: /data/mirror/openbsd/cvs/src/lib/libcrypto/des/fcrypt.c,v
> > > retrieving revision 1.12
> > > diff -u -p -r1.12 fcrypt.c
> > > --- lib/libcrypto/des/fcrypt.c      26 Dec 2016 21:30:10 -0000
> 1.12
> > > +++ lib/libcrypto/des/fcrypt.c      5 Dec 2017 16:03:57 -0000
> > > @@ -78,9 +78,15 @@ char *DES_fcrypt(const char *buf, const
> > >      * crypt to "*".  This was found when replacing the crypt in
> > >      * our shared libraries.  People found that the disabled
> > >      * accounts effectively had no passwd :-(. */
> > > -   x=ret[0]=((salt[0] == '\0')?'A':salt[0]);
> > > +   x = salt[0];
> > > +   if (x == 0 || x >= sizeof(con_salt))
> > > +           x = 'A';
> > > +   ret[0] = x;
> > >     Eswap0=con_salt[x]<<2;
> > > -   x=ret[1]=((salt[1] == '\0')?'A':salt[1]);
> > > +   x = (salt[0] == '\0') ? 'A' : salt[1];
> > > +   if (x == 0 || x >= sizeof(con_salt))
> > > +           x = 'A';
> > > +   ret[1] = x;
> > >     Eswap1=con_salt[x]<<6;
> > >  /* EAY
> > >  r=strlen(buf);
> > > Index: regress/lib/libcrypto/des/destest.c
> > > ===================================================================
> > > RCS file:
> /data/mirror/openbsd/cvs/src/regress/lib/libcrypto/des/destest.c,v
> > > retrieving revision 1.3
> > > diff -u -p -r1.3 destest.c
> > > --- regress/lib/libcrypto/des/destest.c     30 Oct 2015 15:58:40
> -0000      1.3
> > > +++ regress/lib/libcrypto/des/destest.c     5 Dec 2017 16:02:18 -0000
> > > @@ -749,18 +749,38 @@ plain[8+4], plain[8+5], plain[8+6], plai
> > >             }
> > >     printf("\n");
> > >     printf("fast crypt test ");
> > > -   str=crypt("testing","ef");
> > > +   str=DES_crypt("testing","ef");
> > >     if (strcmp("efGnQx2725bI2",str) != 0)
> > >             {
> > >             printf("fast crypt error, %s should be
> efGnQx2725bI2\n",str);
> > >             err=1;
> > >             }
> > > -   str=crypt("bca76;23","yA");
> > > +   str=DES_crypt("bca76;23","yA");
> > >     if (strcmp("yA1Rp/1hZXIJk",str) != 0)
> > >             {
> > >             printf("fast crypt error, %s should be
> yA1Rp/1hZXIJk\n",str);
> > >             err=1;
> > >             }
> > > +   str = DES_crypt("testing", "\202B");
> > > +   if (strncmp("AB",str,2) != 0) {
> > > +           printf("salt %s first non ascii not replaced with A\n",
> str);
> > > +           err = 1;
> > > +   }
> > > +   str = DES_crypt("testing", "B\202");
> > > +   if (strncmp("BA",str,2) != 0) {
> > > +           printf("salt %s second non ascii not replaced with A\n",
> str);
> > > +           err = 1;
> > > +   }
> > > +   str = DES_crypt("testing", "\0B");
> > > +   if (strncmp("AA",str,2) != 0) {
> > > +           printf("salt %s first NUL not replaced with AA\n", str);
> > > +           err = 1;
> > > +   }
> > > +   str = DES_crypt("testing", "B");
> > > +   if (strncmp("BA",str,2) != 0) {
> > > +           printf("salt %s second NUL not replaced with A\n", str);
> > > +           err = 1;
> > > +   }
> > >     printf("\n");
> > >     return(err);
> > >     }
> >
>
>