Bug 192277 (EN-14:11) - [EN-14:11] crypt(3) regression
Summary: [EN-14:11] crypt(3) regression
Status: Closed FIXED
Alias: EN-14:11
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: 9.3-RELEASE
Hardware: Any Any
: --- Affects Many People
Assignee: Dag-Erling Smørgrav
URL:
Keywords:
: 189958 192431 (view as bug list)
Depends on:
Blocks:
 
Reported: 2014-07-30 18:37 UTC by lampa
Modified: 2015-02-24 19:19 UTC (History)
5 users (show)

See Also:


Attachments
Change DES back to the default password format (788 bytes, patch)
2014-07-31 00:08 UTC, Xin LI
no flags Details | Diff
Revert to DES as the default (1.59 KB, patch)
2014-10-07 14:50 UTC, Dag-Erling Smørgrav
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description lampa 2014-07-30 18:37:55 UTC
#include <stdio.h>
#include <unistd.h>

int main()
{
        char *p;

        p = crypt("12345678", "1234");
        printf("hash = %s\n", p);
}

The result is sha512 hash = $6$1234$YlCaDQ/VIZKWwIo2tmk5UTOuoVbHSCBk8.4kcEXuwEVM2CDbAJOGIIPDK5DYedDT0Es/Rj2CSoD8LCpLhu8gy1

According man page, it should return DES format hash. This is serious
regression, it can result in buffer overflow in old applications that don't expect anything else (I have been beaten by one such). IMHO historically incompatible behavior can happen only in Modular case. Both Modular and Traditional format salt should result with DES format hash in default case (without crypt_set_format) exactly like man page says:

man 3 crypt
   Traditional crypt:
     The algorithm used will depend upon whether crypt_set_format() has been
     called and whether a global default format has been specified.  Unless a
     global default has been specified or crypt_set_format() has set the for-
     mat to something else, the built-in default format is used.  This is cur-
     rently DES if it is available, or MD5 if not.
Comment 1 Xin LI freebsd_committer freebsd_triage 2014-07-31 00:08:44 UTC
Created attachment 145169 [details]
Change DES back to the default password format
Comment 2 Xin LI freebsd_committer freebsd_triage 2014-07-31 00:36:56 UTC
IMHO it's a bug with your application as it blindly assumes that crypt(3) returns a constant length string and copies it to a buffer with that.

However, on the other hand this can be seen as a slight ABI breakage, which should not happen in -STABLE branches.  I think the attached patch should be applied against stable/9.

We do want to change the default algorithm, for -HEAD, though.  It looks like the manual page should be updated by the way.

Assigning to des@ for his decision.
Comment 3 Eitan Adler freebsd_committer freebsd_triage 2014-07-31 00:51:06 UTC
IMHO changing a global default is not ABI breakage. If there is a user tweak able setting which changes the output of crypt, the application must be capable of dealing with it.
Comment 4 Xin LI freebsd_committer freebsd_triage 2014-07-31 06:13:17 UTC
(In reply to Eitan Adler from comment #3)
> IMHO changing a global default is not ABI breakage. If there is a user tweak
> able setting which changes the output of crypt, the application must be
> capable of dealing with it.

Well, a side effect of that change is the output length have been changed.

No, I'm not saying the application is not at fault, and by all means that *should* be fixed, however, I believe this still counts as an ABI change, subtle -- maybe, not our fault -- true, but that's still a change.

For -STABLE branches, our promise is not to break ABI until next release unless there is very good reasons, by not breaking ABI, the user can reasonably expect applications used to work on previous -STABLE release work without problem on newer -STABLE release without change, if it's of the same major version.
Comment 5 papowell 2014-08-06 12:17:39 UTC
The crypt(3) C library routine is used by Perl/PHP.  This change has broken a large number of other things.  While there is no argument about the 'strength' of the hash/encryption,  the documentation states that it will behave in a specific manner,  and as far as I can tell with a quick scan of the FreeBSD archives, there has been no disussion of the impact of making this change.

I am surprised that one of the core functions behavior (crypt) was modified and this was not put into the FreeBSD 9.3 release notes.

I would strongly suggest that you publicize this and send out instructions on how to a) make the 'des' encryption the default b) recompile everything that uses crypt and is statically linked c) warn Perl and PHP users about this impact so they can update their Perl and PHP.  (Note, I might be wrong about PHP, but I checked Perl and it uses the -lcrypt library).
Comment 6 Dag-Erling Smørgrav freebsd_committer freebsd_triage 2014-08-06 13:14:01 UTC
I concede that the man page should have been updated (I thought I'd taken care of it), but any application that copies the hash into a buffer without either checking its length first or using strlcpy() and checking the return value is broken.
Comment 7 lampa 2014-08-06 13:57:33 UTC
Nobody mentioned the worst thing - you cannot portably force the traditional behaviour. If you present the old format salt, you expect the old des hash. If you want some new hash, you can always put $6$ in salt to get such result. I really don't understand why this change was done and how it was accepted. This doesn't have anything common with system default password encryption, this is realised using /etc/login.conf and applications like passwd, etc. The crypt_set_format() function is a FreeBSD invention and you cannot use it portably. For me, this change is like changing printf() to generate always UTF-16 characters without any way to force it portably to other charater set :-(
Comment 8 Dirk Meyer freebsd_committer freebsd_triage 2014-09-06 15:44:36 UTC
Regressions in existing applications with unpatched FreeBSD 9.3 RELEASE

1)
SQL-Queries with ENCRYPT() in mysql*-server fail.

2)
Authentication in in OTRS (Perl crypt())


Please add this issue to Errata:
https://www.freebsd.org/releases/9.3R/errata.html
Comment 9 Dag-Erling Smørgrav freebsd_committer freebsd_triage 2014-10-07 14:50:47 UTC
Created attachment 148061 [details]
Revert to DES as the default

Moving DES to the top of the list breaks the algorithm guessing logic in crypt(3).  The attached patch instead sets the default algorithm to the last one in the list and moves SHA512 down so that it becomes the default if libcrypt is compiled without -DHAS_DES.
Comment 10 commit-hook freebsd_committer freebsd_triage 2014-10-09 16:45:34 UTC
A commit references this bug:

Author: des
Date: Thu Oct  9 16:45:12 UTC 2014
New revision: 272830
URL: https://svnweb.freebsd.org/changeset/base/272830

Log:
  Change the hardcoded default back from SHA512 to DES.

  PR:		192277
  MFC after:	3 days

Changes:
  head/lib/libcrypt/crypt.c
Comment 11 Dag-Erling Smørgrav freebsd_committer freebsd_triage 2014-10-16 09:14:15 UTC
*** Bug 192431 has been marked as a duplicate of this bug. ***
Comment 12 commit-hook freebsd_committer freebsd_triage 2014-10-21 21:10:35 UTC
A commit references this bug:

Author: delphij
Date: Tue Oct 21 21:09:55 UTC 2014
New revision: 273425
URL: https://svnweb.freebsd.org/changeset/base/273425

Log:
  MFC r272830 (des):

  Change the hardcoded default back from SHA512 to DES.

  This will become EN-14:11.crypt.

  PR:		192277

Changes:
_U  stable/9/lib/libcrypt/
  stable/9/lib/libcrypt/crypt.c
Comment 13 Xin LI freebsd_committer freebsd_triage 2014-10-22 00:13:30 UTC
Fix committed to all affected stable branches and 9.3 errata branch (the EN notice will be out soon).
Comment 14 Gavin Atkinson freebsd_committer freebsd_triage 2015-02-24 19:19:02 UTC
*** Bug 189958 has been marked as a duplicate of this bug. ***