Skip to content

Feat: Peer Id Checking - Fixes CVE-2021-42072 and CVE-2021-42073#7931

Merged
nbolton merged 7 commits intomasterfrom
peerbase
Feb 17, 2025
Merged

Feat: Peer Id Checking - Fixes CVE-2021-42072 and CVE-2021-42073#7931
nbolton merged 7 commits intomasterfrom
peerbase

Conversation

@sithlord48
Copy link
Copy Markdown
Member

@sithlord48 sithlord48 commented Nov 28, 2024

Based on #8195

The final part that fixes: #7806 ( It may solve the sonar hotspot as we have a mode where both clients and server keys are stored so they can say their name is whatever but w/o the key they must be readded again as the key changed)

Recreates : debauchee#1346 (and more)
From that PR

This PR implements client identity verification. Essentially server gets the same process of accepting and rejecting clients like the clients can accept or reject the server. This is important because even though the client can't move the mouse on the server, it can still receive input and potentially set the clipboard.

This PR fixes the following security vulnerabilities:

  • CVE-2021-42072 server does not verify client identity (certificate fingerprint)
  • CVE-2021-42073 By guessing/listening in on valid client names server clipboard content can be manipulated.
  • Work with Peer ID Check OFF
  • Works with Peer ID Check ON

In Addition:

  • Modify the fingerprint dialog a bit so we can look at the local keys when pairing.

Current local fingerprint dialog header text
Local computer's fingerprints

@sithlord48 sithlord48 self-assigned this Nov 28, 2024
@sithlord48 sithlord48 requested a review from nbolton November 28, 2024 13:26
@sithlord48 sithlord48 changed the title Peerbase Peer checking base changes Nov 29, 2024
@sithlord48 sithlord48 marked this pull request as draft November 29, 2024 14:24
@sithlord48 sithlord48 added this to the v2.0 milestone Nov 29, 2024
@sithlord48 sithlord48 changed the title Peer checking base changes Feat: Peer Id Checking Nov 29, 2024
@sithlord48 sithlord48 force-pushed the peerbase branch 2 times, most recently from 14582bd to a32234d Compare December 1, 2024 13:20
@sithlord48 sithlord48 force-pushed the peerbase branch 8 times, most recently from 3b89576 to a21bda0 Compare December 22, 2024 15:05
@sithlord48 sithlord48 force-pushed the peerbase branch 7 times, most recently from c26a152 to 9599860 Compare December 30, 2024 12:38
@sithlord48 sithlord48 added the ✨ enhancement A suggested change to improve functionality label Jan 4, 2025
@sithlord48 sithlord48 force-pushed the peerbase branch 4 times, most recently from acaaec2 to c94b171 Compare January 8, 2025 01:36
@sithlord48 sithlord48 requested a review from nbolton February 12, 2025 16:43
@sithlord48 sithlord48 marked this pull request as ready for review February 12, 2025 16:43
@nbolton
Copy link
Copy Markdown
Member

nbolton commented Feb 12, 2025

Oh, its finally ready! Will try to review tomorrow.

@nbolton
Copy link
Copy Markdown
Member

nbolton commented Feb 14, 2025

I am concerned that this will confuse users. Why would they need to click this button when they are on this dialog?
image

It's likely in my experience that they will click it and think, "Hey those don't match" to which we will respond "Of course they don't match, they're not supposed to"... and we'll repeat that response until ultimately we remove the button as we tire of this discussion.

Screenshot 2025-02-14 at 10 11 06

TL;DR: People don't read things and will end up wasting our time with questions that don't make sense.

@nbolton

This comment was marked as resolved.

@sithlord48
Copy link
Copy Markdown
Member Author

sithlord48 commented Feb 14, 2025

Please make sure your using the newest version .
that dialog popup should be fixed. you can test it by removing the ~/.config/Deskflow/deskflow.pem . or with a clean install on a machine with no previous settings.

@sithlord48
Copy link
Copy Markdown
Member Author

sithlord48 commented Feb 14, 2025

I am concerned that this will confuse users. Why would they need to click this button when they are on this dialog? image

