[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID:
<SA1PR22MB2978E752B5A2B186F0B4D9B9B5B32@SA1PR22MB2978.namprd22.prod.outlook.com>
Date: Fri, 2 Aug 2024 14:41:18 +0000
From: Dane Bouchie <dbouchie@...dimed.com>
To: Solar Designer <solar@...nwall.com>, Andri Yngvason <andri@...vason.is>
CC: "oss-security@...ts.openwall.com" <oss-security@...ts.openwall.com>,
Travis Wise <travis@...esquared.com>, "security@...pberrypi.com"
<security@...pberrypi.com>, Simon Long <simon@...pberrypi.com>, Moritz
M??hlenhoff <jmm@...til.org>, Salvatore Bonaccorso <carnil@...ian.org>
Subject: RE: Neat VNC Security Vulnerability
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@...dimed.com | P 407.677.8022, 170
1025 Willa Springs Drive, Winter Springs, FL 32708
Iradimed.com
-----Original Message-----
From: Solar Designer <solar@...nwall.com>
Sent: Friday, August 2, 2024 10:39 AM
To: Andri Yngvason <andri@...vason.is>
Cc: oss-security@...ts.openwall.com; Dane Bouchie <dbouchie@...dimed.com>; Travis Wise <travis@...esquared.com>; security@...pberrypi.com; Simon Long <simon@...pberrypi.com>; Moritz M??hlenhoff <jmm@...til.org>; Salvatore Bonaccorso <carnil@...ian.org>
Subject: Re: [oss-security] Neat VNC Security Vulnerability
[You don't often get email from solar@...nwall.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
Powered by blists - more mailing lists
Please check out the
Open Source Software Security Wiki, which is counterpart to this
mailing list.
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
