NigelCunningham / pam-MySQL

PAM MySQL
GNU General Public License v2.0
111 stars 61 forks source link

pam_mysql_check_passwd() returning 6 #65

Open 09173732546 opened 4 years ago

09173732546 commented 4 years ago

hi there,

i used the default libpam-mysql and this manual install libpam-mysql, but im always getting this on /var/log/auth.log

i have inserted "testuser" on the database, i also properly entered the details on /etc/libpam-mysql.conf

i tried crypt=0 and crypt=3 (md5) still same, and i think the main problem is its telling that the username is invalid even its on the database..

May 10 18:47:10 debian sshd[3737]: Invalid user testuser from 10.0.2.2 port 22693 May 10 18:47:14 debian sshd[3737]: pam_mysql - option verbose is set to "1" May 10 18:47:14 debian sshd[3737]: pam_mysql - option log.enabled is set to "false" May 10 18:47:14 debian sshd[3737]: pam_mysql - option log.table is set to "log" May 10 18:47:14 debian sshd[3737]: pam_mysql - option log.message_column is set to "message" May 10 18:47:14 debian sshd[3737]: pam_mysql - option log.pid_column is set to "pid" May 10 18:47:14 debian sshd[3737]: pam_mysql - option log.user_column is set to "user" May 10 18:47:14 debian sshd[3737]: pam_mysql - option log.host_column is set to "host" May 10 18:47:14 debian sshd[3737]: pam_mysql - option log.rhost_column is set to "rhost" May 10 18:47:14 debian sshd[3737]: pam_mysql - option log.time_column is set to "time" May 10 18:47:14 debian sshd[3737]: pam_mysql - pam_sm_authenticate() called. May 10 18:47:14 debian sshd[3737]: pam_mysql - pam_mysql_open_db() called. May 10 18:47:14 debian sshd[3737]: pam_mysql - pam_mysql_open_db() returning 0. May 10 18:47:14 debian sshd[3737]: pam_mysql - pam_mysql_check_passwd() called. May 10 18:47:14 debian sshd[3737]: pam_mysql - pam_mysql_format_string() called May 10 18:47:14 debian sshd[3737]: pam_mysql - pam_mysql_quick_escape() called. May 10 18:47:14 debian sshd[3737]: pam_mysql - SELECT password FROM users WHERE username = 'testuser' May 10 18:47:14 debian sshd[3737]: pam_mysql - pam_mysql_check_passwd() returning 6. May 10 18:47:14 debian sshd[3737]: pam_mysql - pam_mysql_sql_log() called. May 10 18:47:14 debian sshd[3737]: pam_mysql - pam_mysql_sql_log() returning 0. May 10 18:47:14 debian sshd[3737]: pam_mysql - pam_mysql_converse() called. May 10 18:47:14 debian sshd[3737]: pam_mysql - pam_mysql_open_db() called. May 10 18:47:14 debian sshd[3737]: pam_mysql - pam_mysql_check_passwd() called. May 10 18:47:14 debian sshd[3737]: pam_mysql - pam_mysql_format_string() called May 10 18:47:14 debian sshd[3737]: pam_mysql - pam_mysql_quick_escape() called. May 10 18:47:14 debian sshd[3737]: pam_mysql - SELECT password FROM users WHERE username = 'testuser' May 10 18:47:14 debian sshd[3737]: pam_mysql - pam_mysql_check_passwd() returning 6. May 10 18:47:14 debian sshd[3737]: pam_mysql - pam_mysql_sql_log() called. May 10 18:47:14 debian sshd[3737]: pam_mysql - pam_mysql_sql_log() returning 0. May 10 18:47:14 debian sshd[3737]: pam_mysql - pam_sm_authenticate() returning 7. May 10 18:47:14 debian sshd[3737]: pam_unix(sshd:auth): check pass; user unknown May 10 18:47:14 debian sshd[3737]: pam_unix(sshd:auth): authentication failure; logname= uid=0 euid=0 tty=ssh ruser= rhost=10.0.2.2 May 10 18:47:16 debian sshd[3737]: Failed password for invalid user testuser from 10.0.2.2 port 22693 ssh2

on the ssh facing terminal, the prompt is: Permission denied, please try again.

i can only login using root...

maltris commented 4 years ago

