Bug 9013 - Desktop Managers (xdm, gdm, lightdm...) crashes with SIGSEGV in _pam_winbind_change_pwd() when password is expiring
Summary: Desktop Managers (xdm, gdm, lightdm...) crashes with SIGSEGV in _pam_winbind_...
Status: RESOLVED FIXED
Alias: None
Product: Samba 3.6
Classification: Unclassified
Component: Winbind (show other bugs)
Version: 3.6.3
Hardware: All Linux
: P5 normal
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-28 11:20 UTC by Luca 'remix_tj' Lorenzetto
Modified: 2012-09-17 09:37 UTC (History)
0 users

See Also:


Attachments
Fixes bug adding a check for a null value before calling strcasecmp (1.03 KB, patch)
2012-09-11 09:39 UTC, Luca 'remix_tj' Lorenzetto
ddiss: review+
asn: review+
Details
patch for 3.6 series, same code change. (1.07 KB, patch)
2012-09-11 16:59 UTC, David Disseldorp
ddiss: review+
asn: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Luca 'remix_tj' Lorenzetto 2012-06-28 11:20:11 UTC
I'm using libpam-winbind on ubuntu 12.04 and my client is member of a Windows Domain. A domain user can login using samba/winbind without problem in tty and via lightdm (or any other DM) if the user password is ok.

If the password is expiring a domain user logs in correctly via TTY, with a message "Your password is expiring in 10 days". if tries with lightdm the user gets the message "Your password is expiring in 10 days", but then returns to the username request.

So the user with his password in expiring status cannot login graphically. The login via TTY works, but shows a message "erroneous conversation (5)", but even after googling a lot i had no chance to identify how to solve.

So i did some tests to check if also other distributions are affected and i identified the bug as a pam_winbind.so bug that does not depends to ubuntu/debian, because also fedora 17 is affected


This is the report i created for ubuntu, that contains also some core dumps generated with the debug symbols

https://bugs.launchpad.net/ubuntu/+source/samba/+bug/1003296
Comment 1 Luca 'remix_tj' Lorenzetto 2012-06-28 11:24:48 UTC
I can say you that winbind 3.4.7 shipped with ubuntu lucid doesn't have the problem (checked on my collegue's PC)
Comment 2 Luca 'remix_tj' Lorenzetto 2012-06-28 11:35:39 UTC
Also the package shipped in debian stable (3.5.6) has the bug.
Comment 3 Luca 'remix_tj' Lorenzetto 2012-09-11 09:39:39 UTC
Created attachment 7877 [details]
Fixes bug adding a check for a null value before calling strcasecmp

After new tests i've been able to identify the exact problem. 

In the function _pam_winbind_change_pwd, at line 834 of nsswitch/pam_winbind.c there is a call to strcasecmp that causes the crash.

At line 832 the value of resp->resp gets printed on logfile and i get:

Received [(null)] reply from application.

So the function strcasecmp is called with null as parameter, that causes the crash. I do not know if this behavior is correct, but i added a check for a null value on the line 834, avoiding the call of strcasecmp if resp->resp is null.

Now i rebuild the lib with this patch and i can login even if the password is expiring.
Comment 4 David Disseldorp 2012-09-11 16:28:40 UTC
Comment on attachment 7877 [details]
Fixes bug adding a check for a null value before calling strcasecmp

The change looks good to me.

Thanks a lot for the report and patch Luca!
Comment 5 Andreas Schneider 2012-09-11 16:58:24 UTC
Comment on attachment 7877 [details]
Fixes bug adding a check for a null value before calling strcasecmp

lgtm
Comment 6 David Disseldorp 2012-09-11 16:59:55 UTC
Created attachment 7878 [details]
patch for 3.6 series, same code change.

Submitted to autobuild for master inclusion. Further review is required prior to merging for 3.6.next
Comment 7 Andreas Schneider 2012-09-11 17:03:07 UTC
Comment on attachment 7878 [details]
patch for 3.6 series, same code change.

lgtm too
Comment 8 David Disseldorp 2012-09-11 17:19:45 UTC
(In reply to comment #6)
> Created attachment 7878 [details]
> patch for 3.6 series, same code change.
> 
> Submitted to autobuild for master inclusion. Further review is required prior
> to merging for 3.6.next

Karolin, please add this to 3.6.next.
Comment 9 Jeremy Allison 2012-09-11 21:23:57 UTC
FYI Karolin, patch also applies cleanly to 3.5.next (and I've pushed to master).

Please pick :

https://attachments.samba.org/attachment.cgi?id=7878

for 3.5.next and 3.6.next.

Cheers,

Jeremy.
Comment 10 Karolin Seeger 2012-09-12 08:26:04 UTC
(In reply to comment #9)
> FYI Karolin, patch also applies cleanly to 3.5.next (and I've pushed to
> master).
> 
> Please pick :
> 
> https://attachments.samba.org/attachment.cgi?id=7878
> 
> for 3.5.next and 3.6.next.
> 
> Cheers,
> 
> Jeremy.

The release branch is already frozen for 3.6.8 release preparations.
Will be included in 3.6.9.
Comment 11 Jeremy Allison 2012-09-12 17:06:52 UTC
No problem, thanks for looking after the releases so well.

Thanks !

Jeremy.
Comment 12 Karolin Seeger 2012-09-17 09:37:46 UTC
Pushed to v3-5-test and v3-6-test.
Closing out bug report.

Thanks!