Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Day and time cleanups for shadow file entries #876

Merged
merged 2 commits into from
Feb 13, 2024

Conversation

stoeckmann
Copy link
Contributor

The first commit is about SCALE removal:

  • SCALE is always DAY, so drop the definition
  • With SCALE being gone, drop unneeded calculations

The other commits clean up calculations based on days per epoch and seconds per epoch. It is much easier to calculate with days (in a long) than with seconds (in a time_t) because the former is already the native encoding in shadow files and LONG_MAX exists, while TIME_T_MAX does not exist everywhere.

  • Use proper casts which become relevant for 32 bit systems with 64 bit time_t, e.g. musl-based distributions
  • Let passwd work with day precision instead of seconds since epoch to allow easier range checking
  • Unify integer overflow checks in chage
  • Add LONG_MAX range checks when working with sp_max etc.

Proof of Concept (for 64 bit systems):

  1. Compile and definitely use an sgetspent implementation which gets long parsing right (glibc does not)
ac_cv_func_getspnam=no \
ac_cv_func_sgetspent=no \
CFLAGS="-fsanitize=undefined" \
./configure --without-libpam
  1. Setup users with large shadow entries
# chage -m 10 user1
# chage -d 9223372036854775807 user1
# chage -M 10 user2
# chage -d 9223372036854775807 user2
# chage -M 10 user3
# chage -I 20 user3
# chage -d 9223372036854775807 user3
# chage -I 20 user4
# chage -W 1 user4
# chage -d 123 user4
# chage -M 9223372036854775807 user4
  1. Change password as user1
$ passwd
../../src/passwd.c:393:6: runtime error: signed integer overflow: 9223372036854775807 * 86400 cannot be represented in type 'long int'
  1. Run expiry as user2
$ expiry -c
../../lib/isexpired.c:97:28: runtime error: signed integer overflow: 9223372036854775807 + 10 cannot be represented in type 'long int'
  1. Run expiry as user3
$ expiry -c
../../lib/age.c:165:25: runtime error: signed integer overflow: 9223372036854775807 + 99999 cannot be represented in type 'long int'
../../lib/isexpired.c:75:32: runtime error: signed integer overflow: 9223372036854775807 + 99999 cannot be represented in type 'long int'
  1. Run expiry as user4
$ expiry -c
../../lib/age.c:165:25: runtime error: signed integer overflow: 123 + 9223372036854775807 cannot be represented in type 'long int'
../../lib/age.c:165:9: runtime error: signed integer overflow: -9223372036854775686 - 19709 cannot be represented in type 'long int'
../../lib/isexpired.c:75:32: runtime error: signed integer overflow: 123 + 9223372036854775807 cannot be represented in type 'long int'

The user1 password change should not be allowed because the minimum wait time since last change (in the future) is not reached yet.

As a solution I have capped these calculations at LONG_MAX since we are talking, even for 32 bit systems, about a time span of more than 6 million years before this capping becomes a problem. Rather... Theoretical... I hope. :)

lib/isexpired.c Outdated Show resolved Hide resolved
src/chage.c Outdated Show resolved Hide resolved
src/passwd.c Outdated Show resolved Hide resolved
lib/age.c Outdated Show resolved Hide resolved
lib/isexpired.c Outdated Show resolved Hide resolved
lib/isexpired.c Outdated Show resolved Hide resolved
lib/isexpired.c Outdated Show resolved Hide resolved
@stoeckmann
Copy link
Contributor Author

I've checked the box which allows edits by maintainers. Perhaps it's easier to consider my PR as a suggestion so wordings and other things can be changed directly without going through comments/reviews.

@alejandro-colomar
Copy link
Collaborator

I've checked the box which allows edits by maintainers. Perhaps it's easier to consider my PR as a suggestion so wordings and other things can be changed directly without going through comments/reviews.

I find it more interesting to have a conversation. I may say very dumb things, so if you do the changes after my suggestions, there's some chance of catching any mistakes I may say. :)

@stoeckmann
Copy link
Contributor Author

Got the return value checks mixed up. Now it should be functional again.

@alejandro-colomar
Copy link
Collaborator

alejandro-colomar commented Dec 20, 2023

2ce77ab:

Reviewed-by: Alejandro Colomar <alx@kernel.org>

src/passwd.c Outdated Show resolved Hide resolved
@stoeckmann
Copy link
Contributor Author

Got the return value checks mixed up. Now it should be functional again.

And the limit had to be adjusted... AGAIN.

If this keeps going on like this, I'll change this PR into an issue and someone else can look at it. Maybe people can find this kind of interaction enjoyable, but I'm not part of that group.

