Boost logo

Boost :

Subject: Re: [boost] [new Warnings policy] MS C4180 on the Maintenance Guidelines
From: Patrick Horgan (phorgan1_at_[hidden])
Date: 2009-11-23 16:24:06

Zachary Turner wrote:
> On Mon, Nov 16, 2009 at 3:08 PM, Patrick Horgan <phorgan1_at_[hidden]> wrote:
> ... elision by Patrick ...
> C4996 Microsoft deprecated these functions on their own (and they call
>> themselves POSIX compliant), and replaced them with extremely dangerous
>> replacements that they claim are safe. The replacement functions are
>> non-POSIX and aren't available from other compilers, hence non-portable.
>> These warnings should be suppressed. I believe that you can also
>> suppress
>> by defining _CRT_NONSTDC_NO_DEPRECATE before and undef'ing after to do a
>> local suppression.
> Somewhat off topic, but why exactly are these extremely dangerous, and not
> secure?
Sorry about the delay in responding to this, but Nick Stoughton says it
much better than I do, so I was waiting for him to get back to me and
let me know if I could just quote him on it. Nicely enough, he did, so
the following is from him, it's the original liaison memo from POSIX to
C when C was considering a proposal to add these interfaces into the lib:

Nick Stoughton wrote:
> SC 22/WG 14 N1160 Austin Group Concerns on PDTR
24731 Stoughton
> 2006-02-27
> Members of the Austin Group have been reviewing the proposed Technical
> Report on "Bounds Checking Functions" over the last year, and wish to
> express their concerns over its direction.
> The proposed interfaces fail to address many of the aspects related to
> buffer overflow and as a result are only suitable for a narrow range
> of applications.
> The basic idea embodied by the proposed interface is not a new one.
> For example, the proposed strcpy_s function is similar to the strlcpy
> function of OpenBSD 2.4 (1998). However, the basic idea has not achieved
> practical consensus; on the contrary, for reasons discussed below it has
> been controversial almost since it was introduced. A Technical Report
> of type 2 does not seem warranted here: the subject is controversial
> rather than being under technical development, and mere publication of
> a TR is unlikely to further consensus.
> The core of the problem is that memory handling in C is complicated and
> error prone. Nobody doubts that improvements in the supporting APIs
> are useful. However, the existing APIs already provide all the means
> to write correct programs, although it is often cumbersome to do so.
> The proposed interfaces don't change that and, to the contrary, can
> make programs even more complex. A better solution would be to take
> the memory handling off the hands of the programmer as much as possible.
> Let's look at the string functions first. Obviously, code like
> void f(char *t, const char *s1, const char *s2) {
> strcpy(t, s1);
> strcat(t, s2);
> }
> is bad. But it is not written this way because it is impossible to
> write correct code. Obviously the length of the target buffer can be
> passed, though this alters the ABI (see below):
> void f(char *t, size_t tlen, const char *s1, const char *s2) {
> if (strlen(s1)+strlen(s2) >= tlen) abort();
> strcpy(t, s1);
> strcat(t, s2);
> }
> This is cumbersome to write and slow which is why programmers don't
do it.
> But, more importantly, this kind of change cannot retroactively made
> because it changes both the API and ABI. New interfaces would have to be
> introduced (give the new function a different name) but then one might
> as well write a better function than this. If f() is in a third party
> library, it cannot change without all the customers of the library
> changing and recompiling/relinking their applications.
> The version using the proposed interfaces has exactly the same problem.
> void f(char *t, rsize_t tlen, const char *s1, const char *s2) {
> if (strcpy_s(t, tlen, s1) != 0 || strcat_s(t, tlen, s2) != 0)
> abort();
> }
> If anything, this code is even less obvious then the previous version
> even though it is likely a bit faster.
> Even using the exception handler to provide the abort requires the ABI
> to change:
> void f(char *t, rsize_t tlen, const char *s1, const char *s2) {
> (void) set_constraint_handler_s(abort_handler_s);
Page 1 of 4
> strcpy_s(t, tlen, s1);
> strcat_s(t, tlen, s2);
> }
> Another problem with respect to the string functions being used to fix up
> existing code can be demonstrated with this code sequence:
> char *p = malloc (3 * NAME_LEN);
> strcpy (p, name1);
> strcat (p, name2);
> strcat (p, name3);
> All too often fixed values like NAME_LEN are used which are the basis for
> overflows. A programmer could certainly use
> char *p = malloc(strlen(name1) + strlen(name2) + strlen(name3) + 1);
> if (p != NULL)
> {
> strcpy (p, name1);
> strcat (p, name2);
> strcat (p, name3);
> }
> but this is once again cumbersome and therefore won't be used. Now
> assume the new string functions. The code to correctly handle
> the code (more correct than either of the previous two code sequences,
> this is the goal) could look something like this:
> rsize_t len = 3 * NAME_LEN;
> char *p = NULL;
> again:
> char *p2 = realloc(p, len);
> if (p2 == NULL)
> abort ();
> p = p2;
> if (strcpy_s(p, len, name1) != 0
> || strcat_s(p, len, name2) != 0
> || strcat_s(p, len name3) != 0)
> {
> len += 2;
> goto again;
> }
> Nobody can say that this is more appealing to the programmer and
> it is unlikely that code like this will find its way
> into many programs.
> Once again, this can be written more simply as
> char *p = malloc(3*NAME_LEN);
> set_constraint_handler_s(abort_handler_s);
> strcpy_s(p, NAME_LEN, name1);
> strcat_s(p, strlen(p)+NAME_LEN, name2);
> strcat_s(p, strlen(p)+NAME_LEN, name3);
> but this still has several vulnerabilities and coding weaknesses,
> resulting in different end results for p than previously correctly
> functioning code:
> 1. Is it certain that name1, name2 and name3 really were a
> maximum of NAMELEN on entry to this code? What if name1 was
> NAME_LEN+10, while name2 was NAME_LEN-10?
> 2. The code needs to be aware of the exception handler; if the
> handler is not the abort handler, the code *should* check the
> return code and take appropriate steps if it is to function
> correctly. A simple drop in replacement as above will fail with
> the ignore-handler.
Page 2 of 4
> The new string functions are meant as an aid to secure existing code
> bases but the requirement to change ABI (new function parameters,
> additional elements in structures, etc) plus the added complexity of the
> code makes the adoption of these interfaces higher unlikely.
> Instead, a better approach is to eliminate the requirement on the
> programmer to deal with the allocation him/herself. The runtime should
> do this. In this users of systems with the GNU C library can simply use
> char *p;
> if (asprintf(&p, "%s%s%s", name1, name2, name2) == -1)
> abort();
> The runtime makes sure the target string is large enough and that
> error conditions (out-of-memory etc) are recognized. It does leave the
> programmer the responsibility of adding
> free(p);
> when he/she is finished with the result, but is this really harder than
> some of the contortions necessary to use the proposed interfaces?
> The concept of rsize_t and RSIZE_MAX also cause confusion. The lesson
> learned from "640 KiB is enough address space" is that there is no fixed
> limit which people wouldn't want to see lifted over time.
> On 32-bit systems we used to have 2 GiB or up to 3 GiB of address space
> available for user-level code. Nowadays the whole 4 GiB is available
> people asked for it. Any RSIZE_MAX chosen for 2 or 3 GiB address spaces
> would prevent using 4 GiB address spaces. The same is true for any other
> limit and it will definitely remain true for 64-bit architectures as
> Any interface introduced solely for the purpose of using rsize_t instead
> of size_t is completely unnecessary. Aside from the problem of picking
> a size, using rsize_t for different sizes like tmpnam_s (for a string)
> wcscpy_s (a wide char string) and qsort_s (a number of element and a
> type size) makes no sense. How can a function reject handling strings
> of, say, 2^20 bytes but allow wcscpy_s to handling 2^22 bytes (on
> platforms with 4 byte wchar_t)?
> All of this functionality can be implemented with the existing
> implementation. It is always possible for the runtime to determine the
> maximum possible string length, for example, by looking at the gaps in
> the address space at startup time. This *dynamically* determined value
> can then be used for sanity checks; no correct program can ever use
> larger values. It is therefore no violation of the ISO C to handle
> these situations as error cases.
> Then there are the stdio and string functions which are now supposed to
> gracefully handle NULL pointers. However, as "drop-in replacements" for
> the original functions, these are likely to lead to security
weaknesses in
> the application where the unmodified program would crash. Programmers far
> too often don't check for errors and so NULL pointers and the like are
> used in places where they shouldn't. Now assume that the stream pointer
> is supposed to be for a stream where security logs are written to.
> If an attacker can overwrite the FILE* value with NULL no more output
> happens and security problems remain unreported and undetected. The only
> valid exception handler in these cases should be the abort_handler.
> There are a myriad of situations like this where unreported invalid
> input can cause problems. There is a problem with the special handling
> of printf("%s", NULL) in the GNU C library, which chose to print the
> string "(null)". In hindsight, this extension probably was not a good
> idea because it hides problems.
> The only reasonable way to handle invalid inputs is by brute force:
> abort the program or at the very least make absolutely sure the user
> notices the problem. A Denial of Service attack is much less severe and
> easier to notice and battle than an attack which causes, for instance,
Page 3 of 4
> logging to be disabled.
> The "drop-in" replacement technique introduces subtle semantic changes
> in the way an application will behave under a certain set of input data,
> and these semantic changes may well have unintended side effects that
> make the resulting program less "secure". As a result, any programmer
> altering an application to make changes to the interfaces to use these
> bounds-checking versions *must* do more than simply "drop-in" the new
> version. Since he/she is going to that much work anyway, moving to a
> paradigm using malloc'ed memory would be no harder, and is demonstrably
> safer.
> Another problem can arise in cases where the programmer makes incorrect
> assumptions about buffer sizes. In this case, the programmer believes
> that he/she has mitigated all buffer overflow problems by using the new
> interfaces, but in reality there are still buffer overflows possible in
> the code.
> We are not aware of any study demonstrating that the proposed approach
> leads to safer or more-secure software. On the contrary, when one of
> us very-briefly attempted to investigate the matter in 2002, he found
> that the technique (as embodied by the similar strlcpy/strlcat
> functions of OpenBSD) made C code harder to read and to maintain,
> while not catching any bugs in the code surveyed. The code surveyed
> was OpenSSH_3.0.2p1, the then-current version. For some details
> please see:
> In summary, there is no aspect of the proposal which is worth
> standardizing. Either no change is needed for the new interfaces or
> there is no chance that the interfaces will find widespread adoption.
Page 4 of 4

Boost list run by bdawes at, gregod at, cpdaniel at, john at