Bug 2233204

Thank you for the patch. I did not have time to look into this issue yet, but to me this doesn't look like a regression. The variable expansion is to my eyes an unintended behavior, it doesn't seem to be like a feature someone intentionally implemented, therefore the upstream didn't really do anything to implement such behavior when transitioning to the new systemd way of starting Tigervnc. Also, this will always expand to the same user starting the session, right? You already have to add this user to "/etc/tigervnc/vncserver.users" config file so you can also add this user to the list of "PlainUsers" in the main config file. Lastly, we didn't drop the "vncserver" script so you can keep using it as before.

(In reply to Jan Grulich from comment #2)
> You already have to add this user to > "/etc/tigervnc/vncserver.users"
> config file so you can also add this user to > the list of "PlainUsers"
> in the main config file

Yes, but supposing that /etc/tigervnc/vncserver.users contains something
like this:

    :1=vncuser1
    :2=vncuser2

Then vncserver-config-defaults would need to contain this:

    SecurityTypes=TLSPlain
    PlainUsers=vncuser1,vncuser2

So all Xvnc instances would be invoked as

    /usr/bin/Xvnc :1 -plainusers vncuser1,vncuser2 ...
    /usr/bin/Xvnc :2 -plainusers vncuser1,vncuser2

thus permitting vncuser1 to access :2 and vncuser2 to access :1, which is
a security breach.

> Lastly, we didn't drop the "vncserver" script so you can keep using it
> as before.

Sure, but it requires a custom vncserver@.service and a vncserver wrapper like
those used on RHEL 7. This leads to a solution that is incompatible with the
documented setup.

After additional reviewing I found that the actual cause of the problem is the
way /usr/bin/vncserver and /usr/libexec/vncserver (invoked by vncsession)
execute Xvnc:

    system("$cmd & echo \$! >$pidFile"); 

versus

    exec(@cmd);

In the former, expansion of environment variable, quoting and globbing will be
performed by the shell, while in the second they will not. So as an additional
disadvantage, setting a parameter in a config file as

    PlainUsers="vncuser1,vncuser2"

will pass this to Xvnc

    -plainusers "vncuser1,vncuser2"

This is the actual regression.

Created attachment 1985799 [details]
Proposed patch to fix this issue

Hi, I tried to write a solution for your issue, however, I'm not really an expert in Perl so what I wrote might be most likely simplified. Anyway, can you try the attached patch and see if it fixes your issue?

Created attachment 1985946 [details]
Patch to expand all environment variables when loading the configuration files

If you prefer to solve it in vncserver then I suggest you to use something
like this, instead, which is much simpler.

(In reply to Carlos Santos from comment #6)
> Created attachment 1985946 [details]
> Patch to expand all environment variables when loading the configuration
> files
> 
> If you prefer to solve it in vncserver 

I feel like fixing it here is more versatile.

> then I suggest you to use something like this, instead, which is much simpler.

Won't this result into passing an empty option in case the env variable is not set? Not that I can think of a case where someone would do that, but I was trying not to break any other potential use-case. But you are right that doing this in LoadConfig() makes more sense.

(In reply to Jan Grulich from comment #7)
> (In reply to Carlos Santos from comment #6)
> > Created attachment 1985946 [details]
> > Patch to expand all environment variables when loading the configuration
> > files
> > 
> > If you prefer to solve it in vncserver 
> 
> I feel like fixing it here is more versatile.
> 
> > then I suggest you to use something like this, instead, which is much simpler.
> 
> Won't this result into passing an empty option in case the env variable is
> not set? Not that I can think of a case where someone would do that, but I
> was trying not to break any other potential use-case. But you are right that
> doing this in LoadConfig() makes more sense.

We should not try to guess what the user wants to do, so I prefer to assume
that $FOO means "use the FOO environment variable, if any".

Do you want to propose your suggested change to upstream or should I do it? Anyway, this is something we can for sure fix in RHEL 8.10 and RHEL 9.4.

I will submit a PR upstream.

Done: https://github.com/TigerVNC/tigervnc/pull/1664

(In reply to Carlos Santos from comment #11)
> Done: https://github.com/TigerVNC/tigervnc/pull/1664

Thank you. I will wait for upstream response in case there are more things to add before backporting this to RHEL.

Issue migration from Bugzilla to Jira is in process at this time. This will be the last message in Jira copied from the Bugzilla bug.

This BZ has been automatically migrated to the issues.redhat.com Red Hat Issue Tracker. All future work related to this report will be managed there.

Due to differences in account names between systems, some fields were not replicated.  Be sure to add yourself to Jira issue's "Watchers" field to continue receiving updates and add others to the "Need Info From" field to continue requesting information.

To find the migrated issue, look in the "Links" section for a direct link to the new issue location. The issue key will have an icon of 2 footprints next to it, and begin with "RHEL-" followed by an integer.  You can also find this issue by visiting https://issues.redhat.com/issues/?jql= and searching the "Bugzilla Bug" field for this BZ's number, e.g. a search like:

"Bugzilla Bug" = 1234567

In the event you have trouble locating or viewing this issue, you can file an issue by sending mail to rh-issues. You can also visit https://access.redhat.com/articles/7032570 for general account information.