@alejandro-colomar
Copy link
Collaborator

747ca85:

Reviewed-by: Alejandro Colomar <alx@kernel.org>

@alejandro-colomar
Copy link
Collaborator

73d8e90:

Reviewed-by: Alejandro Colomar <alx@kernel.org>

src/pwck.c Outdated Show resolved Hide resolved
@alejandro-colomar
Copy link
Collaborator

918877f:

Reviewed-by: Alejandro Colomar <alx@kernel.org>

lib/age.c Outdated Show resolved Hide resolved
@alejandro-colomar
Copy link
Collaborator

alejandro-colomar commented Jan 29, 2024

v3b changes:

  • tfix
$ git range-diff shadow/master ts/scale scale 
1:  7064949f = 1:  7064949f src/chage.c: Unify long overflow checks in print_day_as_date()
2:  431c1370 = 2:  431c1370 lib/: Saturate addition to avoid overflow
3:  b7d41838 = 3:  b7d41838 lib/, src/, po/: date_to_str(): Move function to header, and make inline
4:  4dcd3740 = 4:  4dcd3740 lib/day_to_str.h: DAY_TO_STR(): Add macro
5:  0c98bcc7 = 5:  0c98bcc7 src/: Use DAY_TO_STR() instead of its pattern
6:  0a6115f1 = 6:  0a6115f1 src/passwd.c: print_status(): Fix typo (bogus use of the comma operator)
7:  88fa4850 ! 7:  dc76b4e2 lib/day_to_str.[ch]: day_to_str(): Accept a day instead of a date, and rename function
    @@ lib/day_to_str.h
      
      
     -#define DAY_TO_STR(str, day)   date_to_str(NITEMS(str), str, date * DAY)
    -+#define DAY_TO_STR(str, day)   day_to_str(NITEMS(str), str, date)
    ++#define DAY_TO_STR(str, day)   day_to_str(NITEMS(str), str, day)
      
      
     -inline void date_to_str(size_t size, char buf[size], long date);

v3c changes:

  • Put the tfix where it belongs
$ git range-diff shadow/master ts/scale scale 
1:  7064949f = 1:  7064949f src/chage.c: Unify long overflow checks in print_day_as_date()
2:  431c1370 = 2:  431c1370 lib/: Saturate addition to avoid overflow
3:  b7d41838 = 3:  b7d41838 lib/, src/, po/: date_to_str(): Move function to header, and make inline
4:  4dcd3740 ! 4:  728c4439 lib/day_to_str.h: DAY_TO_STR(): Add macro
    @@ lib/day_to_str.h
      #include "string/strtcpy.h"
      
      
    -+#define DAY_TO_STR(str, day)   date_to_str(NITEMS(str), str, date * DAY)
    ++#define DAY_TO_STR(str, day)   date_to_str(NITEMS(str), str, day * DAY)
     +
     +
      inline void date_to_str(size_t size, char buf[size], long date);
5:  0c98bcc7 = 5:  0db7c279 src/: Use DAY_TO_STR() instead of its pattern
6:  0a6115f1 = 6:  d5d05811 src/passwd.c: print_status(): Fix typo (bogus use of the comma operator)
7:  dc76b4e2 ! 7:  b762fa5c lib/day_to_str.[ch]: day_to_str(): Accept a day instead of a date, and rename function
    @@ lib/day_to_str.h
      #include "string/strtcpy.h"
      
      
    --#define DAY_TO_STR(str, day)   date_to_str(NITEMS(str), str, date * DAY)
    +-#define DAY_TO_STR(str, day)   date_to_str(NITEMS(str), str, day * DAY)
     +#define DAY_TO_STR(str, day)   day_to_str(NITEMS(str), str, day)
      
      

lib/day_to_str.h Fixed Show fixed Hide fixed
@alejandro-colomar
Copy link
Collaborator

v4 changes:

  • Use gmtime_r(3) to silence a CodeQL warning
$ git range-diff shadow/master ts/scale scale 
1:  7064949f = 1:  7064949f src/chage.c: Unify long overflow checks in print_day_as_date()
2:  431c1370 = 2:  431c1370 lib/: Saturate addition to avoid overflow
3:  b7d41838 = 3:  b7d41838 lib/, src/, po/: date_to_str(): Move function to header, and make inline
4:  728c4439 = 4:  728c4439 lib/day_to_str.h: DAY_TO_STR(): Add macro
5:  0db7c279 = 5:  0db7c279 src/: Use DAY_TO_STR() instead of its pattern
6:  d5d05811 = 6:  d5d05811 src/passwd.c: print_status(): Fix typo (bogus use of the comma operator)
7:  b762fa5c = 7:  b762fa5c lib/day_to_str.[ch]: day_to_str(): Accept a day instead of a date, and rename function
-:  -------- > 8:  cdf2b18d lib/, src/: Call gmtime_r(3) instead of gmtime(3)

