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

Allow to disable sending unneeded user data to the server #3582

Closed
allo- opened this issue Jul 16, 2022 · 15 comments
Closed

Allow to disable sending unneeded user data to the server #3582

allo- opened this issue Jul 16, 2022 · 15 comments
Labels
enhancement New feature or request security

Comments

@allo-
Copy link

allo- commented Jul 16, 2022

When connecting to a server, the client sends personal data that is not needed for the connection, like username, realname of the user (as in /etc/passwd) and hostname of the client machine (sent by the client, not determined using rDNS) which is also logged on the server side depending on the log settings.

It would be nice to have a switch to control how much information is sent. Other information that may not be strictly necessary (but are possibly used to determine compatiblity) would be the used Linux distributions and it seemingly different machine ids.

@allo- allo- added the enhancement New feature or request label Jul 16, 2022
@totaam
Copy link
Collaborator

totaam commented Jul 17, 2022

Great minds think alike!
I already had this issue right at the top of my TODO list after editing https://github.com/Xpra-org/xpra/blob/master/docs/Usage/Security.md

So the commit above is just a start.
What I want to do is review every bit of data that is being exchanged and decide what to do with it:

  • just remove it - which we may be able to do in some cases
  • keep it as it is - if we need it for something
  • gate it with an environment variable (as per the commit - which is disabled by default for now)
  • only expose the information to authenticated users: connections that have an authentication module

I am running with XPRA_FULL_INFO=0 locally for now and not getting any problems - yet?
I haven't even looked at the username or client hostname yet.

@allo-
Copy link
Author

allo- commented Jul 17, 2022

I am running with XPRA_FULL_INFO=0 locally for now and not getting any problems - yet?

I didn't know this variable. I found the OBFUSCATE variable for debug logs, but it seems only to obfuscate machine info but not user info. Trying to find stuff I returned "" in info.py, but didn't yet find out where the hostname comes from. And in info.py returning empty strings is the fallback for non-posix systems and I have no idea what will happen when empty strings are returned for other information, which may be expected by the server. I guess some may even crash the server-side if one just patches stuff to remove information. On the other hand it would be easy to send a dummy hostname to avoid breaking older server versions.

From the security point of view negotiation what to send could also allow downgrade attacks. Let's say an empty hostname is sent when the server is newer than version X, then a malicious server could pretend to be an older version (or the user could just install some older version) to get more client information.

@totaam
Copy link
Collaborator

totaam commented Jul 17, 2022

I didn't know this variable.

It was just added in the commit above.

I guess some may even crash the server-side if one just patches stuff to remove information

No, that's extremely unlikely. The worst that can happen is that the server may reject the connection if it cannot proceed without it somehow.

then a malicious server could pretend to be an older version to get more client information

That's not how it is going to work: in all the cases relevant to this ticket, what is going to be exposed will be decided exclusively by the client or server, without knowledge of the peer's version or requirements.

totaam added a commit that referenced this issue Jul 19, 2022
(still requires XPRA_FULL_INFO=0 for now)
@totaam
Copy link
Collaborator

totaam commented Jul 19, 2022

Following the commits above, things are a lot better - though one still needs to run with XPRA_FULL_INFO=0 to see any difference, for now.
I am reluctant to make this the new default because a huge number of bug reports would end up being useless to me without this information... I stopped counting the number of times people didn't volunteer some weird bit of information that I ended up digging from the logs.

To see the capabilities exchanged between server and client, run both with: XPRA_LOG_HELLO=1 xpra ..


Still TODO:

  • username - it is difficult to know when it should be sent to the server in advance, I'm not at all sure that the authentication modules could all handle a username set after the challenge is sent
  • audio mixins still need trimming down

totaam added a commit that referenced this issue Jul 20, 2022
totaam added a commit that referenced this issue Jul 20, 2022
totaam added a commit that referenced this issue Jul 21, 2022
totaam added a commit that referenced this issue Jul 21, 2022
totaam added a commit that referenced this issue Aug 16, 2022
totaam added a commit that referenced this issue Aug 21, 2022
XPRA_FULL_INFO is now an integer,
0 means minimal data
1 is the default
2 includes everything we have available
@totaam
Copy link
Collaborator

