Fix buffer overflow in chat server example

[Ralith]

message, which contains at most 2048 bytes, was previously copied after a prefix into message2, which has a size of 2048 bytes, with no bounds checking. This could result in a stack overflow of strlen("Server: ") bytes.

[alliekins]

Please update to strncpy() as @phoem suggested. If we're fixing buffer overflows we might as well go all the way.

[BrianHook]

strncpy is insufficient however. strncpy_s at the very least.

Brian Hook
Audio Engineering Lead
Oculus VR

On Tue, Jul 8, 2014 at 2:19 PM, Alex Howland notifications@github.com
wrote:

Please update to strncpy() as @phoem https://github.com/phoem
suggested. If we're fixing buffer overflows we might as well go all the way.

—
Reply to this email directly or view it on GitHub
#1 (comment).

[alliekins]

strncpy_s is MS only though. RakNet is cross-plat.

[BrianHook]

I would recommend that you just grab the source to a safe string copy from
OpenBSD and use that (modulo license issues) and all your xplat issues go
away. This is particularly important for networking code for obvious
reasons.

Brian Hook
Audio Engineering Lead
Oculus VR

On Tue, Jul 8, 2014 at 5:22 PM, Alex Howland notifications@github.com
wrote:

strncpy_s is MS only though. RakNet is cross-plat.

—
Reply to this email directly or view it on GitHub
#1 (comment).

[Ralith]

I left the initial strcpy in-place as it is obviously safe due to the static source and desitnation size and the static source data, and to minimize the impact of the change. Is it desirable to replace all uses of strcpy solely for consistency/maintenance reasons?

It's not clear to me what the benefit of using the MSVC-specific strncpy_s would be, nor what other alternatives might be so preferable that such a change would be the "very least" acceptable alternative. Could you elaborate on how strncpy is insufficient? Unlike the previous code, it can be used safely in this situation.

I've corrected a typo in the original PR.

[BrianHook]

strcpy can be safe when you look at each instance and verify that it's
safe, but that's not the issue. The problem is when/if something changes
and what was verified safe is now no longer safe. This is how a lot of
security vulnerabilities are introduced.

So it's good practice to just make them all buffer safe (as much as
possible) so that any later changes minimize the introduction of security
issues, and also because static analyzers tend to flip out (such as MSVC)
upon seeing 'unsafe' versions of functions used.

strncpy doesn't necessarily null terminate the string and this can
introduce buffer exploits.

Any widespread network codebase really needs buffer safe versions of
everything. Whether it's rolling your own buffer safe strncpy, wrapping
around strncpy_s/strlcpy, etc. it's a good idea to get it in place earlier
than later.

Brian Hook
Audio Engineering Lead
Oculus VR

On Tue, Jul 8, 2014 at 9:57 PM, Benjamin Saunders notifications@github.com
wrote:

I left the initial strcpy in-place as it is obviously safe due to the
static source and desitnation size and the static source data, and to
minimize the impact of the change. Is it desirable to replace all uses of
strcpy solely for consistency/maintenance reasons?

It's not clear to me what the benefit of using the non-portable strncpy_s
would be, nor what other alternatives might be so preferable that such a
change would be the "very least" acceptable alternative. Could you
elaborate on how strncpy is insufficient? Unlike the previous code, it
can be used safely in this situation.

I've corrected a typo in the original PR.

—
Reply to this email directly or view it on GitHub
#1 (comment).

[Ralith]

I've dropped in strncpy as per discussion. I'll leave the addition of strlcpy, with proper measures to ensure linking on BSDs is not interfered with, to another PR. OpenBSD's libc is, of course, BSD-licensed, so its implementation should suit.

[phoem]

What @BrianHook said: just because it fits now doesn't mean it will still fit later on when a change is made.

[alliekins]

I'm going to pull this in now, but we can talk about strncpy replacements for the future. Right now this quickly fixes an existing vulnerability. We can make this fix more proactive in the future, but right now the fix is what matters.