Seems to be similar/same to https://github.com/NigelCunningham/pam-MySQL/issues/41

09173732546 commented 4 years ago

Seems to be similar/same to #41

yes but i dont know how to fix it... its my first time to use this plugin and i have no background on compiling stuff

kyrian666 commented 4 years ago

Hi,

Thanks for taking over this project, Nigel :)

So, I hit this error too. Looking at the code maybe this has a lot to do with the legacy "use_323_passwd" password support?

For all other cases of missing support in defines, there is an error message emitted in the logs, but for this one nothing seems to be emitted, so no wonder people are confused.

https://github.com/NigelCunningham/pam-MySQL/blob/master/pam_mysql.c#L4121-L4129

Looking at the code it appears that this has an undocumented fallback onto another crypt call that might not be compatible?

Tested this, and else being equal, if I use OLD_PASSWORD() style encryption in the mysql database and configure pam-mysql to use:

crypt=2 use_323_passwd=true

... Then auth fails.

However, if I re-encrypt the same with PASSWORD() instead and

crypt=2 use_323_passwd=false

Then authentication works.

Testing my dev setup, because I'm trying to upgrade a legacy system too, but I have complete freedom to faff with this and I do want it sorted properly because I'm using vagrant etc for testing to weed out issues just such as this.

However I will not necessary always know the password for all users, so doing that might not always be an option, and several people, myself included, would like a proper fix please :) Happy to dig in and help.

K.

PS. Test setup:

Description: Debian GNU/Linux 10 (buster)

ii libpam-mysql:amd64 0.8.1-1+b1 amd64 PAM module interfacing with MySQL databases

kyrian666 commented 4 years ago

Debian installs mariadb now, not 'normal' mysql, since the 'great fork event'. I saw it mooted somewhere that between mariadb 10.0.8 and 10.0.9 it lost the capability to do legacy authentication. And debian buster is 10.3 so well past that:

$ apt-get source libpam-mysql $ apt-get install debhelper default-libmysqlclient-dev libpam0g-dev $ dpkg-buildpackage -uc -b ... checking for make_scrambled_password_323... no

Well, that would explain a few things for Debian Buster at least, against mariadb 10.3.

This:

https://tecadmin.net/install-mysql-on-debian-10-buster/

For mysql community version 8.0.21-1debian10 with legacy authentication enabled. There's a reference in it to mysql > 5.x needing this legacy plugin for it to work but I didn't dwell on it.

$ apt-get purge mariadb* $ apt-get install libmysqlclient-dev $ Remove the 'default-' from the front of 'libmysqlclient-dev' in debian/control $ Add a new version entry in debian/changelog $ dpkg-buildpackage -uc -b ... checking for make_scrambled_password_323... no

Repeat with mysql 5.7.31-1debian10, and behaviour is the same, so not really surprising, annoyingly, that the legacy encryption isn't working when it's done using libraries rather than a custom SQL.

kyrian666 commented 4 years ago

All of this waffle aside, for people with 323-type encrypted passwords I don't think there's any option but to create a replica function for pam-mysql, like other similar projects have done:

https://www.mail-archive.com/debian-bugs-dist@lists.debian.org/msg1755569.html

Because using custom SQL containing OLD_PASSWORD() instead would expose the plaintext password in the mysql process list, which would be bad, probably.

It's too late in the day for me to consider that now, but maybe over the weekend.

PS. The affected code is the same as version 0.7RC1, so nothing to do with Nigel. I guess it's simply that mysql has moved on and this code has not.

kyrian666 commented 4 years ago

The attached patch appears to both a) report when the function is present/missing via defines and whether the use_323_password configuration directive expects it to be present (as expected it is not defined, but configured for use) when configured to log DEBUG messages, and provides a version of the functionality cut & pasted from mysql 5.0.15 source code (first place I could find a reasonably recent version) sql/password.c, which is GPL distributed, so I believe that should be fine as pam-mysql carries the same license.

diff.patch.txt

NigelCunningham commented 4 years ago

Hi Kyrian.

Apologies for not replying sooner. I'm always really busy but this week was worse than usual.

pam_mySQL needs a lot of love, so I really appreciate your effort on this issue and the level of detail you've provided. I've started work on changing the build system to meson (I bought an Autotools book but the system is just incomprehensible in my opinion!) and also want to refactor the code so I can get some unit tests written. Improved logging is also important so I'm happy to see your syslog messages in there.