With the dialog up there is no way to see your local keys to confirm what is on the screen in the case a new server connecting to a new client both will have the dialog up showing the others keys with no way to view their own. I do not want to add the local keys into dialog as having to see them to compare can happen in cases where you are not needing to see the local ones for example a known client or sever generates new keys.

I'd welcome some better tooltip wording for the tooltip

Ideally we could tell the app when this needs to happen and have one machine show the dialog with the remote keys while the remote shows it keys on its screen for easier comparison. But since the remote machine is not even aware of the connection status is in a need auth state to show this dialog. so i opted to allow the user to open it from the remote box.

@nbolton
Copy link
Copy Markdown
Member

nbolton commented Feb 14, 2025

With the dialog up there is no way to see your local keys to confirm what is on the screen in the case a new server connecting to a new client both will have the dialog up showing the others keys with no way to view their own. I do not want to add the local keys into dialog as having to see them to compare can happen in cases where you are not needing to see the local ones for example a known client or sever generates new keys.

Which problem are we trying to solve?

  1. In case of a MITM, the user will see this dialog abruptly, and because they can't see what their existing client-side fingerprint was (to compare with the server) they may be tricked into thinking that the fingerprint hasn't changed.
  2. When the client connects and you see the dialog on the server, you cannot check your local fingerprints to ensure that the fingerprint on the client side is valid.

In both cases, I see the value of the button but I still think the wording will confuse some people.

I also think that the word "this" will cause confusion: Does "this computer" mean the client that is connecting or this current computer that I'm on? To us devs, it's obviously this current computer that I'm on but that's because we know how it works.

@sithlord48
Copy link
Copy Markdown
Member Author

With the dialog up there is no way to see your local keys to confirm what is on the screen in the case a new server connecting to a new client both will have the dialog up showing the others keys with no way to view their own. I do not want to add the local keys into dialog as having to see them to compare can happen in cases where you are not needing to see the local ones for example a known client or sever generates new keys.

Which problem are we trying to solve?

1. In case of a MITM, the user will see this dialog abruptly, and because they can't see what their existing client-side fingerprint was (to compare with the server) they may be tricked into thinking that the fingerprint hasn't changed.

2. When the client connects and you see the dialog on the server, you cannot check your local fingerprints to ensure that the fingerprint on the client side is valid.

In both cases, I see the value of the button but I still think the wording will confuse some people.

I also think that the word "this" will cause confusion: Does "this computer" mean the client that is connecting or this current computer that I'm on? To us devs, it's obviously this current computer that I'm on but that's because we know how it works.

Yes lets word it better

@nbolton

This comment was marked as resolved.

@sithlord48
Copy link
Copy Markdown
Member Author

sithlord48 commented Feb 14, 2025

When I accept the dialog on the client, I see the usual message to add the client:
image

Edit Solved this by adding a bool to the method that makes the dialog

  • When False (the default) we show the above dialog.
  • When True, (only when tls enabled and require peer certificates) we show a simple information box saying the new client has to be added to the screen layout, When the user closes that dialog they are taken to the add screens dialog.

Copy link
Copy Markdown
Member

@nbolton nbolton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works well. Nits aside, I'm concerned about a couple of things.

…client needs to be put in the layout when both tls is enabled and peer key checking is enabled , otherwise show the old dialog asking to allow the client to connect
@nbolton
Copy link
Copy Markdown
Member

nbolton commented Feb 18, 2025

/tip $100 @sithlord48

@algora-pbc
Copy link
Copy Markdown

algora-pbc bot commented Feb 18, 2025

🎉🎈 @sithlord48 has been awarded $100! 🎈🎊

@nbolton
Copy link
Copy Markdown
Member

nbolton commented Mar 28, 2025

@sithlord48 The milestone set for this PR (2.0.0) may be causing some confusion. We should review what issues/PRs are in 2.0.0 milestone.

@sithlord48
Copy link
Copy Markdown
Member Author

This is a goal we need to completed before 2.0.0 can be released.. We don't hold code back for a specific release we just land it when ready and our milestones act as a guide for us to know when we can say we can bump to version ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

✨ enhancement A suggested change to improve functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CVEs related to TLS and TCP connections

2 participants