SLikeSoft / SLikeNet

SLikeNetâ„¢ is an Open Source/Free Software cross-platform network engine written in C++ and specifially designed for games (and applications which have comparable requirements on a network engine like games) building upon the discontinued RakNet network engine which had more than 13 years of active development.
https://www.slikenet.com/
Other
396 stars 62 forks source link

RE: using security enhanced CRT functions, replacing obsolete less secure CRT functions with up-to-date ones #19

Open pps83 opened 7 years ago

pps83 commented 7 years ago

After reading these statements I would not trust your changes more than original raknet which scores solid 0 on 0 to 10 scale. I strongly suggest you to fix these bugs: there is no "enhanced" CRT function. There are standard C functions and raknet used to use them. "obsolete less secure CRT functions" - who and since when obsoleted standard functions?!

Luke1410 commented 7 years ago

I think you misunderstood the statement. Maybe it was also not too well phrased, to begin with (allowing such misinterpretation to occur). In this case, I'm happy for any suggestions to improve the wording.

I assume you are referring to the following statement in section 1.3 ("Changes between RakNet (4.081/4.082) and SLikeNet") of the readme file: "[...] The major differences/improvements of SLikeNet over RakNet are: [...]

[...]"

First of all I've to make it clear that software security is something which we do understand is tackled and ensured by not only a single golden bullet but rather by a collection of different processes and establishing high quality standards. While we are striving towards the goal to make SLikeNet a network library which provides a top level of security, we certainly aren't there yet. This is, first of all, reflected by the fact that SLikeNet in its current version is still a development version and hasn't even reached alpha-quality-level (by our own standards) yet. Still, building upon RakNet, we honestly think that even in its current early state it's an improvement over the quality standards of RakNet.

That said, the purpose of the list in the readme file is solely meant to give examples of where SLikeNet improves things over those which are available in RakNet. No more, no less.

With regards to security, we made already some initial steps to raise the security standard of the library. The list provided here doesn't reflect all the changes done, but rather gives some examples. a) several buffer overflows in RakNet were fixed (some of which were discovered during code review and with the use of static code analysis tools; others were fixed based on pull requests and issue reports on GitHub or reported by customers) b) we decided to use the security-enhanced versions of CRT functions (see [1][2][3]) simply because they provide an additional layer of protection against common coding bugs which can lead to security relevant issues (like buffer overflows). For instance the strcpy() function does not validate whether the given destination buffer is large enough to retrieve the string copy whereas strcpy_s() takes the size of the destination buffer and therefore can check that no buffer overflow occurs (under the premission that the passed parameters are correct of course). This doesn't only have the advantage of these additional compile and runtime checks for us, but also allows easier code reviews (be it manual or automated ones) since you directly see the destination buffer size. We certainly understand that the use of these security-enhanced CRT functions can be seen controversial and there's no doubt that there are also arguments against using these. Feel free to make your point why you would advice against using these in SLikeNet, but atm we think that the advantages outweight the disadvantages (take into consideration also that several of these functions made it into the C11 standard and the old argument using these violating standard compliance no longer stands for all cases which were risen when Microsoft introduced these back in VS 2005). c) completely independent of the security-enhanced versions of CRT functions mentioned above, RakNet used some deprecated functions. One example being gethostbyname() [4]. These deprecated functions were replaced with their newer counterparts (f.e. in case of gethostbyname() we use getaddrinfo() now). Some of these deprecated functions had security related flaws. Therefore, we listed this here (f.e. gethostbyname() doesn't check the size of the name parameter and hence could trigger a heap corruption if improperly used). As said: This entry in the list of changes is completely unrelated to the previous point of the security-enhanced versions of CRT functions.

As stated above: These are just some examples of how we improved the security of RakNet in SLikeNet. There are other things we did which aren't explicitly mentioned in the readme and some of which are also done outside the code (f.e. using static code analysis, certain procedures we do as part of the release procedure, additional testruns, etc.). Also note that we just got started here, and you'll see more changes to follow which further increase the security level of SLikeNet in the upcoming versions. For example for 0.2.0 we will process more of remaining/outstanding pullrequest/issue reports and also will raise certain dependencies to resolve other known (and unknown) (security) issues.

I hope this answers your questions and that we'll be able to show you already with the next version that indeed we take the security concerns on the library seariously. If you have concrete issues you'd like us to tackle here, don't hesitate to state them. Also if I got your question wrong, don't hesitate to ask again.

[1] https://docs.microsoft.com/en-us/cpp/c-runtime-library/security-features-in-the-crt [2] https://docs.microsoft.com/en-us/cpp/c-runtime-library/security-enhanced-versions-of-crt-functions [3] http://resources.infosecinstitute.com/sdl-for-cc-code-in-visual-studio-2013-overview/#gref [4] https://msdn.microsoft.com/en-us/library/windows/desktop/ms738524(v=vs.85).aspx

pps83 commented 7 years ago

