Re: libressl: crash in DES_fcrypt
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 Raadtwrote: > 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 > > > > 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 - > 1.12 > > > +++ lib/libcrypto/des/fcrypt.c 5 Dec 2017 16:03:57 - > > > @@ -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 > - 1.3 > > > +++ regress/lib/libcrypto/des/destest.c 5 Dec 2017 16:02:18 - > > > @@ -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); > > > } > > > >
Re: libressl: crash in DES_fcrypt
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 > > > 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 - 1.12 > > +++ lib/libcrypto/des/fcrypt.c 5 Dec 2017 16:03:57 - > > @@ -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 - > > 1.3 > > +++ regress/lib/libcrypto/des/destest.c 5 Dec 2017 16:02:18 - > > @@ -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); > > } >
Re: libressl: crash in DES_fcrypt
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 > > 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.c26 Dec 2016 21:30:10 - 1.12 > +++ lib/libcrypto/des/fcrypt.c5 Dec 2017 16:03:57 - > @@ -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 - > 1.3 > +++ regress/lib/libcrypto/des/destest.c 5 Dec 2017 16:02:18 - > @@ -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); > }
Re: libressl: crash in DES_fcrypt
On Fri, Oct 27, 2017 at 01:50:26AM +0200, Jan Engelhardt wrote: > #include > 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 - 1.12 +++ lib/libcrypto/des/fcrypt.c 5 Dec 2017 16:03:57 - @@ -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 - 1.3 +++ regress/lib/libcrypto/des/destest.c 5 Dec 2017 16:02:18 - @@ -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); }
Re: libressl: crash in DES_fcrypt
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 Engelhardtwrote: > > > libressl-2.6.2 is susceptible to an out-of-bounds read: > > #include > 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: => 0x777a6fa8 <+56>:movzbl (%rcx,%rdx,1),%ebp > rcx = con_salt > rdx = 0xfff8 > > > Because salt[0] is -8, x will be 0xfff8 due to > type promotion and conversion. con_salt[0xfff8] > 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 . >
libressl: crash in DES_fcrypt
libressl-2.6.2 is susceptible to an out-of-bounds read: #include 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:=> 0x777a6fa8 <+56>:movzbl (%rcx,%rdx,1),%ebp rcx = con_salt rdx = 0xfff8 Because salt[0] is -8, x will be 0xfff8 due to type promotion and conversion. con_salt[0xfff8] 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 .