With the above in mind, I'll commit your patch both to the main branch and my WIP.

Regards,

Nigel

NigelCunningham commented 4 years ago

Actually, I was a bit quick to send the above. I should have first asked you to make a fork and a pull request, please.

Thanks in advance!

kyrian666 commented 4 years ago

Sure no worries. Will do it this evening. Was late and couldn't be arsed to try to fight git permissions, just wanted to go to bed. ;-)

K.

On 29 August 2020 01:48:01 Nigel Cunningham notifications@github.com wrote:

Actually, I was a bit quick to send the above. I should have first asked you to make a fork and a pull request, please. Thanks in advance! — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

Sent with Aqua Mail for Android https://www.mobisystems.com/aqua-mail

kyrian666 commented 4 years ago

Here you go:

https://github.com/NigelCunningham/pam-MySQL/pull/66

I've made minor cosmetic changes from the version above, so might warrant testing again but I guess it will be OK, I guess I'll rebuild it in my test rig later today and report any problems, that makes a bit of sense.

There is an argument that the debug logging I've added might be spurious, but actually when you consider that end users might have no idea how the code was built when it's packaged into an RPM or .deb package, but they do control the logging level parameter of the built code, I think the right thing to do is to leave them in so end users can gain clarity without having to do as much digging as I did.

As a DevOps person myself I totally agree that automated testing and build pipelines is the way forward. Unfortunately I uncovered this issue myself manually because I could not readily see a way of populating a test user into a database, connecting to an IMAP port, attempting authentication, etc in an automated way without using 'expect' which would be horrible. I could write some lengthy perl thing or whatever, but it didn't feel worth the investment of time etc.

K.

PS. I also have a mild concern that old config files might have been able to accept =0 or =1 but it looks like the updated versions of this code may accept only literal strings of true and false, so that might be worth investigation or noting in the README. That issue is not serious enough for me to want to dig into it at length though!

NigelCunningham commented 4 years ago

I thought about the level of logging too and am on the same page there. Thanks very much for the PR.

I'll leave this issue open for now so I'm prompted to check that parameter issue you mentioned in the last paragraph of your previous comment.

Thanks again!

slimlv commented 3 years ago

fc985823c3a7c414c6071d2560a376285be40458 cannot be compiled on debian 10: a bunch of include lines are missed, ulong/uint are not recognized by gcc version 8.3.0 (Debian 8.3.0-6) @kyrian666 Can you please fix the code ?

kyrian666 commented 3 years ago

Compiling from source? On Debian 10? I left that behind in the '80s or '90s ;) I can't remember right now, but I did manage to get it to work as I'm currently testing an upgrade of an older system to Debian 10.x and I have a custom built .deb for that. I'll attach it momentarily, and if I can find out what I did to make it work, I'll post it here too.

kyrian666 commented 3 years ago

I've checked back through my notes and I see nothing about this patch not compiling correctly. Remember that Debian 10.x uses MariaDB not MySQL, so you probably need to update the package dependencies and rebuild the source debian package and then you'll be good. ISTR the mysql packages for debian 10.x are now called mysql-community-XYZ

Oh, I though there was no way to do it, but writing this message I see there's a way to attach files here, so I'll attach my custom .deb.... then I tried and they do not accept .deb files as an attachment. Ugh.

kyrian666 commented 3 years ago

