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 <dera...@openbsd.org> 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);
> > > }
> >
>
>
