search

shadow-maint / shadow

Upstream shadow tree
Other
292 stars 228 forks source link

useradd incorrectly modifies symbolic link target #933

Closed stoeckmann closed 5 months ago

stoeckmann commented 7 months ago

The copy_symlink function in lib/copydir.c incorrectly adjusts symbolic link targets. I am not sure if these links should be modified at all.

How to reproduce:

ln -s /etc/skel2 /etc/skel/link
useradd -m user
ls -l /home/user

The output is lrwxrwxrwx 1 user user 11 Jan 31 21:02 link -> /home/user2.

This happens because the copy_symlink function just checks if the target of the link starts with /etc/skel which is the directory from where we are copying. If at all, it would have to check for /etc/skel/. This wouldn't work for link targets like ///etc/skel though or other variants.

Since someone created a symbolic link with an absolute path, I would argue it should stay absolute. It's the easiest fix.

hallyn commented 6 months ago

Since someone created a symbolic link with an absolute path, I would argue it should stay absolute. It's the easiest fix.

This is what it's basically done forever. I don't think this is the kind of change we are at liberty to make.

alejandro-colomar commented 6 months ago

That makes sense, @hallyn .

If we don't accept https://github.com/shadow-maint/shadow/pull/966 (@edneville), then I think my second favourite option would be to keep this obviously broken, so keep it as is.

I wouldn't like changing this to check for a trailing /, which would still be a breaking change, and would be behavior that isn't expected by anyone (except probably by who wrote that code in the first place).

I'll keep my approval in that PR, for historic reasons (and because I agree that's the behavior we'd want if adding the feature today). I guess we can close the PR, and eventually come back to it if we need to.

edneville commented 5 months ago

@alejandro-colomar, if not accepted then perhaps this should be documented so that we don't get the same issue popping up. Does that seem a better approach, if so I can adjust the PR. The behaviour can be written in the man page, if needed instead.

alejandro-colomar commented 5 months ago

@alejandro-colomar, if not accepted then perhaps this should be documented so that we don't get the same issue popping up. Does that seem a better approach, if so I can adjust the PR. The behaviour can be written in the man page, if needed instead.

Sure, I think this issue should be documented, in a BUGS section (it's a known bug that we can't fix for reasons). But please don't change that PR. I'd like to keep it as is. Please open a new one.