lib/chkname.c: Fix off by one validation of _SC_LOGIN_NAME_MAX #935
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The _SC_LOGIN_NAME_MAX value includes space for the NUL byte. The length of name must smaller than this value to be valid. Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
If the system does not have a user name length limit, support it accordingly. If the system has no _SC_LOGIN_NAME_MAX, use LOGIN_NAME_MAX constant instead. Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
stoeckmann
force-pushed
the
user_name_length
branch
from
2b603c6
to
7b4e40e
Compare
hallyn
reviewed
Feb 4, 2024
alejandro-colomar
approved these changes
Feb 4, 2024
Closed
alejandro-colomar
added a commit
that referenced
this pull request
Feb 4, 2024
I used size_t because: sysconf(3) can return -1 if the value is not supported, but then it can only mean that there's no limit. Having no limit is the same as having a limit of SIZE_MAX (to which -1 is converted). Signed-off-by: Alejandro Colomar <alx@kernel.org> Cherry-picked-from: 6be85b0 ("lib/chkname.c: Use tmp variable to avoid a -Wsign-compare warning") [alx: This is to cherry-pick the next commit without conflict] Link: <#801> Link: <#935> Cc: Serge Hallyn <serge@hallyn.com> Cc: Tobias Stoeckmann <tobias@stoeckmann.org> Signed-off-by: Alejandro Colomar <alx@kernel.org>
alejandro-colomar
pushed a commit
that referenced
this pull request
Feb 4, 2024
The _SC_LOGIN_NAME_MAX value includes space for the NUL byte. The length of name must smaller than this value to be valid. Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org> Cherry-picked-from: 403a2e3 ("lib/chkname.c: Take NUL byte into account") Link: <#935> Cc: Serge Hallyn <serge@hallyn.com> Signed-off-by: Alejandro Colomar <alx@kernel.org>
alejandro-colomar
added a commit
to alejandro-colomar/shadow
that referenced
this pull request
Feb 5, 2024
Before 3b7cc05 ("lib: replace `USER_NAME_MAX_LENGTH` macro"), this code did use a length. It used a utmp(5) fixed-width buffer, so the length matches the buffer size (there was no terminating NUL byte). However, sysconf(_SC_LOGIN_NAME_MAX) returns a buffer size that accounts for the terminating null byte; see sysconf(3). Thus, the commit that introduced the call to sysconf(3), should have taken that detail into account. 403a2e3 ("lib/chkname.c: Take NUL byte into account"), by Tobias, caught that bug in <lib/chkname.c>, but missed that the same commit that introduced that bug, introduced the same bug in two other places. This fixes all remaining calls to sysconf(_SC_LOGIN_NAME_MAX). I still observe some suspicious code after this fix: if (do_rlogin(hostname, username, max_size - 1, term, sizeof(term))) ... login_prompt(username, max_size - 1); We're passing a length to functions that want a size. But since the fix to those will be different, let's do that in the following commits. Link: <shadow-maint#935> Link: <shadow-maint#920 (comment)> Link: <shadow-maint#757> Link: <shadow-maint#674> See-also: 403a2e3 ("lib/chkname.c: Take NUL byte into account") Fixes: 3b7cc05 ("lib: replace `USER_NAME_MAX_LENGTH` macro") Cc: Tobias Stoeckmann <tobias@stoeckmann.org> Cc: Serge Hallyn <serge@hallyn.com> Cc: Iker Pedrosa <ipedrosa@redhat.com> Signed-off-by: Alejandro Colomar <alx@kernel.org>
alejandro-colomar
added a commit
to alejandro-colomar/shadow
that referenced
this pull request
Feb 5, 2024
These functions expect a size, not a length. Don't subtract 1 to the size. Link: <shadow-maint#935> Link: <shadow-maint#920 (comment)> Link: <shadow-maint#757> Link: <shadow-maint#674> See-also: 0656a90bfd0d ("src/login.c: Fix off-by-one buggs") See-also: 403a2e3 ("lib/chkname.c: Take NUL byte into account") Fixes: 3b7cc05 ("lib: replace `USER_NAME_MAX_LENGTH` macro") Cc: Tobias Stoeckmann <tobias@stoeckmann.org> Cc: Serge Hallyn <serge@hallyn.com> Cc: Iker Pedrosa <ipedrosa@redhat.com> Signed-off-by: Alejandro Colomar <alx@kernel.org>
alejandro-colomar
added a commit
to alejandro-colomar/shadow
that referenced
this pull request
Feb 5, 2024
If (maxsize == -1), then ((size_t)maxsize == SIZE_MAX). And no size can ever be >= SIZE_MAX, so it will never return false if sysconf(3) reports an unlimited user-name size via returning -1. The commit that introduced that check missed that this code had always supported unlimited user-name sizes since it was introduced by Iker in 3b7cc05 ("lib: replace `USER_NAME_MAX_LENGTH` macro"), and 6be85b0 ("lib/chkname.c: Use tmp variable to avoid a -Wsign-compare warning") even clarified this in the commit message. So, while the code in 6a1f45d ("lib/chkname.c: Support unlimited user name lengths") wasn't bad per se, the commit message was incorrect. What that patch did was adding code for handling EINVAL (or any other errors that a future kernel might add). Link: <shadow-maint#935> Link: <shadow-maint#935 (comment)> See-also: 6be85b0 ("lib/chkname.c: Use tmp variable to avoid a -Wsign-compare warning") Fixes: 6a1f45d ("lib/chkname.c: Support unlimited user name lengths") Cc: Tobias Stoeckmann <tobias@stoeckmann.org> Cc: Serge Hallyn <serge@hallyn.com> Signed-off-by: Alejandro Colomar <alx@kernel.org>
alejandro-colomar
added a commit
to alejandro-colomar/shadow
that referenced
this pull request
Feb 5, 2024
If (maxsize == -1), then ((size_t)maxsize == SIZE_MAX). And no size can ever be >= SIZE_MAX, so it will never return false if sysconf(3) reports an unlimited user-name size via returning -1. Well, to be pedantic, that disallows a user-name siz of precisely SIZE_MAX bytes when sysconf(3) returns -1. However, that's probably a good thing; such a long user name might trigger Undefined Behavior somewhere else, so be cautious and disallow it. I hope nobody will be using the entire address space for a user name. The commit that introduced that check missed that this code had always supported unlimited user-name sizes since it was introduced by Iker in 3b7cc05 ("lib: replace `USER_NAME_MAX_LENGTH` macro"), and 6be85b0 ("lib/chkname.c: Use tmp variable to avoid a -Wsign-compare warning") even clarified this in the commit message. So, while the code in 6a1f45d ("lib/chkname.c: Support unlimited user name lengths") wasn't bad per se, the commit message was incorrect. What that patch did was adding code for handling EINVAL (or any other errors that a future kernel might add). To be more pedantically correct, that commit also allowed (under certain circumstances, user names of SIZE_MAX bytes, but those were originally allowed (by accident), and only became disallowed in 403a2e3 ("lib/chkname.c: Take NUL byte into account"). But again, let's disallow those, just to be cautious. Link: <shadow-maint#935> Link: <shadow-maint#935 (comment)> See-also: 6be85b0 ("lib/chkname.c: Use tmp variable to avoid a -Wsign-compare warning") Fixes: 6a1f45d ("lib/chkname.c: Support unlimited user name lengths") Cc: Tobias Stoeckmann <tobias@stoeckmann.org> Cc: Serge Hallyn <serge@hallyn.com> Signed-off-by: Alejandro Colomar <alx@kernel.org>
alejandro-colomar
added a commit
to alejandro-colomar/shadow
that referenced
this pull request
Feb 5, 2024
Before 3b7cc05 ("lib: replace `USER_NAME_MAX_LENGTH` macro"), this code did use a length. It used a utmp(5) fixed-width buffer, so the length matches the buffer size (there was no terminating NUL byte). However, sysconf(_SC_LOGIN_NAME_MAX) returns a buffer size that accounts for the terminating null byte; see sysconf(3). Thus, the commit that introduced the call to sysconf(3), should have taken that detail into account. 403a2e3 ("lib/chkname.c: Take NUL byte into account"), by Tobias, caught that bug in <lib/chkname.c>, but missed that the same commit that introduced that bug, introduced the same bug in two other places. This fixes all remaining calls to sysconf(_SC_LOGIN_NAME_MAX). I still observe some suspicious code after this fix: if (do_rlogin(hostname, username, max_size - 1, term, sizeof(term))) ... login_prompt(username, max_size - 1); We're passing size-1 to functions that want a size. But since the fix to those will be different, let's do that in the following commits. Link: <shadow-maint#935> Link: <shadow-maint#920 (comment)> Link: <shadow-maint#757> Link: <shadow-maint#674> See-also: 403a2e3 ("lib/chkname.c: Take NUL byte into account") Fixes: 3b7cc05 ("lib: replace `USER_NAME_MAX_LENGTH` macro") Cc: Tobias Stoeckmann <tobias@stoeckmann.org> Cc: Serge Hallyn <serge@hallyn.com> Cc: Iker Pedrosa <ipedrosa@redhat.com> Signed-off-by: Alejandro Colomar <alx@kernel.org>
alejandro-colomar
added a commit
to alejandro-colomar/shadow
that referenced
this pull request
Feb 5, 2024
These functions expect a size, not a length. Don't subtract 1 to the size. Link: <shadow-maint#935> Link: <shadow-maint#920 (comment)> Link: <shadow-maint#757> Link: <shadow-maint#674> See-also: 0656a90bfd0d ("src/login.c: Fix off-by-one buggs") See-also: 403a2e3 ("lib/chkname.c: Take NUL byte into account") Fixes: 3b7cc05 ("lib: replace `USER_NAME_MAX_LENGTH` macro") Cc: Tobias Stoeckmann <tobias@stoeckmann.org> Cc: Serge Hallyn <serge@hallyn.com> Cc: Iker Pedrosa <ipedrosa@redhat.com> Signed-off-by: Alejandro Colomar <alx@kernel.org>
alejandro-colomar
added a commit
to alejandro-colomar/shadow
that referenced
this pull request
Feb 5, 2024
If (maxsize == -1), then ((size_t)maxsize == SIZE_MAX). And no size can ever be >= SIZE_MAX, so it will never return false if sysconf(3) reports an unlimited user-name size via returning -1. Well, to be pedantic, that disallows a user-name siz of precisely SIZE_MAX bytes when sysconf(3) returns -1. However, that's probably a good thing; such a long user name might trigger Undefined Behavior somewhere else, so be cautious and disallow it. I hope nobody will be using the entire address space for a user name. The commit that introduced that check missed that this code had always supported unlimited user-name sizes since it was introduced by Iker in 3b7cc05 ("lib: replace `USER_NAME_MAX_LENGTH` macro"), and 6be85b0 ("lib/chkname.c: Use tmp variable to avoid a -Wsign-compare warning") even clarified this in the commit message. So, while the code in 6a1f45d ("lib/chkname.c: Support unlimited user name lengths") wasn't bad per se, the commit message was incorrect. What that patch did was adding code for handling EINVAL (or any other errors that a future kernel might add). To be more pedantically correct, that commit also allowed (under certain circumstances, user names of SIZE_MAX bytes, but those were originally allowed (by accident), and only became disallowed in 403a2e3 ("lib/chkname.c: Take NUL byte into account"). But again, let's disallow those, just to be cautious. Link: <shadow-maint#935> Link: <shadow-maint#935 (comment)> See-also: 6be85b0 ("lib/chkname.c: Use tmp variable to avoid a -Wsign-compare warning") Fixes: 6a1f45d ("lib/chkname.c: Support unlimited user name lengths") Cc: Tobias Stoeckmann <tobias@stoeckmann.org> Cc: Serge Hallyn <serge@hallyn.com> Signed-off-by: Alejandro Colomar <alx@kernel.org>
alejandro-colomar
added a commit
to alejandro-colomar/shadow
that referenced
this pull request
Feb 8, 2024
Before 3b7cc05 ("lib: replace `USER_NAME_MAX_LENGTH` macro"), this code did use a length. It used a utmp(5) fixed-width buffer, so the length matches the buffer size (there was no terminating NUL byte). However, sysconf(_SC_LOGIN_NAME_MAX) returns a buffer size that accounts for the terminating null byte; see sysconf(3). Thus, the commit that introduced the call to sysconf(3), should have taken that detail into account. 403a2e3 ("lib/chkname.c: Take NUL byte into account"), by Tobias, caught that bug in <lib/chkname.c>, but missed that the same commit that introduced that bug, introduced the same bug in two other places. This fixes all remaining calls to sysconf(_SC_LOGIN_NAME_MAX). I still observe some suspicious code after this fix: if (do_rlogin(hostname, username, max_size - 1, term, sizeof(term))) ... login_prompt(username, max_size - 1); We're passing size-1 to functions that want a size. But since the fix to those will be different, let's do that in the following commits. Link: <shadow-maint#935> Link: <shadow-maint#920 (comment)> Link: <shadow-maint#757> Link: <shadow-maint#674> See-also: 403a2e3 ("lib/chkname.c: Take NUL byte into account") Fixes: 3b7cc05 ("lib: replace `USER_NAME_MAX_LENGTH` macro") Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com> Cc: Tobias Stoeckmann <tobias@stoeckmann.org> Cc: Serge Hallyn <serge@hallyn.com> Signed-off-by: Alejandro Colomar <alx@kernel.org>
alejandro-colomar
added a commit
to alejandro-colomar/shadow
that referenced
this pull request
Feb 8, 2024
These functions expect a size, not a length. Don't subtract 1 to the size. Link: <shadow-maint#935> Link: <shadow-maint#920 (comment)> Link: <shadow-maint#757> Link: <shadow-maint#674> See-also: 0656a90bfd0d ("src/login.c: Fix off-by-one buggs") See-also: 403a2e3 ("lib/chkname.c: Take NUL byte into account") Fixes: 3b7cc05 ("lib: replace `USER_NAME_MAX_LENGTH` macro") Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com> Cc: Tobias Stoeckmann <tobias@stoeckmann.org> Cc: Serge Hallyn <serge@hallyn.com> Signed-off-by: Alejandro Colomar <alx@kernel.org>
alejandro-colomar
added a commit
to alejandro-colomar/shadow
that referenced
this pull request
Feb 8, 2024
If (maxsize == -1), then ((size_t)maxsize == SIZE_MAX). And no size can ever be >= SIZE_MAX, so it will never return false if sysconf(3) reports an unlimited user-name size via returning -1. Well, to be pedantic, that disallows a user-name siz of precisely SIZE_MAX bytes when sysconf(3) returns -1. However, that's probably a good thing; such a long user name might trigger Undefined Behavior somewhere else, so be cautious and disallow it. I hope nobody will be using the entire address space for a user name. The commit that introduced that check missed that this code had always supported unlimited user-name sizes since it was introduced by Iker in 3b7cc05 ("lib: replace `USER_NAME_MAX_LENGTH` macro"), and 6be85b0 ("lib/chkname.c: Use tmp variable to avoid a -Wsign-compare warning") even clarified this in the commit message. So, while the code in 6a1f45d ("lib/chkname.c: Support unlimited user name lengths") wasn't bad per se, the commit message was incorrect. What that patch did was adding code for handling EINVAL (or any other errors that a future kernel might add). To be more pedantically correct, that commit also allowed (under certain circumstances, user names of SIZE_MAX bytes, but those were originally allowed (by accident), and only became disallowed in 403a2e3 ("lib/chkname.c: Take NUL byte into account"). But again, let's disallow those, just to be cautious. Link: <shadow-maint#935> Link: <shadow-maint#935 (comment)> See-also: 6be85b0 ("lib/chkname.c: Use tmp variable to avoid a -Wsign-compare warning") Fixes: 6a1f45d ("lib/chkname.c: Support unlimited user name lengths") Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com> Cc: Tobias Stoeckmann <tobias@stoeckmann.org> Cc: Serge Hallyn <serge@hallyn.com> Signed-off-by: Alejandro Colomar <alx@kernel.org>
hallyn
pushed a commit
that referenced
this pull request
Feb 13, 2024
Before 3b7cc05 ("lib: replace `USER_NAME_MAX_LENGTH` macro"), this code did use a length. It used a utmp(5) fixed-width buffer, so the length matches the buffer size (there was no terminating NUL byte). However, sysconf(_SC_LOGIN_NAME_MAX) returns a buffer size that accounts for the terminating null byte; see sysconf(3). Thus, the commit that introduced the call to sysconf(3), should have taken that detail into account. 403a2e3 ("lib/chkname.c: Take NUL byte into account"), by Tobias, caught that bug in <lib/chkname.c>, but missed that the same commit that introduced that bug, introduced the same bug in two other places. This fixes all remaining calls to sysconf(_SC_LOGIN_NAME_MAX). I still observe some suspicious code after this fix: if (do_rlogin(hostname, username, max_size - 1, term, sizeof(term))) ... login_prompt(username, max_size - 1); We're passing size-1 to functions that want a size. But since the fix to those will be different, let's do that in the following commits. Link: <#935> Link: <#920 (comment)> Link: <#757> Link: <#674> See-also: 403a2e3 ("lib/chkname.c: Take NUL byte into account") Fixes: 3b7cc05 ("lib: replace `USER_NAME_MAX_LENGTH` macro") Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com> Cc: Tobias Stoeckmann <tobias@stoeckmann.org> Cc: Serge Hallyn <serge@hallyn.com> Signed-off-by: Alejandro Colomar <alx@kernel.org>
hallyn
pushed a commit
that referenced
this pull request
Feb 13, 2024
These functions expect a size, not a length. Don't subtract 1 to the size. Link: <#935> Link: <#920 (comment)> Link: <#757> Link: <#674> See-also: 0656a90bfd0d ("src/login.c: Fix off-by-one buggs") See-also: 403a2e3 ("lib/chkname.c: Take NUL byte into account") Fixes: 3b7cc05 ("lib: replace `USER_NAME_MAX_LENGTH` macro") Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com> Cc: Tobias Stoeckmann <tobias@stoeckmann.org> Cc: Serge Hallyn <serge@hallyn.com> Signed-off-by: Alejandro Colomar <alx@kernel.org>
hallyn
pushed a commit
that referenced
this pull request
Feb 13, 2024
If (maxsize == -1), then ((size_t)maxsize == SIZE_MAX). And no size can ever be >= SIZE_MAX, so it will never return false if sysconf(3) reports an unlimited user-name size via returning -1. Well, to be pedantic, that disallows a user-name siz of precisely SIZE_MAX bytes when sysconf(3) returns -1. However, that's probably a good thing; such a long user name might trigger Undefined Behavior somewhere else, so be cautious and disallow it. I hope nobody will be using the entire address space for a user name. The commit that introduced that check missed that this code had always supported unlimited user-name sizes since it was introduced by Iker in 3b7cc05 ("lib: replace `USER_NAME_MAX_LENGTH` macro"), and 6be85b0 ("lib/chkname.c: Use tmp variable to avoid a -Wsign-compare warning") even clarified this in the commit message. So, while the code in 6a1f45d ("lib/chkname.c: Support unlimited user name lengths") wasn't bad per se, the commit message was incorrect. What that patch did was adding code for handling EINVAL (or any other errors that a future kernel might add). To be more pedantically correct, that commit also allowed (under certain circumstances, user names of SIZE_MAX bytes, but those were originally allowed (by accident), and only became disallowed in 403a2e3 ("lib/chkname.c: Take NUL byte into account"). But again, let's disallow those, just to be cautious. Link: <#935> Link: <#935 (comment)> See-also: 6be85b0 ("lib/chkname.c: Use tmp variable to avoid a -Wsign-compare warning") Fixes: 6a1f45d ("lib/chkname.c: Support unlimited user name lengths") Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com> Cc: Tobias Stoeckmann <tobias@stoeckmann.org> Cc: Serge Hallyn <serge@hallyn.com> Signed-off-by: Alejandro Colomar <alx@kernel.org>
alejandro-colomar
added a commit
that referenced
this pull request
Feb 14, 2024
Before 3b7cc05 ("lib: replace `USER_NAME_MAX_LENGTH` macro"), this code did use a length. It used a utmp(5) fixed-width buffer, so the length matches the buffer size (there was no terminating NUL byte). However, sysconf(_SC_LOGIN_NAME_MAX) returns a buffer size that accounts for the terminating null byte; see sysconf(3). Thus, the commit that introduced the call to sysconf(3), should have taken that detail into account. 403a2e3 ("lib/chkname.c: Take NUL byte into account"), by Tobias, caught that bug in <lib/chkname.c>, but missed that the same commit that introduced that bug, introduced the same bug in two other places. This fixes all remaining calls to sysconf(_SC_LOGIN_NAME_MAX). I still observe some suspicious code after this fix: if (do_rlogin(hostname, username, max_size - 1, term, sizeof(term))) ... login_prompt(username, max_size - 1); We're passing size-1 to functions that want a size. But since the fix to those will be different, let's do that in the following commits. Link: <#935> Link: <#920 (comment)> Link: <#757> Link: <#674> See-also: 403a2e3 ("lib/chkname.c: Take NUL byte into account") Fixes: 3b7cc05 ("lib: replace `USER_NAME_MAX_LENGTH` macro") Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com> Cc: Tobias Stoeckmann <tobias@stoeckmann.org> Cc: Serge Hallyn <serge@hallyn.com> Signed-off-by: Alejandro Colomar <alx@kernel.org> Cherry-picked-from: 6551709 ("src/login.c: Fix off-by-one buggs") Link: <#936> Signed-off-by: Alejandro Colomar <alx@kernel.org>
alejandro-colomar
added a commit
that referenced
this pull request
Feb 14, 2024
These functions expect a size, not a length. Don't subtract 1 to the size. Link: <#935> Link: <#920 (comment)> Link: <#757> Link: <#674> See-also: 0656a90bfd0d ("src/login.c: Fix off-by-one buggs") See-also: 403a2e3 ("lib/chkname.c: Take NUL byte into account") Fixes: 3b7cc05 ("lib: replace `USER_NAME_MAX_LENGTH` macro") Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com> Cc: Tobias Stoeckmann <tobias@stoeckmann.org> Cc: Serge Hallyn <serge@hallyn.com> Signed-off-by: Alejandro Colomar <alx@kernel.org> Cherry-picked-from: 15882a5 ("src/login.c: Fix off-by-one bugss") Link: <#936> 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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
According to sysconf manual page, _SC_LOGIN_NAME_MAX is the maximum name length, including the terminating null byte.
Take this into account while validating user names.
Also support systems which might have no upper limit or no _SC_LOGIN_NAME_MAX at all.