For reasons uncertain (probably building in a docker container and discarding what I didn't need, or similar), all I can offer at the moment is this from the package control file:

Depends: libc6 (>= 2.14), libmariadb3 (>= 3.0.0), libpam0g (>= 0.99.7.1), libssl1.1 (>= 1.1.0)

slimlv commented 3 years ago

indeed there is a code issue, see my dirty patch

diff  --git a/compat_323_password.h b/compat_323_password.h
index 9807d07..3df745d 100644
--- a/compat_323_password.h
+++ b/compat_323_password.h
@@ -58,7 +58,8 @@
            // this three steps are done in check_scramble()

 *****************************************************************************/
-
+#include <string.h>
+#include <stdio.h>

 /*
     Generate binary hash from raw text string
@@ -70,22 +71,22 @@
     password_len IN  password length (password may be not null-terminated)
 */

-void compat_323_hash_password(ulong *result, const char *password, uint password_len)
+void compat_323_hash_password(unsigned long *result, const char *password, unsigned int password_len)
 {
-  register ulong nr=1345345333L, add=7, nr2=0x12345671L;
-  ulong tmp;
+  register unsigned long nr=1345345333L, add=7, nr2=0x12345671L;
+  unsigned long tmp;
   const char *password_end= password + password_len;
   for (; password < password_end; password++)
   {
     if (*password == ' ' || *password == '\t')
       continue;                                 /* skip space in password */
-    tmp= (ulong) (unsigned char) *password;
+    tmp= (unsigned long) (unsigned char) *password;
     nr^= (((nr & 63)+add)*tmp)+ (nr << 8);
     nr2+=(nr2 << 8) ^ nr;
     add+=tmp;
   }
-  result[0]=nr & (((ulong) 1L << 31) -1L); /* Don't use sign bit (str2int) */;
-  result[1]=nr2 & (((ulong) 1L << 31) -1L);
+  result[0]=nr & (((unsigned long) 1L << 31) -1L); /* Don't use sign bit (str2int) */;
+  result[1]=nr2 & (((unsigned long) 1L << 31) -1L);
 }

@@ -100,7 +101,7 @@ void compat_323_hash_password(ulong *result, const char *password, uint password

 void compat_make_scrambled_password_323(char *to, const char *password)
 {
-  ulong hash_res[2];
-  compat_323_hash_password(hash_res, password, (uint) strlen(password));
+  unsigned long hash_res[2];
+  compat_323_hash_password(hash_res, password, (unsigned int) strlen(password));
   sprintf(to, "%08lx%08lx", hash_res[0], hash_res[1]);
 }
diff --git a/pam_mysql.c b/pam_mysql.c
index 4c030e4..4f89236 100644
--- a/pam_mysql.c
+++ b/pam_mysql.c
@@ -137,6 +137,8 @@
 #include <assert.h>
 #endif

+#include "compat_323_password.h"
+
 #ifdef HAVE_MYSQL_H
 #include <mysql.h>
 #endif
kyrian666 commented 3 years ago

Between tiredness and a failing RAID controller at the time, and lots missing from my notes on the subject I can't explain why the only versions of my code I can find use 'ulong' instead of 'unsigned long', and why it's missing a #include compat_323_password.h, all of which should probably be there, and yet I still have a working (tested with serverspec a lot!) package that I built. I think I only ever built a binary package not a source one either. The above all seems valid to me, but I won't have the opportunity to verify any time soon. Suggest you fork the repo and raise a PR back like I did, so Nigel can just approve it.

PS. I just realised that because the reason I did all this in the first place was to make a repeatable build of the server that is running this stuff using ansible & serverspec, if I can't rebuild this package next time because bits are missing that's a total FAIL, so I'll have to revisit this whether I like it or not!

NigelCunningham commented 3 years ago

Thanks for the followup. Are you saying I should just revert the commit? FWIW, I'm slowly working on reworking things so we use Meson to build and so the one big file is split into logical parts, which I'm then seeking to use in unit tests. Hopefully that will help with the reproducibility of your build.

kyrian666 commented 3 years ago

Just apply the above patch of top of my code to fix it :)

https://github.com/NigelCunningham/pam-MySQL/issues/65#issuecomment-727166310

I've spent about 2 hours just now trying to replicate how on earth I managed to build a working debian package, but I have had no success. Debian complains about patched up source code not matching the signatures it has in the control files and I don't recall seeing that before or how on earth I worked around it. This is going to take time to figure out again, and damn sure I'm going to be taking thorough notes this time and not letting my sodding RAID controller eat the results (it is now working properly btw).

kyrian666 commented 3 years ago

In case there are any debian gurus out there who can help, or just if end users want a working version for Debian 10.x, however that patch above pans out, I've uploaded the debian package to http://kyrian.ore.org/libpam-mysql_0.8.1-3orenet_amd64.deb

kyrian666 commented 3 years ago

For the sake of at least some secondary verification, I've abandoned the idea of rebuilding the debian package and tried a straight command-line build with the patch applied, and it looks good (in so far as it outputs a library, whereas without the patch as stated the build does indeed choke), so let's merge it!

The only point of note is that for Debian (10.5, amd64) one has to invoke the configure command thusly:

./configure --with-pam-mods-dir=/lib/x86_64-linux-gnu/security

I guess it might be worth enabling it as an option in the configure script which appears not to search that directory for places to put pam modules?

PS. It would appear the checksum stuff above comes about when you try to build a source, but not binary version of the package, and that's probably why I skipped trying to build the source package, and only built a binary.

NigelCunningham commented 3 years ago

@wferi Want to give your input on the above?

kyrian666 commented 3 years ago

Created PR as above, no conflicts, should be a simple merge. I would suspect next steps would be to release a new minor version containing these two patches. Thereafter, if it passes all of the Debian QA (including not being scuppered by the included MySQL code), it can be released into mainline Debian as an update to fix the issue there. As for what CentOS and friends will be doing with it I don't know because I've drifted that strongly towards Debian.

wferi commented 3 years ago

I put together a little test suite, which shows this regression from Debian stretch to buster. The reason is that make_scrambled_password_323 is exported by MariaDB 10.1 (in stretch) but not exported by MariaDB 10.3 (in buster). It's really unfortunate that the current pam_mysql code simply ignores the respective configuration setting in this case without a warning (I'd say it should even die with a fatal error). Embedding the necessary code should be straightforward (licenses permitting); I'll look into this. A stable update fixing the buster version might be possible then.

