libxcrypt accesses the second character of the salt if the first one is an asterisk.

[t184256]

This test, adapted from glibc's badsalttest, shows that libxcrypt accesses the second character of the salt if the first one is invalid.

The test below segfaults with libxcrypt (4.4.8) and returns 0 with glibc (glibc-2.17-292.el7).

Test

/* Test program for bad DES salt detection in crypt.
   Copyright (C) 2012 Free Software Foundation, Inc.
   This file is part of the GNU C Library.

   The GNU C Library is free software; you can redistribute it and/or
   modify it under the terms of the GNU Lesser General Public
   License as published by the Free Software Foundation; either
   version 2.1 of the License, or (at your option) any later version.

   The GNU C Library is distributed in the hope that it will be useful,
   but WITHOUT ANY WARRANTY; without even the implied warranty of
   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
   Lesser General Public License for more details.

   You should have received a copy of the GNU Lesser General Public
   License along with the GNU C Library; if not, see
   <http://www.gnu.org/licenses/>.  */

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <sys/mman.h>

#include <crypt.h>

int
main (void)
{
  size_t pagesize = (size_t) sysconf (_SC_PAGESIZE);
  char *page;

  char* phrase = "end of page";
  char* setting;

  /* Check that crypt won't look at the second character if the first
     one is invalid.  */
  page = mmap (NULL, pagesize * 2, PROT_READ | PROT_WRITE,
	       MAP_PRIVATE | MAP_ANON, -1, 0);
  if (page == MAP_FAILED)
    {
      perror ("mmap");
      return 1;
    }
  else
    {
      if (mmap (page + pagesize, pagesize, 0,
		MAP_PRIVATE | MAP_ANON | MAP_FIXED,
		-1, 0) != page + pagesize)
	perror ("mmap 2");
      page[pagesize - 1] = '*';
      setting = &page[pagesize - 1];
    }


    if (crypt (phrase, setting)) {
	  printf ("%s: crypt returned non-NULL with salt \"%s\"\n",
	  phrase, setting);
          return 1;
    }

  return 0;
}

[besser82]

This example hits a single corner case, as libxcrypt uses the asterisk (*) character as a failure token. This failure toke may or may not be the return value of the crypt and crypt_r functions, depending on the build-time configuration.

The make_failure_token function needs to access the second character of the setting (salt), if the first one is an asterisk, to ensure the possibly returned token will not be identical to the actual setting.

page[pagesize - 1] = '@'; for instace, will make the test pass.

If you use any other invalid character aside from an asterisk, the behaviour is the same as in glibc's libcrypt.

[t184256]

Hm, interesting, thanks for a clarification!

I can't find whether failure tokens are documented anywhere except for the source code comments. If they are not, could you maybe reuse this bug as a TODO to document them?

Do I understand correctly, that if one wants to use libxcrypt as a drop-in replacement to glibc crypt, it should be compiled without failure tokens?

[solardiz]

The behavior described here isn't a bug. It would have been wrong to access a character after NUL, but it is no problem accessing one after a non-NUL. crypt is defined to accept a NUL-terminated string as setting, and the reported behavior is consistent with proper processing of such string.

The corner case @besser82 mentions is irrelevant to such reasoning and thus unnecessary to bring up in this context. Moreover, we see that even in this corner case the behavior is correct.

I think we should just close this issue, with no action needed.

[solardiz]

Is there any reason why this issue is still not closed?

[zackw]

I'm inclined to agree with @solardiz that this should be closed with no action. He's correct that, in general, C library functions that take "string" arguments should be expected to crash if they are given a pointer to a memory area that isn't NUL-terminated.

Also, the crypt.3 manpage doesn't use the term "failure token", but the generation of failure tokens is documented:

Upon error, crypt_r, crypt_rn, and crypt_ra write an invalid hashed
passphrase to the output field of their data argument, and crypt writes
an invalid hash to its static storage area. This string will be shorter
than 13 characters, will begin with a ‘*’, and will not compare equal to
setting.
Upon error, crypt_rn and crypt_ra return a null pointer. crypt_r and
crypt may also return a null pointer, or they may return a pointer to the
invalid hash, depending on how libcrypt was configured. (The option to
return the invalid hash is for compatibility with old applications that
assume that crypt cannot return a null pointer. See PORTABILITY NOTES
below.)

The only way to implement "this string ... will begin with a ‘*’, and will not compare equal to setting" is to access the second character of setting when its first character is an asterisk. And because the invalid hashed passphrase is written to caller-accessible storage whether or not libxcrypt was configured with --enable-failure-tokens, configuring with --disable-failure-tokens will not make the behavior in this edge case match glibc's.

I am a little curious why this test was added to glibc in the first place, but it was added eight years ago, and the person who wrote the test isn't an active maintainer anymore, so I don't particularly want to bother him about it.

[zackw]

Since nobody's been terribly active in libxcrypt development lately, I am going to leave this open for another week in case someone wants to argue with the logic above.

[t184256]

I'd like to preempt that I have no objections to closing this. After reading the explanations above, I see no issue with libxcrypt behavior or claims here.