I'm aware about these "secure" CRT functions. For the last 10 years I've been disabling these warnings about secure CRT :) Regular functions are not less secure if they are used properly. I would be very surprised that C11 has any of the _s functions standardized. I would flip on the spot if any were. I would even argue that the _s functions are in fact less secure, simply because programmers are in general less familiar with these and this is a problem. RakNet is meant to be cross platform, peppering it with these _s "secure" functions would make code less clear (which in general makes it less secure as well). (Note, I didn't check actual code in SLikeNet and how it's done, it might as well be ok if it's not polluted all over with ifdefs). Regarding strcpy - personally I don't use in production code.

gethostbyname isn't about security. No modern code shouldn't use it, as it doesn't support ipv6.

I think raknet in general has lots of issues. I looked quickly once when merging some code and that code was about filetransfer or something like that in raknet and it looked like it allocates memory size that remote specified (to receive files). I might be wrong, as I didn't even try to look into that code closely, but to me it immediately looked like remote could simply say 1GB transfer and raknet would happily try to eat your ram. Note that I'm merging some old raknet code, latest versions might have such problems fixed.

Overall, I use raknet in production (millions of daily users) and I see some really bad results when network conditions are less than perfect (e.g mobiles). So, I'm trying to port some fixes that got accumulated over years and merge them with latest raknet to see if latest raknet/slikenet still have that same problem. I'll describe issues and how to reproduce them, maybe they were already fixed or maybe you could test if your version has similar problems.

There are two major bugs/complaints that I have with version of raknet that I use that triggered me to try to upgrade to latest. In my case it takes about 1.5MB for client to download from a game server before a game can be started. Let's assume that on 10Mbps connection it takes raknet about 3 seconds in my case to load these 1.5MB.

It's just unbelievable how bad raknet is. I'm just speechless. Any idea if these issues were fixed and were known?.. I'm stuck with 4.037 or something like that, and I see lots of changes were done since then in reliability layer, so I decided to give it a try and test latest if it also has the same issues before attempting to fix them.

Luke1410 commented 7 years ago

Regarding the standardization of the bounds-checking interfaces (which is the standard's way of naming these functions), see the C11 standard Annex K. strcpy_s() is covered in K.3.7.1.3 for example.

On the point of allocating unbound memory upon file transfer: I've assigned this an internal case number now for review/investigation and will get back here with our findings (please note that this might take a couple of days, since we've other priorities right now already in the pipeline / it's however scheduled atm to be done in-time for the 0.2.0 release) - case number: SLNET-126.

The report about the suboptimal network transfer behavior is indeed new to me. I'll make sure that this gets checked/investigated. Thanks for that great report, this is really a valuable pointer for us and I'll get back on that once we know a bit more about the issue - case number SLNET-127 (this also is currently scheduled for 0.2.0).

Kiddinglife commented 6 years ago

@pps83 Are you using TCPInterface ? Or raknet's UDP-based protocol?

Luke1410 commented 6 years ago

@pps83 Sorry for the delay in getting back to you. We investigated SLNET-126 now (i.e. unbound memory upon file transfers - related to FileListTransfer) and can confirm your suspicion. SLikeNet/RakNet indeed takes the incoming file size and tries to allocate the required amount of memory. This is only limited by the current max supported filesize (4 GB). While investigating this we discovered a bunch of related flaws in the current implementation. We'll see what we can do for the 0.x/1.x versions, with a more complete fix (which will require API/ABI breaking changes) to be integrated in 2.x. A mitigating factor however is that the receiver of a file explicitly sets the senders it allows to receive files from (i.e. trust). Certainly one should perform some means of verification prior to start receiving files. Still, I fully agree with you that even in this case, memory allocation should not be dictated by a peer.

We'll work on this part a bit and then look into your second report. If it's vital for you to get a solution ASAP, don't hesitate to bump the topic.

pps83 commented 6 years ago

No, it's not vital for me. From the top of my head other obvious issues I've seen in rak net:

That's why it's a solid 0 on 0 to 10 scale. IMO, for that reason FB perhaps totally abandoned it and don't even bother accepting PRs

Luke1410 commented 6 years ago

SLikeNet 0.1.1 is expected to contain a first mitigation for SLNet-126 via a newly introduced define (internal case number: SLNET-168): SLNET_MAX_RETRIEVABLE_FILESIZE by which the maximum retrieval filesize can be configured at compile time. We know this is just a small first step to correct the behavior, but thought this might be worthwhile putting quickly in the 0.1.1 version since it's something which can be done quickly.

In addition some display issues for filesizes > 2 GiB in the FileTransferSample were resolved too and are also expected to go into 0.1.1 (internal case number SLNET-167)

These changes are already available in the SVN and GitHub repositories.

A more mature solution will be checked out for the 0.2.0 version and as mentioned above a permanent solution should go in the 2.x branch at a later point.

We'll look into the second report (SLNET-127) next.

Also thanks for the other additional pointers. I just put them on the list to check out in detail (internal case number SLNET-169). Atm I'd have to postpone them unfortunately, since there's already so much on the immediate tasklist. Feel free to ping me, if I'm not getting back in a timely manner on this one (it certainly won't get forgotten however).

Luke1410 commented 6 years ago

SLikeNet 0.1.1 with the fixes for SLNET-168 and SLNET-167 has just been released.

Luke1410 commented 6 years ago

We think to have tackled all issues with regards to creating the ranges of IDs (used for ACKs/NAKs). This issue has been reassigned to SLNET-204. Fixes are already in the GitHub/SVN repositories and are expected to be shipped in the upcoming 0.1.2 version. If you like, we'd appreciate your thoughts on these changes, in case we overlooked something.

Luke1410 commented 6 years ago

SLikeNet 0.1.2 which resolves the issue SLNET-204 is available now at https://www.slikenet.com/ or here on GitHub at https://github.com/SLikeSoft/SLikeNet/releases/v.0.1.2 .