totaam commented Aug 21, 2022

Now with fine grained filtering: 2a5e368

This will do.

@totaam totaam closed this as completed Aug 21, 2022
@allo-
Copy link
Author

allo- commented Aug 21, 2022

What about adding a commandline/config option for this?

@totaam
Copy link
Collaborator

totaam commented Aug 21, 2022

What about adding a commandline/config option for this?

Not at present. There are too many options as it is!
Feel free to submit a PR though.
Including man page updates, command line, etc...

totaam added a commit that referenced this issue Aug 29, 2022
@allo-
Copy link
Author

allo- commented Sep 23, 2022

It seems that the username is still transmitted.

Hand schake complete; enabling connection
(...)
PyGTK version 4
   as 'USERNAME'
setting key repeat rate (...)

where USERNAME is the username on the client PC. The log fragment looks like a line before was removed, but the actual line containing the username is still present (and obviously the client transmitted the username).

I'm using XPRA_FULL_INFO=0 xpra (...) on the command line of the client.

@totaam
Copy link
Collaborator

totaam commented Sep 23, 2022

The username is used for authentication purposes.

@allo-
Copy link
Author

allo- commented Sep 23, 2022

Can it be disabled when the authentication is done via ssh? The server could reject connections that do not provide a username if an additional method is configured.

I would even assume it would be more useful to use the remote-username, like

client-user@client-host$ ssh remote-user@remote-host

tries to authenticate as remote-user and not as client-user.

@allo-
Copy link
Author

allo- commented Sep 23, 2022

xpra attach --ssh=/usr/bin/ssh ssh:remote-user@remote-host actually fixes the issue. I wonder if it is a bug altogether to send the local username when using ssh (with a remote username configured in .ssh/config).

@allo-
Copy link
Author

allo- commented Sep 23, 2022

I think the bug is the following:

  • xpra uses by default a python ssh implementation
  • using --ssh=/path/to/ssh one can use the system ssh, which allows more advanced ssh configurations (ssh-keys, configured usernames, ciphers, etc.)
  • using --ssh can change the login username (to one configured in .ssh/config), but xpra does not know that the external program is using another username and sets the username from the ssh:user@host part or as fallback the local user as username variable.

I wonder if the effective username on the remote host can't be determined after the ssh login, no matter if it was using the python implementation or a system ssh program.

@totaam
Copy link
Collaborator

totaam commented Sep 23, 2022

which allows more advanced ssh configurations (ssh-keys, configured usernames, ciphers, etc.)

All of those are also available with paramiko.

@allo-
Copy link
Author

allo- commented Sep 23, 2022

I think I started to use system ssh, when I had to connect with a rather complex setup using a jumphost for a network that doesn't have direct access. I am not sure if paramiko supports that.

Anyway, when using --ssh, the xpra server doesn't see the correct username when USER remoteuser is configured in the .ssh/config file, which could also affect other authentication methods (and possibly prevent a login?).

I wonder if a ssh connection should transmit a username at all or rely on reading the effective username from the remote shell instead of having the client transmit what it thinks what the username is.
After all, my client managed to send another username than the remote host actually uses. I think it is probably not a security issue and may only prevent logins but not allow unauthenticated logins, but still the server trusts the client to submit a correct username when the client may login using one name and transmit another name.

What can happen is that the client causes a wrong log entry.

  • Create a local user "alice"
  • Create a .ssh/config containing User bob for her (having the credentials to login as remote user bob)
  • run as user alice xpra --ssh=/usr/bin/ssh ssh:remotehost to login as bob
  • The log contains New unix-domain connection received (...) as 'alice' even when the remote host does not have a user alice.

(By patching the client one can probably skip the step to create a local user. This configuration is just how it happened accidentally on my machine)

@totaam
Copy link
Collaborator

totaam commented Sep 24, 2022

This ticket is closed and will not be worked on, please file a separate ticket and make sure you understand what the log messages mean.

@Xpra-org Xpra-org locked as off-topic and limited conversation to collaborators Sep 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request security
Projects
None yet
Development

No branches or pull requests

2 participants