Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fputsx is not round-trip safe with backslash-ending usernames #1055

Open
zeha opened this issue Jul 20, 2024 · 10 comments · May be fixed by #1056
Open

fputsx is not round-trip safe with backslash-ending usernames #1055

zeha opened this issue Jul 20, 2024 · 10 comments · May be fixed by #1056

Comments

@zeha
Copy link
Contributor

zeha commented Jul 20, 2024

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1076619 reports:

The following sequence breaks /etc/group:

$ useradd 'vijai\'
$ usermod -aG sudo 'vijai\'
$ useradd blafasel
$ usermod -aG sudo blafasel

Results in:

sudo:x:27:install,vijaiaudio:x:29:install

Should have been:

sudo:x:27:install,vijai\,blafasel
audio:x:29:install

I believe this is caused by fgetsx "supporting" backslash as line-continuation indicator, but

  • fputsx does not escape backslashes before line end
  • there's a comment in fputsx indicating this is unsupported by everything else anyway.
@alejandro-colomar
Copy link
Collaborator

alejandro-colomar commented Jul 20, 2024

I had been looking at that code, and it seemed too brittle, and inconsistent.

I preferred to not change it at all while it worked, but your comment seems to be what I was waiting/expecting for: a report that it actually doesn't work.

Do you have any specific fix in mind?

Should we remove support for line continuations completely? That would keep it simple, by calling standard fgets(3). (But I was worried that it could break existing config files.)

@zeha
Copy link
Contributor Author

zeha commented Jul 21, 2024

But I was worried that it could break existing config files.

From what I can tell, line continuation doesn't work on glibc systems for lookup:

root@tiksta:~# tail -n2 /etc/group
tftp:x:112:\
ch
root@tiksta:~# getent group tftp
tftp:x:112:\

root@tiksta:~# tail -n1 /etc/group
tftp:x:112:ch
root@tiksta:~# getent group tftp
tftp:x:112:ch

Thus I cannot imagine it being used a lot, and wouldn't worry about breaking config files (which were already broken if used).

Do you have any specific fix in mind?

I was thinking replace fgetsx/fputsx by fgets/fputs.

@alejandro-colomar
Copy link
Collaborator

alejandro-colomar commented Jul 21, 2024

But I was worried that it could break existing config files.

From what I can tell, line continuation doesn't work on glibc systems for lookup:

root@tiksta:~# tail -n2 /etc/group
tftp:x:112:\
ch
root@tiksta:~# getent group tftp
tftp:x:112:\

root@tiksta:~# tail -n1 /etc/group
tftp:x:112:ch
root@tiksta:~# getent group tftp
tftp:x:112:ch

Thus I cannot imagine it being used a lot, and wouldn't worry about breaking config files (which were already broken if used).

Thanks!

Do you have any specific fix in mind?

I was thinking replace fgetsx/fputsx by fgets/fputs.

Sounds like what I had in mind. I'll have a try. :-)

alejandro-colomar added a commit to alejandro-colomar/shadow that referenced this issue Jul 21, 2024
It seems they never worked correctly.  Let's keep it simple and remove
support for escaped newlines.

Closes: <shadow-maint#1055>
Reported-by: Chris Hofstaedtler <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
alejandro-colomar added a commit to alejandro-colomar/shadow that referenced this issue Jul 21, 2024
It seems they never worked correctly.  Let's keep it simple and remove
support for escaped newlines.

Closes: <shadow-maint#1055>
Reported-by: Chris Hofstaedtler <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar
Copy link
Collaborator

alejandro-colomar commented Jul 21, 2024

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1076619 reports:

The following sequence breaks /etc/group:

$ useradd 'vijai\'
$ usermod -aG sudo 'vijai\'
$ useradd blafasel
$ usermod -aG sudo blafasel

Results in:

sudo:x:27:install,vijaiaudio:x:29:install

Should have been:

sudo:x:27:install,vijai\,blafasel
audio:x:29:install

I believe this is caused by fgetsx "supporting" backslash as line-continuation indicator, but

* fputsx does not escape backslashes before line end

* there's a comment in fputsx indicating this is unsupported by everything else anyway.

On the other hand, user and group names are not allowed to contain a \; do they?

* [a-zA-Z0-9_.][a-zA-Z0-9_.-]*$\?

@zeha
Copy link
Contributor Author

zeha commented Jul 21, 2024

On the other hand, user and group names are not allowed to contain a \; do they?

I think --force-badname will skip this check.

