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
Status: RESOLVED FIXED
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
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-11-03 20:07 UTC by Michael Tokarev
Modified: 2022-11-23 14:51 UTC (History)
2 users (show)

See Also:


Attachments
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+
Details

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
        https://bugzilla.samba.org/show_bug.cgi?id=15224

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:

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

f6284877ce07fc5ddf4f4e2d824013b645d6e12c
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.

lgtm
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:

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

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

Thanks!