Fields678 #1265
Fields678 #1265
Conversation
|
Regarding the check for 6 fields, I don't know why the code checks for 6 fields. The manual page says the fields are: |
|
git-blame(1) says the source code is checking for 6 fields since |
|
Ughhhh, CodeQL has caught an important bug here. Lines 1235 to 1237 in b9323a0 This is reading a buffer that we have never initialized. :( |
This issue could be bad. The good thing is that the read is limited to one byte. Link: <shadow-maint#1265> Signed-off-by: Alejandro Colomar <alx@kernel.org>
d8d180f to
8d09dfb
Compare
Link: <shadow-maint#1265> Signed-off-by: Alejandro Colomar <alx@kernel.org>
|
Cc: @thesamesam , @ikerexxe , @jubalh |
This array is only ever used as an array of 7.
alx@devuan:/srv/alx/src/shadow/shadow/master$ sed_rm_ccomments()
{
perl -p -e 's%/\*.*?\*/%%g' \
|sed -E '\%/\*%, \%\*/% {\%(\*/|/\*)%!d}' \
|sed -E '\%/\*% {s%/\*.*%%; n; s%.*\*/%%;}' \
|sed -E '\%/\*% {s%/\*.*%%; n; s%.*\*/%%;}' \
|sed 's%//.*%%';
}
alx@devuan:/srv/alx/src/shadow/shadow/master$ cat src/newusers.c | grepc main | sed_rm_ccomments | grep -nT 'n\?fields\(\[.*]\)\?'
4: char *fields[8];
5: int nfields;
59: for (cp = buf, nfields = 0; nfields < 7; nfields++) {
60: fields[nfields] = strsep(&cp, ":");
64: if (nfields != 6) {
72: pw = pw_locate (fields[0]);
74: if (NULL == pw && getpwnam(fields[0]) != NULL) {
77: Prog, fields[0]);
81: if (NULL == pw && get_user_id(fields[2], &uid) != 0) {
91: && (add_group (fields[0], fields[3], &gid, uid) != 0)) {
101: && (add_user (fields[0], uid, gid) != 0)) {
110: pw = pw_locate (fields[0]);
114: Prog, line, fields[0], pw_dbname ());
132: usernames[nusers-1] = xstrdup(fields[0]);
133: passwords[nusers-1] = xstrdup(fields[1]);
135: if (add_passwd (&newpw, fields[1]) != 0) {
141: if (!streq(fields[4], "")) {
142: newpw.pw_gecos = fields[4];
145: if (!streq(fields[5], "")) {
146: newpw.pw_dir = fields[5];
149: if (!streq(fields[6], "")) {
150: newpw.pw_shell = fields[6];
153: if ( !streq(fields[5], "")
195: if (is_sub_uid && want_subuids() && !local_sub_uid_assigned(fields[0])) {
205: if (sub_uid_add(fields[0], sub_uid_start, sub_uid_count) == 0)
216: if (is_sub_gid && want_subgids() && !local_sub_gid_assigned(fields[0])) {
225: if (sub_gid_add(fields[0], sub_gid_start, sub_gid_count) == 0) {
alx@devuan:/srv/alx/src/shadow/shadow/master$ cat src/newusers.c | grepc main | sed_rm_ccomments | grep -nTo 'n\?fields\(\[.*]\)\?'
4: fields[8]
5: nfields
59: nfields
59: nfields
59: nfields
60: fields[nfields]
64: nfields
72: fields[0]
74: fields[0]
77: fields[0]
81: fields[2]
91: fields[0], fields[3]
101: fields[0]
110: fields[0]
114: fields[0]
132: fields[0]
133: fields[1]
135: fields[1]
141: fields[4]
142: fields[4]
145: fields[5]
146: fields[5]
149: fields[6]
150: fields[6]
153: fields[5]
195: fields[0]
205: fields[0]
216: fields[0]
225: fields[0]
Fixes: 45c6603 (2007-10-07; "[svn-upgrade] Integrating new upstream version, shadow (19990709)")
Link: <shadow-maint#1155 (comment)>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Fixes: 45c6603 (2007-10-07; "[svn-upgrade] Integrating new upstream version, shadow (19990709)") Link: <shadow-maint#1265> Signed-off-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
The code is checking for 6 fields, while the comment is saying there must be 7 fields. Something is clearly bogus here. Let's assume the code is correct, and the comment is wrong. Fixes: 45c6603 (2007-10-07; "[svn-upgrade] Integrating new upstream version, shadow (19990709)") Signed-off-by: Alejandro Colomar <alx@kernel.org>
8d09dfb to
1dc42b0
Compare
|
Yeah i think it is correct at the moment. Nfields ends as 6 bc cp is null at bottom of loop after the seventh field is read. |
|
I don't understand your comments. This is the situation as I see it:
|
|
Maybe I should split the PR. |
@hallyn
This addresses several off-by-one issues found in <src/newusers.c>, while discussing #1155 (comment) during development/review of #1155.
Revisions:
v2
v3
v3b