(As such, the reproducer above doesn't work as-is with upstream shadow. It works without --force-badname in Debian because we carry a patch to relax the user/group-name check - which I eventually want to get rid of.)

@alejandro-colomar
Copy link
Collaborator

alejandro-colomar commented Jul 21, 2024

On the other hand, user and group names are not allowed to contain a \; do they?

I think --force-badname will skip this check.

(As such, the reproducer above doesn't work as-is with upstream shadow. It works without --force-badname in Debian because we carry a patch to relax the user/group-name check - which I eventually want to get rid of.)

What's your opinion? Do you still think fgetsx()/fputsx() are broken?

@zeha
Copy link
Contributor Author

zeha commented Jul 21, 2024

What's your opinion? Do you still think fgetsx()/fputsx() are broken?

Yes.

@alejandro-colomar
Copy link
Collaborator

alejandro-colomar commented Jul 21, 2024

What's your opinion? Do you still think fgetsx()/fputsx() are broken?

Yes.

I tend to agree; --badname is a flag we support, and these functions break its behavior.

(And they keep the software more complex, which is bad per se.)

alejandro-colomar added a commit to alejandro-colomar/shadow that referenced this issue Jul 23, 2024
It seems they never worked correctly.  Let's keep it simple and remove
support for escaped newlines.

Closes: <shadow-maint#1055>
Reported-by: Chris Hofstaedtler <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
alejandro-colomar added a commit to alejandro-colomar/shadow that referenced this issue Jul 30, 2024
It seems they never worked correctly.  Let's keep it simple and remove
support for escaped newlines.

Closes: <shadow-maint#1055>
Reported-by: Chris Hofstaedtler <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
alejandro-colomar added a commit to alejandro-colomar/shadow that referenced this issue Aug 12, 2024
It seems they never worked correctly.  Let's keep it simple and remove
support for escaped newlines.

Closes: <shadow-maint#1055>
Reported-by: Chris Hofstaedtler <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
alejandro-colomar added a commit to alejandro-colomar/shadow that referenced this issue Aug 26, 2024
It seems they never worked correctly.  Let's keep it simple and remove
support for escaped newlines.

Closes: <shadow-maint#1055>
Reported-by: Chris Hofstaedtler <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
alejandro-colomar added a commit to alejandro-colomar/shadow that referenced this issue Aug 31, 2024
It seems they never worked correctly.  Let's keep it simple and remove
support for escaped newlines.

Closes: <shadow-maint#1055>
Reported-by: Chris Hofstaedtler <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
alejandro-colomar added a commit to alejandro-colomar/shadow that referenced this issue Sep 1, 2024
It seems they never worked correctly.  Let's keep it simple and remove
support for escaped newlines.

Closes: <shadow-maint#1055>
Reported-by: Chris Hofstaedtler <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
alejandro-colomar added a commit to alejandro-colomar/shadow that referenced this issue Oct 14, 2024
It seems they never worked correctly.  Let's keep it simple and remove
support for escaped newlines.

Closes: <shadow-maint#1055>
Reported-by: Chris Hofstaedtler <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
alejandro-colomar added a commit to alejandro-colomar/shadow that referenced this issue Oct 14, 2024
It seems they never worked correctly.  Let's keep it simple and remove
support for escaped newlines.

Closes: <shadow-maint#1055>
Reported-by: Chris Hofstaedtler <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
alejandro-colomar added a commit to alejandro-colomar/shadow that referenced this issue Oct 14, 2024
It seems they never worked correctly.  Let's keep it simple and remove
support for escaped newlines.

Closes: <shadow-maint#1055>
Reported-by: Chris Hofstaedtler <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
alejandro-colomar added a commit to alejandro-colomar/shadow that referenced this issue Oct 15, 2024
It seems they never worked correctly.  Let's keep it simple and remove
support for escaped newlines.

Closes: <shadow-maint#1055>
Reported-by: Chris Hofstaedtler <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
alejandro-colomar added a commit to alejandro-colomar/shadow that referenced this issue Oct 15, 2024
It seems they never worked correctly.  Let's keep it simple and remove
support for escaped newlines.

Closes: <shadow-maint#1055>
Reported-by: Chris Hofstaedtler <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
alejandro-colomar added a commit to alejandro-colomar/shadow that referenced this issue Oct 27, 2024
It seems they never worked correctly.  Let's keep it simple and remove
support for escaped newlines.

Closes: <shadow-maint#1055>
Reported-by: Chris Hofstaedtler <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
@Zugschlus
Copy link

Usernames containing backslashes at least used ot be a rather common phenomenon in mixed Unix/Windows environments, with Unix user names being like DOMAIN\username. And breaking basic unix configuration files like /etc/group is a rather severe misbehavior.

@alejandro-colomar
Copy link
Collaborator

alejandro-colomar commented Oct 29, 2024

@Zugschlus

The title is "fputsx is not round-trip safe with backslash-ending usernames". It doesn't say anything about backslash-containing usernames.

alejandro-colomar added a commit to alejandro-colomar/shadow that referenced this issue Nov 1, 2024
It seems they never worked correctly.  Let's keep it simple and remove
support for escaped newlines.

Closes: <shadow-maint#1055>
Reported-by: Chris Hofstaedtler <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
alejandro-colomar added a commit to alejandro-colomar/shadow that referenced this issue Nov 1, 2024
It seems they never worked correctly.  Let's keep it simple and remove
support for escaped newlines.

Closes: <shadow-maint#1055>
Reported-by: Chris Hofstaedtler <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
alejandro-colomar added a commit to alejandro-colomar/shadow that referenced this issue Nov 2, 2024
It seems they never worked correctly.  Let's keep it simple and remove
support for escaped newlines.

Closes: <shadow-maint#1055>
Reported-by: Chris Hofstaedtler <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants