login: Fix no-pam authorization regression

[stoeckmann]

A regression in /etc/login.access parsing could allow or deny more users than configured. It affects shadow 4.17.x.

The regression takes place in login tool if compiled without PAM support. Basically all distributions ship either with PAM support or use util-linux login, so this affects rather few systems, let alone it requires the EXCEPT keyword in /etc/login.access. Affected right now:

• Linux From Scratch

And these distributions could be affected depending on compile configuration, but have older unaffected versions:

• Gentoo
• NixOS

The list_match function handles EXCEPT entries in /etc/login.access through recursive calls. It calls itself with NULL, which was then passed to strtok so parsing continued at current position.

Replacing strtok with strsep, this means that EXCEPT entries never match, because strsep(NULL, ...) always returns NULL, i.e. the code treats everything after EXCEPT as non-existing.

Fix this by passing current list pointer to recursive call.

Proof of Concept:

1. Compile shadow without PAM support, i.e. use --without-libpam, no need to install login into system
2. Create this /etc/login.access file (replace myuser with a user which is okay if access is denied):

# Allow everyone explicitly (first matching rule wins) except myuser
+:ALL EXCEPT myuser:ALL
# Explicitly deny myuser
-:myuser:ALL

3. Run compiled login program

sudo ./src/login

If you try to log in as denied user, you can actually log in. This happens because the first rule matches myuser because EXCEPT handling is broken. With this patch, myuser is correctly blocked because the first rule does not match, but the second.

Fixes: 90afe61 (2024-07-04; "lib/, src/: Use strsep(3) instead of strtok(3)")

[stoeckmann]

I've noticed this while further investigating src/login_nopam.c which could use a bit refactoring. Definitely not adding it to this PR.

[alejandro-colomar]

Reviewed-by: Alejandro Colomar <[email protected]>

[alejandro-colomar]

CC: @ikerexxe , @hallyn

@stoeckmann

BTW, a minor issue I noticed in the commit message after merging.

The syntax I'm using for git commit references is:

hash ([author-date,] commit-date; "subject")

So it would be either of these:

Fixes: 90afe61003ef (2024-12-05; "lib/, src/: Use strsep(3) instead of strtok(3)")
Fixes: 90afe61003ef (2024-07-04, 2024-12-05; "lib/, src/: Use strsep(3) instead of strtok(3)")

The reason I use the commit date is that it makes it easy to know if a release is affected by a bug and needs cherrypicking just by looking at the commit date. The author date can differ significantly from the commit date.

The author date I usually omit it, and only write it if it's important for some reason.

I have a couple of git aliases that make it easy to print those references:

[alias]
	ref = show --no-patch --abbrev=12 --date=short \
		--format=tformat:'%C(auto)%h%C(reset) %C(white)(%cd%x3B \"%C(reset)%C(auto)%s%C(reset)%C(white)\")%C(reset)'
	ref2 = show --no-patch --abbrev=12 --date=short \
		--format=tformat:'%C(auto)%h%C(reset) %C(white)(%ad, %cd%x3B \"%C(reset)%C(auto)%s%C(reset)%C(white)\")%C(reset)'

Which produces:

$ git ref 90afe61003
90afe61003ef (2024-12-05; "lib/, src/: Use strsep(3) instead of strtok(3)")
$ git ref2 90afe61003
90afe61003ef (2024-07-04, 2024-12-05; "lib/, src/: Use strsep(3) instead of strtok(3)")

Also, in relation with https://lwn.net/Articles/1001526/, assuming a collision in the future where both a hash (prefix) and a subject collide (the subject may collide for trivial cosmetic fixes), the commit date makes it easy to search the commit in the log: commit dates are strictly increasing in a branch. The author date doesn't help searching.

[ikerexxe]

The author date I usually omit it, and only write it if it's important for some reason.

Makes sense.

I have a couple of git aliases that make it easy to print those references:

[alias]
	ref = show --no-patch --abbrev=12 --date=short \
		--format=tformat:'%C(auto)%h%C(reset) %C(white)(%cd%x3B \"%C(reset)%C(auto)%s%C(reset)%C(white)\")%C(reset)'
	ref2 = show --no-patch --abbrev=12 --date=short \
		--format=tformat:'%C(auto)%h%C(reset) %C(white)(%ad, %cd%x3B \"%C(reset)%C(auto)%s%C(reset)%C(white)\")%C(reset)'

Wonderful! Thank you for those aliases ;)

Also, in relation with https://lwn.net/Articles/1001526/, assuming a collision in the future where both a hash (prefix) and a subject collide (the subject may collide for trivial cosmetic fixes), the commit date makes it easy to search the commit in the log: commit dates are strictly increasing in a branch. The author date doesn't help searching.

I'm not deeply preoccupied by this right now since I'm assuming git itself will create a way to avoid this problem. But maybe I'm too optimistic.