Adding checks for fd omission

[skyler-ferrante]

Adding function check_fds to new file fd.c. The function check_fds should be called in every setuid/setgid program. I also added sanitize_env to every setuid/setgid program.

[alejandro-colomar]

On Fri, Mar 08, 2024 at 05:18:22PM -0800, Skyler Ferrante wrote:

@skyler-ferrante commented on this pull request.

(Alternatively, you might unconditionally close devnull.)

I think this is dangerous, since devnull is guaranteed to be equal to the fd we are checking. If stdin (0) is not open and we open(/dev/null) devnull should be equal to 0. The dup2 isn't actually necessary, its just a good idea to do, since a system might not hand out new fds starting at the lowest available.

Ahh, sorry, I forgot! You're right.

Mind that the if is there because a leak for fd 0 1 or 2 is not really a leak, since those fds we actually want them open.

It should only be a leak if the system doesn't give new fds starting at the lowest option available since we start at 0 and go to 3. But yeah, it's still probably a good idea to check.

Hmmm, I don't mind removing the check. Or maybe we should abort if it is true, because it should never be.

-- https://www.alejandro-colomar.es/

[alejandro-colomar]

@hallyn , I'd merge this, but the master branch doesn't yet contain the 4.15.0 release tag (and thus still has the 4.14.0 version in configure.ac). Would you please fast-forward master before merging this?

[hallyn]

@hallyn , I'd merge this, but the master branch doesn't yet contain the 4.15.0 release tag (and thus still has the 4.14.0 version in configure.ac). Would you please fast-forward master before merging this?

It doesn't? I did push it, and used it for making the release:

https://github.com/shadow-maint/shadow/releases/tag/4.15.0

What's missing?

[hallyn]

Anyway +1 from me, I was about to merge when I noticed your comment.

[alejandro-colomar]

@hallyn , I'd merge this, but the master branch doesn't yet contain the 4.15.0 release tag (and thus still has the 4.14.0 version in configure.ac). Would you please fast-forward master before merging this?

It doesn't? I did push it, and used it for making the release:

https://github.com/shadow-maint/shadow/releases/tag/4.15.0

What's missing?

I think the branch protection might have caused your push to (partially?) fail?

Now I see you've merged&pushed another branch:

* 5ce1b0a6 (shadow/master) tests/unit/test_zustr2stp.c: Test ZUSTR2STP()
* d3cf98ff lib/string/zustr2stp.[ch]: Remove zustr2stp(); keep ZUSTR2STP()
| * ba43b49a (tag: 4.15.0, stable/4.15.x, shadow/4.15.x, 4.15.x) configure.ac: Release 4.15.0
|/  
* 89c4da43 (HEAD -> master, stable/master, alx/master) src/vipw.c: Use string literals to initialize 'Prog'

The tag was correctly pushed, but the branch wasn't fast-forwarded to the tag.