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 https://github.com/OculusVR/RakNet/pull/1#issuecomment-48379058.

[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 https://github.com/OculusVR/RakNet/pull/1#issuecomment-48401235.

[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 https://github.com/OculusVR/RakNet/pull/1#issuecomment-48421455.

[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.