some RFB 3.3 compatibility and DES auth by AndrianBdn · Pull Request #160 · any1/neatvnc

AndrianBdn · 2026-01-25T07:04:29Z

This PR introduces some RFB < 3.8 compatibility (primarily at handshake time) and DES auth.
Because of the way DES auth work, nvnc_auth_fn had to be updated too - which brings some minor changes to other auth implementations.

Additionally, because of some prior incidents at code review level with auth-related bugs, I decided to implement some functional tests. Functional tests now use special harness-binary rfb-test-server (based on examples) and few hundred lines of stdlib-only python code (because it is a bit nicer to write tests in).

We now test: handshake, none, DES and some vencrypt (just to test something from 3.8).

Feedback is welcome. I haven't opened a wayvnc PR yet since it depends on these changes.

I've read and understood CONTRIBUTING.md.

any1 · 2026-01-26T22:39:20Z

Thanks for the contribution. I'll be a bit busy the next couple of weeks. I'll give this a look when I have the time.

any1

This does actually look neat enough.

Did you see #149 and #146?

AndrianBdn · 2026-02-08T11:45:45Z

Thanks for the positive feedback @any1 I am happy there is a way to get this merged.

Would you prefer a new PR with logically re-organized commits or few smaller separate PRs? Perhaps one PR with smaller fixes + one for DES auth?

Re #146 - I feel it would take too long to make it fully working and I personally see more value in supporting legacy protocols that will work in many clients. In many cases weak authorization and no encryption is not an issue (similar to Basic HTTP auth) - ssh and vpn are sufficient already. To be honest only reason I bother with auth at all, because Apple client cannot skip it entirely.

Re #149 - I saw it later. It feels a bit abandoned and I don't need to bring complete DES implementation to the source tree.

any1 · 2026-02-08T12:01:23Z

Would you prefer a new PR with logically re-organized commits or few smaller separate PRs? Perhaps one PR with smaller fixes + one for DES auth?

You can make one PR for miscellaneous improvements and fixes and keep this one for the DES stuff, but I would suggest changing the name to "Add DES Authentication" or something like that.

Re #146 - I feel it would take too long to make it fully working and I personally see more value in supporting legacy protocols that will work in many clients. In many cases weak authorization and no encryption is not an issue (similar to Basic HTTP auth) - ssh and vpn are sufficient already. To be honest only reason I bother with auth at all, because Apple client cannot skip it entirely.

Fair enough. I really wanted to keep out worst legacy bits for sake of keeping things neat, but for better or worse, RFB is a legacy protocol.

Re #149 - I saw it later. It feels a bit abandoned and I don't need to bring complete DES implementation to the source tree.

I definitely do not want a whole DES implementation in there.

AndrianBdn · 2026-02-26T04:17:41Z

Hey @any1 - I have updated the PR and PR description. Please take a look when you will have time.

any1 · 2026-02-26T09:28:50Z

I should have mentioned this earlier, but all cryptography dependencies (except gnutls) are wrapped inside src/crypto. The goal here is to make it easy to implement the same cryptography functions using a different library such as openssl.

AndrianBdn · 2026-02-26T14:28:32Z

@any1 moved to src/crypto

any1

I like that you wrote tests.

I'd rather have the first commit squashed with the second one instead of having a broken commit in there. Ideally, their order should be swapped, but I understand if you don't have the patience for such git gymnastics. It is easier to review PRs when commits are logically ordered, each commit changing only "one thing" and not breaking anything.

any1 · 2026-03-03T20:22:10Z

+{
+	nvnc_log(NVNC_LOG_WARNING, "Connection rejected: %s", reason_string);
+
+	return security_send_failure(client, RFB_SECURITY_TYPE_INVALID,


RFB_SECURITY_TYPE_INVALID is not a result code.

I get what you're doing now, but it's confusing because it conflates two different message types.

This would probably be clearer if these functions had duplicate code. It's not as if it's a lot of code anyway.

It might even be better to inline this function.

any1 · 2026-03-03T20:47:09Z

+#endif
+
+		if (server->auth_flags & NVNC_AUTH_REQUIRE_AUTH) {
+			security_type_invalid(client,


In this situation, 3.3 is an unsupported version, so it should be treated as such, by calling handle_unsupported_version.

3.3 protocol can't handle "unsupported version" message see https://github.com/rfbproto/rfbproto/blob/master/rfbproto.rst#security

The server decides the security type and sends a single word... A value of 0 means that the connection has failed and is followed by a string giving the reason...

The difference is subtle:
3.7+ will send U8 with 0 and then U32 len of reason and reason
3.3 will send U32 with all zeroes, then U32 len of reason and reason

If you want - i can do 3.7 way here - we are going to close the connection anyway. Probably clients will show just a bit more obscure error.

Well, it's better to give a good error if possible, so this is functionally correct.

any1 · 2026-03-04T11:39:13Z

+int security_type_invalid(struct nvnc_client* client,
+		const char* reason_string)
+{
+	nvnc_log(NVNC_LOG_WARNING, "Connection rejected: %s", reason_string);


Nit: This should be an info-level message.

AndrianBdn · 2026-03-06T15:10:10Z

Hey @any1 - thanks for your review and your patience with this. I squashed first two commits as you suggested. Also I fixed everything and wrote new tests covering RFB 3.7 RFC compliance.

any1 · 2026-03-06T17:39:57Z

Can you squash fixups into the individual commits that they're supposed to fix? Except that one that I commented on, of course. That whole commit can be dropped.

AndrianBdn · 2026-03-07T04:46:55Z

@any1 How would you like the commits to tell the story? We could split it into three commits: DES (unreachable), RFB 3.3/3.7, and tests. Or do the broader RFB 3.3/3.7 work first (excluding DES), then DES, then tests. TDD-style with failing tests first would also work.

any1 · 2026-03-07T09:48:25Z

@any1 How would you like the commits to tell the story? We could split it into three commits: DES (unreachable), RFB 3.3/3.7, and tests. Or do the broader RFB 3.3/3.7 work first (excluding DES), then DES, then tests. TDD-style with failing tests first would also work.

The best order would be:
DES (not unreachable, it is a valid security type for 3.8), RFB 3.3/3.7, then tests.

Thank you.

Implement DES challenge-response auth for RFB 3.8 clients. Refactor auth callback to use nvnc_auth_creds struct with accessor functions. Requires NVNC_AUTH_ALLOW_BROKEN_CRYPTO flag.

Parse RFB version with sscanf instead of exact string match. Extract on_version_message_rfb33() for RFB 3.3 clients (U32 security type). Send SecurityResult reason string only for RFB 3.8+.

14 Python tests covering DES auth (RFB 3.3, 3.7, 3.8) and VeNCrypt X509_PLAIN auth, using a minimal rfb-test-server.

AndrianBdn · 2026-03-08T04:47:15Z

done, rearranged in 3 commits; I run my tests after commit 1 (DES, only subset of tests) and commit 2 (DES + RFB compatibility).

any1 · 2026-03-08T09:58:32Z

+#endif
+
+	if (server->auth_flags & NVNC_AUTH_REQUIRE_AUTH) {
+		security_handshake_failed(client, NULL,


This was supposed to be the sec type plus reason string but inlined, but that can be fixed later. It's good enough for now.

any1 · 2026-03-08T10:02:04Z

Thanks!

I wouldn't object to having tests like this for attempted authentication bypasses. I.e. if a client tries to select an unsupported security type after receiving the list of types.

any1 · 2026-03-08T10:07:43Z

Oops, I forgot, it would have been good to have the changes for wayvnc ready before merging this. Can you make a PR for that soon?

AndrianBdn · 2026-03-08T16:08:35Z

@any1 thanks - appreciate your patience. I am glad we've got this merged!

I'll open the wayvnc PR shortly. I already have a working version that I’ve been using to test neatvnc.

any1 reviewed Feb 7, 2026

View reviewed changes

Comment thread src/enc/util.c Outdated

Comment thread src/enc/zrle.c

Comment thread src/server.c

Comment thread src/server.c Outdated

Comment thread meson.build Outdated

Comment thread include/neatvnc.h Outdated

any1 mentioned this pull request Feb 20, 2026

server: Normalize pixel format depth from client #165

Merged

AndrianBdn mentioned this pull request Feb 21, 2026

Misc fixes: ZRLE buffer overflow, log typo #166

Merged

AndrianBdn force-pushed the legacy-vnc-auth branch from 4cac5b6 to b44b93f Compare February 26, 2026 04:06

AndrianBdn changed the title ~~Add macOS Screen Sharing compatibility~~ some RFB 3.3 compatibility and DES auth Feb 26, 2026

AndrianBdn requested a review from any1 February 26, 2026 04:17

any1 reviewed Mar 3, 2026

View reviewed changes

any1 reviewed Mar 4, 2026

View reviewed changes

AndrianBdn force-pushed the legacy-vnc-auth branch from 5e00683 to a25548e Compare March 6, 2026 13:29

AndrianBdn requested a review from any1 March 6, 2026 15:10

AndrianBdn added 3 commits March 8, 2026 08:38

Add DES authentication (VNC security type 2)

e52f856

Implement DES challenge-response auth for RFB 3.8 clients. Refactor auth callback to use nvnc_auth_creds struct with accessor functions. Requires NVNC_AUTH_ALLOW_BROKEN_CRYPTO flag.

Add RFB 3.3/3.7 protocol support

693fdbc

Parse RFB version with sscanf instead of exact string match. Extract on_version_message_rfb33() for RFB 3.3 clients (U32 security type). Send SecurityResult reason string only for RFB 3.8+.

test: Add RFB handshake and auth tests

dffc44a

14 Python tests covering DES auth (RFB 3.3, 3.7, 3.8) and VeNCrypt X509_PLAIN auth, using a minimal rfb-test-server.

AndrianBdn force-pushed the legacy-vnc-auth branch from a25548e to dffc44a Compare March 8, 2026 04:43

any1 reviewed Mar 8, 2026

View reviewed changes

any1 merged commit 57e9572 into any1:master Mar 8, 2026

nob13 mentioned this pull request Mar 12, 2026

Draft: Simple VNC Auth #149

Closed

Uh oh!

Conversation

AndrianBdn commented Jan 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

any1 commented Jan 26, 2026

Uh oh!

any1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AndrianBdn commented Feb 8, 2026

Uh oh!

any1 commented Feb 8, 2026

Uh oh!

AndrianBdn commented Feb 26, 2026

Uh oh!

any1 commented Feb 26, 2026

Uh oh!

AndrianBdn commented Feb 26, 2026

Uh oh!

any1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

any1 Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

any1 Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

any1 Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

AndrianBdn Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

any1 Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

any1 Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

AndrianBdn commented Mar 6, 2026

Uh oh!

any1 commented Mar 6, 2026

Uh oh!

AndrianBdn commented Mar 7, 2026

Uh oh!

any1 commented Mar 7, 2026

Uh oh!

AndrianBdn commented Mar 8, 2026

Uh oh!

any1 Mar 8, 2026

Choose a reason for hiding this comment

Uh oh!

any1 commented Mar 8, 2026

Uh oh!

any1 commented Mar 8, 2026

Uh oh!

AndrianBdn commented Mar 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

AndrianBdn commented Jan 25, 2026 •

edited

Loading