kyrian666 commented 3 years ago

^^ this is obviously the fundamentally better way to do things.

I think I found out what I did, though, which was to start with the debian building machinery from pam-mysql_0.7~RC1-4 rather than 0.8.1, which is a bit more friendly to ad hoc hacking like this. Then it was a simple (if tedious) matter of amending the patching to make it work (remove duplicates and failing patch segments, add changelog entry, etc). and use the right path to install the module into finally (as above configure parameter).

Hence I've been able to rebuild my package (which installs on Debian 10.x) in a way that might make sense in a few months time (Obviously I have revision controlled and backed up the **** out of the recipie this time), tested this in the build environment I already had automated, on commit 42e596f from my fork of the repo which contains both my original patch and @slimlv's build time fixes (with apologies for keeping on spelling your handle wrongly), and the serverspec tests I have for both correct and incorrect authentication details, among others report:

"Finished in 13.49 seconds (files took 0.04823 seconds to load) 101 examples, 0 failures"

MUCH SUCCESS!!!

So I can confirm that the approach taken between the two patches both builds and works for simple test cases.

K.

PS. I've uploaded the result to http://kyrian.ore.org/libpam-mysql_0.8.1-5orenet_amd64.deb in case it helps anyone with their short-term struggles to get this stuff to work. md5sums included below, should anything ghastly ever happen to my web server:

md5sum libpam-mysql_0.8.1-* 649fe011ed11e65a280c3bc8598d2c39 libpam-mysql_0.8.1-3orenet_amd64.deb 4d3a28fbc552e76830c309f7ab9b263a libpam-mysql_0.8.1-5orenet_amd64.deb

PPS. Incidentally, 'testsaslauthd' provides a nice automation-friendly, although not fool proof way to verify logins through pam-mysql and others, so could form some part of a future test case for the software itself.

wferi commented 3 years ago

I uploaded 0.8.1-4 to Debian unstable with these patches (and some other small changes). If you want this to appear in a buster (stable) update, please fill an important bug on the package in the Debian BTS. My basic stance with respect to this function (and my_make_scrambled_password()) was presented in #16 (and on the linked commit): they shouldn't be imported but always reimplemented internally. (And OpenSSL be made a strict dependency, so that the internal hash implementations can be removed for good. This would get rid lots of conditional compilation directives, vastly simplifying the code.)

sylvestre commented 1 year ago

I saw this ticket and it was helpful. My issue was that the password had "{CRYPT}" at the beginning. removing it fixed the issue.

If I may, it would be helpful if pam_mysql_check_passwd() was returning a string with more information :)

NigelCunningham commented 6 months ago

Coming back to this after all this time (sorry), I'm not sure where we ended up - in the meaintime I've done a big refactor and switched to building using Meson. Perhaps I should look at the Debian source / patching?