RE: Neat VNC Security Vulnerability

[Dane Bouchie <dbouchie () iradimed com>]

The client chooses the security type, so they can pass in "None" to the switch statement. is_allowed_security_type() 
now prevents that.

Dane Bouchie | Software Engineer
dbouchie () iradimed com | P 407.677.8022, 170
1025 Willa Springs Drive, Winter Springs, FL  32708
Iradimed.com


-----Original Message-----
From: Solar Designer <solar () openwall com> 
Sent: Friday, August 2, 2024 10:39 AM
To: Andri Yngvason <andri () yngvason is>
Cc: oss-security () lists openwall com; Dane Bouchie <dbouchie () iradimed com>; Travis Wise <travis () wavesquared 
com>; security () raspberrypi com; Simon Long <simon () raspberrypi com>; Moritz M??hlenhoff <jmm () inutil org>; 
Salvatore Bonaccorso <carnil () debian org>
Subject: Re: [oss-security] Neat VNC Security Vulnerability

[You don't often get email from solar () openwall com. Learn why this is important at 
https://aka.ms/LearnAboutSenderIdentification ]

Hi Andri,

On Thu, Aug 01, 2024 at 11:05:27PM +0000, Andri Yngvason wrote:
> It has come to my attention that there is a security vulnerability in Neat VNC.
>
> I've released a new version that fixes the vulnerability:
> https://github.com/any1/neatvnc/releases/tag/v0.8.1

Thank you very much for bringing this to oss-security!

On oss-security, we need a description of the vulnerability, not just a note that some vulnerability existed.

The release notes mention:

"The vulnerability was reported by Dane Bouchie and Travis Wise."

Dane and/or Travis, maybe you can provide the missing detail here?

The fix commit appears to be:

Add sanity check for chosen security type
https://github.com/any1/neatvnc/commit/cc71650a69abc2573a0d96d082409d2468802d47

Skimming it, I see it increases the size of buf in on_version_message() from 3 to 32 security types.  That function is 
also split in two and otherwise refactored.  A number of "security->types[security->n++]"
lines got replaced with ADD_SECURITY_TYPE(), which has an assert() against the new maximum.  There were 4 of those 
lines, so I can see how a buffer for 3 could be too small.  So there was a bug.  However, none of this looks like 
attacker-controlled input, or is it?  Now, even without attacker-controlled input an out-of-bounds write is undefined 
behavior, so theoretically could result in an exploitable vulnerability via some other correctly processed input, but 
was there any analysis whether it commonly or realistically does in this case?

I see the commit also adds an is_allowed_security_type() check to the beginning of on_security_message().  However, the 
rest of that function only has a switch statement covering a few known security types and:

        default:
                security_handshake_failed(client, NULL,
                                "Unsupported security type");

The action on failing the added pre-check is almost the same.  So I don't immediately see how not pre-checking could 
have been problematic.

Maybe I'm missing some bigger issue also fixed by those changes?

Don't get me wrong, fixing a bug and defensive programming is great, but we also need the security impact documented.

Alexander