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

Don't use ' dup2(0, 3)' unconditionally #1709

Closed
alexey-tikhonov opened this issue Dec 20, 2023 · 3 comments · Fixed by #1714
Closed

Don't use ' dup2(0, 3)' unconditionally #1709

alexey-tikhonov opened this issue Dec 20, 2023 · 3 comments · Fixed by #1714
Labels
bug Something isn't working

Comments

@alexey-tikhonov
Copy link

alexey-tikhonov commented Dec 20, 2023

When a system is configured to use 'libnss_sss.so.2' as NSS provider (i.e. /etc/nsswitch.conf contains "passwd: sss files ...") this code -


- ruins internal state of 'libnss_sss.so.2'

The flow is as follows:

  • after all the fork/exec/etc, Xvnc calls one of NSS functions (most probably getpwuid(getuid)) but not necessarily);
  • this initializes NSS stack, and calls to 'libnss_sss'
  • this initializes internal data structures of 'libnss_sss', in particular opens some file descriptors
  • one of file descriptors obtained by 'libnss_sss' is 3 (first free)
  • Xvnc "hijacks" this fd with dup2(0, 3);
  • Xvnc call NSS again, 'libnss_sss' tries to work with this fd, discovers there is "something wrong", closes it (fd=3), reinits (and gets fd=3 again)
  • Xvnc keeps working with fd=3, discovers there is "something wrong", terminates

strace looks like:

fstat(3, {st_mode=S_IFSOCK|0777, st_size=0, ...}) = 0
munmap(0x7f3bf9792000, 9253600)         = 0
close(3)                                = 0
openat(AT_FDCWD, "/var/lib/sss/mc/passwd", O_RDONLY|O_CLOEXEC) = 3
...
setsockopt(3, SOL_TCP, TCP_CORK, [1], 4) = -1 ENOTSOCK (Socket operation on non-socket)

And Xvnc log:

Xvnc[76330]: XserverDesktop: client gone, sock 3
Xvnc[76330]: Selection: Remote clipboard lost, removing local ownership
Xvnc[76330]: VNCSConnST: closing [::ffff:...]::56002: read: Socket operation on non-socket (88)

To Reproduce
Configure system:

  • to use Xvnc with '-inetd'
  • nsswitch.conf to use "sss" before "files"
  • very recent SSSD (that has SSSD/sssd@9c4ac1b)

and try to login: after successful authentication session terminates.

Client (please complete the following information):
Doesn't matter.

Server (please complete the following information):
Linux/TigerVNC (Xvnc)

What is the reason to use dup2() and assign vncInetdSock=3 unconditionally instead of dup() and obtain assigned fd using F_GETFD?

Issue is worked around with the help of SSSD/sssd#7085 but I think this is a bug in TigerVNC

@CendioOssman
Copy link
Member

That code is absolutely ancient, so unfortunately you would have to ask RealVNC why it was written that way.

But I agree, using dup() seems the sensible approach here.

Have you tried changing it and see if everything seems to work?

@CendioOssman CendioOssman added the bug Something isn't working label Dec 20, 2023
@alexey-tikhonov
Copy link
Author

Have you tried changing it and see if everything seems to work?

Not yet.
I can try to craft a patch. But wanted to get feedback first, maybe this '3' is "hardcoded"/expected in other places of the code...

@CendioOssman
Copy link
Member

CendioOssman commented Dec 20, 2023

It might be, unfortunately. It shouldn't, of course, but it might. So testing will be needed.

It does set vncInetdSock, though, which hopefully everything else looks at.

dcommander added a commit to TurboVNC/turbovnc that referenced this issue Sep 17, 2024
... introduced when we migrated to the xorg-server 1.19.x code base
in TurboVNC 2.2 (a661ed1).

- Use dup() to obtain a free file descriptor for the inetd socket rather
  than statically assigning FD 3.  Otherwise, if Xvnc calls a function
  (such as getpwuid()) that invokes libnss_sss prior to calling
  ddxProcessArgument(), then libnss_sss will obtain the first free file
  descriptor (3) for its own purposes, ddxProcessArgument() will clobber
  that file descriptor, and hilarity will ensue.  TurboVNC doesn't
  currently make an early call to a function that invokes libnss_sss,
  but referring to TigerVNC/tigervnc#1709,
  TigerVNC does.  It is possible that we will do so in the future.

  Based on:
  TigerVNC/tigervnc@7a9773a

- Explicitly redirect stderr (FD 2) to /dev/null rather than simply
  closing it.  Otherwise, OsInit() (more specifically ospoll_create())
  will obtain the first free file descriptor (2) to use for polling,
  then it will check whether FD 2 (which it assumes is still stderr) is
  writable.  Since polling file descriptors aren't writable, OsInit()
  will redirect FD 2 to /dev/null, thus breaking the polling subsystem.
  (Symptomatically, that caused the TurboVNC Server, when used with the
  -inetd option, to do nothing when a VNC viewer connected.)

  Based on:
  TigerVNC/tigervnc@712cf86

- Move the inetd RFB connection code from ProcessInputEvents() to the
  end of InitInput(), since ProcessInputEvents() isn't usually called
  until/unless a window manager is started.  (Symptomatically, that also
  caused the TurboVNC Server, when used with the -inetd option, to do
  nothing when a VNC viewer connected.)

Probably fixes #303
dcommander added a commit to TurboVNC/turbovnc that referenced this issue Sep 17, 2024
... introduced when we migrated to the xorg-server 1.19.x code base
in TurboVNC 2.2 (a661ed1).

- Use dup() to obtain a free file descriptor for the inetd socket rather
  than statically assigning FD 3.  Otherwise, if Xvnc calls a function
  (such as getpwuid()) that invokes libnss_sss prior to calling
  ddxProcessArgument(), then libnss_sss will obtain the first free file
  descriptor (3) for its own purposes, ddxProcessArgument() will clobber
  that file descriptor, and hilarity will ensue.  TurboVNC doesn't
  currently make an early call to a function that invokes libnss_sss,
  but referring to TigerVNC/tigervnc#1709,
  TigerVNC does.  It is possible that we will do so in the future.

  Based on:
  TigerVNC/tigervnc@7a9773a

- Explicitly redirect stderr (FD 2) to /dev/null rather than simply
  closing it.  Otherwise, OsInit() (more specifically ospoll_create())
  will obtain the first free file descriptor (2) to use for polling,
  then it will check whether FD 2 (which it assumes is still stderr) is
  writable.  Since polling file descriptors aren't writable, OsInit()
  will redirect FD 2 to /dev/null, thus breaking the polling subsystem.
  (Symptomatically, that caused the TurboVNC Server, when used with the
  -inetd option, to do nothing when a VNC viewer connected.)

  Based on:
  TigerVNC/tigervnc@712cf86

- Move the inetd RFB connection code from ProcessInputEvents() to the
  end of InitInput(), since ProcessInputEvents() isn't usually called
  until/unless a window manager is started.  (Symptomatically, that also
  caused the TurboVNC Server, when used with the -inetd option, to do
  nothing when a VNC viewer connected.)

Probably fixes #303
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants