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

[zeha]

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]

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]

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]

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]

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?

https://github.com/shadow-maint/shadow/blob/f3f501c81c1f5fd253bf0d243971cd6868a2d89e/lib/chkname.c#L60

[zeha]

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]

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]

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

Yes.

[alejandro-colomar]

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.)