@alejandro-colomar
Copy link
Collaborator

v4b changes:

  • tfix
$ git range-diff shadow/master ts/scale scale 
1:  7064949f = 1:  7064949f src/chage.c: Unify long overflow checks in print_day_as_date()
2:  431c1370 = 2:  431c1370 lib/: Saturate addition to avoid overflow
3:  b7d41838 = 3:  b7d41838 lib/, src/, po/: date_to_str(): Move function to header, and make inline
4:  728c4439 = 4:  728c4439 lib/day_to_str.h: DAY_TO_STR(): Add macro
5:  0db7c279 = 5:  0db7c279 src/: Use DAY_TO_STR() instead of its pattern
6:  d5d05811 = 6:  d5d05811 src/passwd.c: print_status(): Fix typo (bogus use of the comma operator)
7:  b762fa5c = 7:  b762fa5c lib/day_to_str.[ch]: day_to_str(): Accept a day instead of a date, and rename function
8:  cdf2b18d ! 8:  5eca2425 lib/, src/: Call gmtime_r(3) instead of gmtime(3)
    @@ lib/day_to_str.h: inline void day_to_str(size_t size, char buf[size], long day);
     -  time_t           date;
     -  const struct tm  *tm;
     +  time_t     date;
    -+  struct tm  *tm;
    ++  struct tm  tm;
      
        if (day < 0) {
                strtcpy(buf, "never", size);

@alejandro-colomar
Copy link
Collaborator

v4c changes:

  • Rebase to master
$ git range-diff master..ts/scale shadow/master..scale 
1:  7064949f = 1:  b87b00b8 src/chage.c: Unify long overflow checks in print_day_as_date()
2:  431c1370 = 2:  2e9c342e lib/: Saturate addition to avoid overflow
3:  b7d41838 = 3:  84ee257f lib/, src/, po/: date_to_str(): Move function to header, and make inline
4:  728c4439 = 4:  b35c17c4 lib/day_to_str.h: DAY_TO_STR(): Add macro
5:  0db7c279 = 5:  f618c183 src/: Use DAY_TO_STR() instead of its pattern
6:  d5d05811 = 6:  7a774808 src/passwd.c: print_status(): Fix typo (bogus use of the comma operator)
7:  b762fa5c = 7:  621d80db lib/day_to_str.[ch]: day_to_str(): Accept a day instead of a date, and rename function
8:  5eca2425 = 8:  3ddd2772 lib/, src/: Call gmtime_r(3) instead of gmtime(3)

The conversion from day to seconds can be done in print_date
(renamed to print_day_as_date for clarification).  This has the nice
benefit that DAY multiplication and long to time_t conversion are done
at just one place.

Co-developed-by: Tobias Stoeckmann <tobias@stoeckmann.org>
Co-developed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Very large values in /etc/shadow could lead to overflows.  Make sure
that these calculations are saturated at LONG_MAX.  Since entries are
based on days and not seconds since epoch, saturating won't hurt anyone.

Co-developed-by: Tobias Stoeckmann <tobias@stoeckmann.org>
Co-developed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
@alejandro-colomar
Copy link
Collaborator

v4d changes:

  • Rebase to master
$ git range-diff f98e43ee..ts/scale shadow/master..scale 
1:  b87b00b8 = 1:  741fc096 src/chage.c: Unify long overflow checks in print_day_as_date()
2:  2e9c342e = 2:  d5d9194e lib/: Saturate addition to avoid overflow
3:  84ee257f = 3:  20bb258f lib/, src/, po/: date_to_str(): Move function to header, and make inline
4:  b35c17c4 = 4:  8461594e lib/day_to_str.h: DAY_TO_STR(): Add macro
5:  f618c183 = 5:  b1f47ef6 src/: Use DAY_TO_STR() instead of its pattern
6:  7a774808 = 6:  3379b384 src/passwd.c: print_status(): Fix typo (bogus use of the comma operator)
7:  621d80db = 7:  fd3908fa lib/day_to_str.[ch]: day_to_str(): Accept a day instead of a date, and rename function
8:  3ddd2772 = 8:  5f46fdf5 lib/, src/: Call gmtime_r(3) instead of gmtime(3)

@alejandro-colomar
Copy link
Collaborator

v4e changes:

  • Add missing include
$ git range-diff shadow/master ts/scale scale 
1:  741fc096 = 1:  741fc096 src/chage.c: Unify long overflow checks in print_day_as_date()
2:  d5d9194e = 2:  d5d9194e lib/: Saturate addition to avoid overflow
3:  20bb258f = 3:  20bb258f lib/, src/, po/: date_to_str(): Move function to header, and make inline
4:  8461594e ! 4:  46362e76 lib/day_to_str.h: DAY_TO_STR(): Add macro
    @@ lib/day_to_str.h
      #include <time.h>
      
     +#include "defines.h"
    ++#include "sizeof.h"
      #include "string/strtcpy.h"
      
      
5:  b1f47ef6 = 5:  fd9ca675 src/: Use DAY_TO_STR() instead of its pattern
6:  3379b384 = 6:  ce086de9 src/passwd.c: print_status(): Fix typo (bogus use of the comma operator)
7:  fd3908fa = 7:  1a73b4d1 lib/day_to_str.[ch]: day_to_str(): Accept a day instead of a date, and rename function
8:  5f46fdf5 = 8:  d7b2729e lib/, src/: Call gmtime_r(3) instead of gmtime(3)

@alejandro-colomar
Copy link
Collaborator

v5 changes:

  • Drop some patches, which have been moved to a separate PR.
$ git range-diff master ts/scale scale 
1:  741fc096 = 1:  741fc096 src/chage.c: Unify long overflow checks in print_day_as_date()
2:  d5d9194e = 2:  d5d9194e lib/: Saturate addition to avoid overflow
3:  20bb258f < -:  -------- lib/, src/, po/: date_to_str(): Move function to header, and make inline
4:  46362e76 < -:  -------- lib/day_to_str.h: DAY_TO_STR(): Add macro
5:  fd9ca675 < -:  -------- src/: Use DAY_TO_STR() instead of its pattern
6:  ce086de9 < -:  -------- src/passwd.c: print_status(): Fix typo (bogus use of the comma operator)
7:  1a73b4d1 < -:  -------- lib/day_to_str.[ch]: day_to_str(): Accept a day instead of a date, and rename function
8:  d7b2729e < -:  -------- lib/, src/: Call gmtime_r(3) instead of gmtime(3)

@hallyn hallyn merged commit 674409e into shadow-maint:master Feb 13, 2024
9 checks passed
alejandro-colomar pushed a commit that referenced this pull request Feb 14, 2024
ITI_AGING is not set through any build environment. If it would be set,
then timings in /etc/shadow would not fit anymore.

Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
Cherry-picked-from: ab260fc ("lib/defines.h: Remove ITI_AGING")
Link: <#873>
Link: <#876>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
[alx: This is a pre-requisite for 674409e ("lib/: Saturate addition to avoid overflow")]
Signed-off-by: Alejandro Colomar <alx@kernel.org>
alejandro-colomar pushed a commit that referenced this pull request Feb 14, 2024
SCALE is always DAY (and has to be always DAY), so replace it with DAY
in source code and remove unneeded calculations.

Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
Link: <#876>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Cherry-picked-from: ecc3508 ("lib/, src/: Remove SCALE definition")
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Link: <#888>
Link: <#876>
[alx: This is a pre-requisite for 674409e ("lib/: Saturate addition to avoid overflow")]
Signed-off-by: Alejandro Colomar <alx@kernel.org>
alejandro-colomar pushed a commit that referenced this pull request Feb 14, 2024
The conversion from day to seconds can be done in print_date
(renamed to print_day_as_date for clarification).  This has the nice
benefit that DAY multiplication and long to time_t conversion are done
at just one place.

Co-developed-by: Tobias Stoeckmann <tobias@stoeckmann.org>
Co-developed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Cherry-picked-from: 20100e4 ("src/chage.c: Unify long overflow checks in print_day_as_date()")
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Link: <#876>
[alx: This is a pre-requisite for 674409e ("lib/: Saturate addition to avoid overflow")]
Signed-off-by: Alejandro Colomar <alx@kernel.org>
alejandro-colomar pushed a commit that referenced this pull request Feb 14, 2024
Very large values in /etc/shadow could lead to overflows.  Make sure
that these calculations are saturated at LONG_MAX.  Since entries are
based on days and not seconds since epoch, saturating won't hurt anyone.

Co-developed-by: Tobias Stoeckmann <tobias@stoeckmann.org>
Co-developed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Cherry-picked-from: 674409e ("lib/: Saturate addition to avoid overflow")
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Link: <#876>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants