Bug 15224 - pam_winbind uses time_t and pointers assuming they are of the same size
Summary: pam_winbind uses time_t and pointers assuming they are of the same size
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: Winbind (show other bugs)
Version: 4.17.1
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Jule Anger
QA Contact: Samba QA Contact
Depends on:
Reported: 2022-11-03 20:07 UTC by Michael Tokarev
Modified: 2022-12-15 16:35 UTC (History)
2 users (show)

See Also:

git-am fix for master. (2.63 KB, patch)
2022-11-09 00:18 UTC, Jeremy Allison
no flags Details
git-am fix for 4.17.next, 4.16.next. (4.02 KB, patch)
2022-11-16 20:47 UTC, Jeremy Allison
npower: review+

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Tokarev 2022-11-03 20:07:23 UTC
In nsswitch/pam_winbind.c, there are 2 places like this:

               time_t pwdlastset_prelim [ = some value];

# pam_winbind.c 3271
                pam_set_data(pamh, PAM_WINBIND_PWD_LAST_SET,
                             (void *)pwdlastset_prelim, NULL);

                time_t pwdlastset_update = 0;

# pam_winbind.c 3346
                pam_get_data(pamh, PAM_WINBIND_PWD_LAST_SET,
                             (const void **) &pwdlastset_update);

It is not guaranteed that time_t is exactly of the same size as a pointer.  There are already architecture(s) today which breaks this assumption, for example x32, where time_t is 8 bytes but pointer is 4 bytes, so we're storing only the half of the value there.  This breaks with musl where time_t is always 64bit no matter which size a pointer is.

Here, either a pointer to an allocated time_t should be stored (maybe conditionally, I dunno), or some other time representation should be used.  The latter - the pam_get_data case - even looks dangerous, - I'd use a void* variable, pass its address to pam_get_data, and use direct cast from pointer to time_t, instead of passing an address to an object of potentially different size.
Comment 1 Jeremy Allison 2022-11-09 00:18:02 UTC
Created attachment 17639 [details]
git-am fix for master.

Michael, does this fix work for you ? If you can confirm I'll add it as an MR and try and get it reviewed and merged.
Comment 2 Jeremy Allison 2022-11-11 19:31:49 UTC
From: Michael Tokarev <mjt@tls.msk.ru>
To: Jeremy Allison <jra@samba.org>
Subject: Re: Can you look at the patch on bug

Hello again!  As promised, I'm following up.  It looks good so far.
I also tested it on a real system, - things are looking promising.
Comment 3 Samba QA Contact 2022-11-16 15:10:03 UTC
This bug was referenced in samba master:

Comment 4 Samba QA Contact 2022-11-16 19:30:04 UTC
This bug was referenced in samba master:

Comment 5 Jeremy Allison 2022-11-16 20:47:20 UTC
Created attachment 17661 [details]
git-am fix for 4.17.next, 4.16.next.

Cherry-picked from master.
Comment 6 Noel Power 2022-11-19 08:26:40 UTC
Comment on attachment 17661 [details]
git-am fix for 4.17.next, 4.16.next.

Comment 7 Noel Power 2022-11-19 08:27:24 UTC
reassign to Jule for inclusion in 4.16/17
Comment 8 Jule Anger 2022-11-23 07:17:08 UTC
Pushed to autobuild-v4-{17,16}-test.
Comment 9 Samba QA Contact 2022-11-23 13:53:25 UTC
This bug was referenced in samba v4-16-test:

Comment 10 Samba QA Contact 2022-11-23 13:57:19 UTC
This bug was referenced in samba v4-17-test:

Comment 11 Jule Anger 2022-11-23 14:51:20 UTC
Closing out bug report.

Comment 12 Samba QA Contact 2022-12-15 16:34:37 UTC
This bug was referenced in samba v4-16-stable (Release samba-4.16.8):

Comment 13 Samba QA Contact 2022-12-15 16:35:56 UTC
This bug was referenced in samba v4-17-stable (Release samba-4.17.4):