Disposition of comments WDTR 24731

Disposition of comments for WDTR 24731

Document: WG14 N1114
Date: 2005/04/12

Canada

CA01: Canada does not agree with the use of the word "secure" in the title of this document. This word carries considerable baggage for the safety and security communities and implies functionality that is not provided. We would suggest alternative terminology, such as "bounded".
Committee Response:

The Committee agrees, and suggests "secure" be changed to "safer".
CA02: Some of the functions provided exist or conflict with functions provided in other domains, such as POSIX or SUS. If the behaviour is different than these specifications, then programs that attempt to use them may be incorrect. Even if the behaviour is the same, then the pre-existing specification must be respected. Examples of such functions are:
strtok_r, strcpy_s and strcat_s.
In particular, strcpy_s provides identical functionality to strlcpy which has been in use for some time.
Committee Response:

The Committee wants to deliver a complete package, not _s for some _r for others. The committee will develop an Annex or Rationale to describe all differences, and keep parallel functionality whenever possible. The library constraint option can and most likely will make these functions behavior inconsistent with functions defined in other Standards. Also see GB08.
CA03: Special mention is made of strtok_r. This function should clearly state that any string to be read by such a function may already be effectively unbounded, and while it may be bounded by the buffer in strtok_s, the effects of inputting the original unbounded string may already have occured.
Committee Response:

See GB08.
CA04: Special mention is required of strerror_s. A mechanism is required to predetermine the size of the string to be returned so that the buffer can be preallocated.
Committee Response:

The Committee agrees.
CA05: Special mention is also made or "rand". Saying that cryptography methods must be used is insufficient. The function specification ignores several issues, such as blocking, enthropy and seeding of such functions.
Committee Response:

The WDTR 24731 did not contain this function.

Germany

DE01: sprintf_s and other read functions with the exception of strnlen need to be addressed somehow.
Committee Response:

TR 24731 has the goal of not producing unterminated strings or storing past the end of a buffer. Except for a few specialized cases, functions that take strings as input parameters trust that the string is null terminated. More Rationale for this will be added. Also see US04 and US30.
DE02: It is worth to consider, whether strncpy_s and strncat_s variants with one parameter for length should be added. Zero-termination is in that case assumed.
Committee Response:

The reivew copy of WDTR 24731 contains a strcpy_s and strcat_s, with a length parameter.
DE03: It is worth to consider offering a feature, simply aborting the program upon buffer length overrun. Such an option could be activated conditionally via #pragma or #define.
Committee Response:

This will be addressed by the library constraint feature. See GB02.

Japan

General: As already described in our comment on NWIP ballot, we think it is extremely hard to make the C programming language secure by just adding a set of new functions leaving the existing insecure ones untouched. To make the C programming language truly secure it is necessary to abolish the existing insecure features like pointer operations and some dangerous functions. We strongly request WG14 to produce the rationale or annex which clearly states the following points:
Committee Response:

See CA01.
JP01: Clearly state the policy and criteria for introduction of new functions For example, we think that gets_s() is redundant and should be eliminated from the TR because the functionality of gets_s() can be covered by existing fgets() function. So, please make clear the reason of adding gets_s() besides fgets().
Committee Response:

The Committee will generate a clear policy for the inclusion of the function gets_s in the Rationale.
JP02: Clearly and concretely state what kinds of security hole exist in the current ISO C standard(syntax and libraries).
Committee Response:

The Rationale will be extended to cover these issues, and the introduction section will be expended.
JP03: Clearly explain what kind of vulnerability is fixed for each proposed function one-by-one.
For example, we cannot understand what is improved in wctomb_s() from the original wctomb(). Please provide the explanation of the advantage of wctomb_s() over wctomb().
Committee Response:

The Rationale will be extended to cover these issues, there will also be some code examples added to the TR to show the advantage these new functions provide.
JP04: Give the guide for secure programming by using the proposed functions.
Committee Response:

See JP03.

Netherlands

NL01: Section 3.1, the definition of the term 'diagnosed undefined behavior' is unclear; change to behavior, that is invoked by the use of a nonportable or erroneous program construct or of erroneous data,and that an implementation shall diagnose by, in effect, calling an implementation-defined function.
Committee Response:

The Committee agrees that this section was not clear. This entire behavior is being redone.
NL02: Section 5.3, para 2; replace 2nd sentence by If a function that has parameters of type rsize_t is called with values for those parameters that are greater than RSIZE_MAX the behaviour shall be diagnosed undefined behaviour.
Committee Response:

The Committee agrees, this sentence will be rewritten
NL03: Section 5.3, para 4, last sentence: replace 'diagnosable' by 'diagnosed'.
Committee Response:

The Committee agrees, this behavior is being redone.
NL04: Section 5.5.2.1, para 5, first sentence: replace 'diagnosable' by 'diagnosed'.
Committee Response:

The Committee agrees, this behavior is being redone.

United Kindom

GB01: Change title to "Specification for additional C library functions with additional parameter checking and/or re-entrancy". Make consequential changes; in particular, change __STDC_[WANT_]SECURE_LIB__ to __STDC_[WANT_]CHECKING_LIB__and change _s suffixes to _c or _r as appropriate.
Rationale: these functions don't provide additional security, since they can still be misused (e.g. set all bounds parameters to RSIZE_MAX). They do, however, provide parameter checking and re-entrancy.
Committee Response:

See CA01. The Macro name will be changed to match the package.
GB02: Change the term "diagnosed undefined behaviour" to "diagnosed erroneous behaviour" or some other term not using the conformance terms "undefined" or "unspecified".
Rationale: the term "undefined" has a well-known meaning which includes no requirement to diagnose it. A different term would be less confusing.
Committee Response:

The Committee agrees. Also see NL01.
GB03: Prefix to 5.1.1 para 3: "For those names which are reserved by ISO/IEC 9899:2004,"
Rationale: footnote 7 is not normative, and this change makes it clear that this requirement does not allow an implementation to intrude into the user's name space.
Committee Response:

The Committee does not agree with moving this footnote. The Rationale will be expended to cover the name space pollution issue.
GB04: Delete clause 5.2. More generally, replace the use of the errno_t and rsize_t typedefs with some other notational mechanism.
Rationale: typedefs should not be used for pedagogical purposes, but only where the type that meets the requirements varies between implementations. Slightly reducing the amount of text in the TR does not justify polluting the namespace.
Committee Response:

The Committee does not agree. Precedence exists in ISO standards (C++ and POSIX) to give meaningful names to types. This is common good programming practice.
GB05: In 5.5.2.1 para 2, delete the second compar == NULL.
Rationale: clearly an error.
Committee Response:

The Committee agrees.
GB06: Delete clause 5.5.3.
Rationale: the Standard already has better functions in the form of mbrtowc and wcrtomb. If these latter have a problem, fix it rather than creating alternatives.
Committee Response:

The Committee does not agree. These functions have problems; the new functions will provide a safer approach. Conversion to the new functions will be more mechanical.
GB07: The strcpy_s, strncpy_s, strcat_s, strncat_s, wcscpy_s, wcsncpy_s, wcscat_s, and wcsncat_s functions should all explicitly guarantee that s1 is left null-terminated after the call, provided of course that (s1 != NULL && s1max > 0 && s1max < RSIZE_MAX).
Rationale: lack of null termination is a major cause of problems. Better to require it than to rely on other bits of code spotting it.
Committee Response:

WDTR 24731 provides this functionality.
GB08: The strtok_s function needs an s1max parameter with appropriate tests on s1 and s2. I am agnostic as to whether an s2max is needed.
Rationale: any string which gets altered should be bounds-checked.
Committee Response:

The Committee agrees. s2 does not need to have a length associated.
GB09: Delete 5.6.4.1 para 5, second sentence.
Rationale: "..." is a cultural-specific convention, and it is not even clear that three dot characters is the right approach. In particular, some people would say that "[...]" is better while others might point to the specific ellipsis character in various character sets.
Any such patching should be left to the application, based on the return code.
Committee Response:

The Committee does not agree. The "..." is directed to the programmer where there is history for using this character.
GB10: In 5.7.1, change "rages" to "ranges".
Rationale: obvious.
Committee Response:

The Committee agrees.
GB11: In 5.7.2.1 para 2, change "0" to "-999".
Rationale: the behaviour of asctime is defined for these years, so there is no justification for the restriction.
Committee Response:

The Committee disagrees; this has been discussed and there was no consensus to make this change. strftime is the function that is recommended.
GB12: Checking versions of the strftime and wcsftime functions should be provided.
Rationale: although these functions already provide a "maxsize" parameter, there are many other checks which can and should be made - for example, the RSIZE_MAX test or that the year number is sensible.
Committee Response:

The Committee disagrees; these functions do not currently have the level of vulnerable issues this document is addressing.
GB13: If item 4 is not accepted, change size_t to rsize_t in 5.8.1.1 para 3.
Rationale: obvious.
Committee Response:

The Committee agrees.
GB14: 5.1.1 should use the same terminology concerning reserved names as 7.1 of the main Standard does. For example, it is not clear whether "are defined" means macro versions of function names must be defined.
Rationale: clarity of the text.
Committee Response:

The Committee agrees; words along the lines of 7.1.3 #1 will be incorporated.
GB15: In 5.4.4.1, the newline should not count towards the maximum number of characters read.
Rationale: the size parameter is supposed to indicate how much space is available. Compare %s and friends in scanf, which don't count the skipped leading spaces.
Committee Response:

The Committee agrees.
GB16: In 5.6.4.2 and 5.8.2.3.1, RSIZE_MAX + 1 would seem a better return value than 0 for the null pointer and not-null-terminated cases.
Rationale: making the value be RSIZE_MAX + 1 will trigger appropriate alerts in the other functions in this library, while zero will just silently truncate strings. If RSIZE_MAX == SIZE_MAX, this will still fall back to zero.
Committee Response:

The Committee disagrees, the return maxsize is more algorithmically useful. Rationale will be provided for this.
GB17: In 5.7.2.1, change the description to simply require the output to be the same as that of asctime.
Rationale: avoids the risk of inconsistency creeping in.
Committee Response:

The output strings will match in most cases, the behavior of asctime_s is better defined. Rationale will be provided for this.
GB18: In many places the character "s" is spuriously italicised
Committee Response:

The Committee believes this is a rendering problem with the program you use to read the document.

United States

US01

WG14/N1089 is a good start on a rationale. There should be one, either included in the TR or in a separate document.

Committee Response:

The Committee agrees.

US02

The rationale should discuss why there are not _s versions of the following functions, and the general philosophy involving minimizing performance impact.

strchr, strcspn, strpbrk, strrchr, strspn and strstr

Committee Response:

The Committee agrees.

US03

The following functions are missing the specification that they open a file for exclusive access.

tmpfile_s, fopen_sand freopen_s

Committee Response:

The Committee agrees. A footnote will be added to clarify.

US04

From the editor's report, it appears that sprintf_s and snprintf_s will do essentially if not exactly the same thing. Maybe only one of them is needed.

Committee Response:

The Committee notes the concern.

US05

It would be best for the committee to spend some more time discussing the notion of diagnosed undefined behavior. This is a new invention for the C standard.

Committee Response:

See GB02.

US06

Page v: End of paragraph 6: Extra(?) "*".

Committee Response:

This is a delete indicator, and will not be in the final version.

US07

Page 1: Should ISO/IEC 9899:1999/Cor 2:2004 be added to list of references?

Committee Response:

The Committee agrees.

US08

Page 7+: Consider changing "bug" to "programming error".

Committee Response:

The Committee agrees.

US09

Page 7: Footnote 9: Consider changing "constant expression" to "integer constant expression".

Committee Response:

The Committee disagrees, RSIZE_MAX is always a size_t and is not appropriate for a static initializer.

US10

Page 13: Paragraph 7 / Example 2: The end of that example says: "No assignment to s occurs.". Where is that requirement in normative text? Also, this seems like a large burden to place on the implementation (requires a temporary buffer to hold the input string until that string's length is known). Better would be s[0] is set to a null character, and the other elements of s are unspecified.

Committee Response:

The Committee agrees. The "No assignment to s occurs." will be removed.

US11

Page 14/15: Footnote 13: Is on wrong page and overlays the section number. Can the same footnote be referenced by three different sections (which are on different pages)?

Committee Response:

The Committee agrees.

US12

Page 16: 5.4.4.1p4: Consider adding: "and the other elements of s are unspecified".

Committee Response:

The Committee agrees.

US13

Page 17: 5.5.1.1p2: Consider adding:

maxsize ==
        0

to the list of diagnosed undefined behavior.

Committee Response:

The Committee agrees.

US14

Page 20: 5.5.2.2: Why does qsort_s return void instead of errno_t?
How does it indicate failure?

Committee Response:

The Committee agrees.

US15

Page 24: 5.6.1.2: End of paragraph 3: Extra(?) "*".

Committee Response:

See US06.

US16

Page 28: 5.6.2.2p4: The 'm' in 'm+n' should be italic.

Committee Response:

The Committee agrees.

US17

Page 31: 5.6.4.1p5: Why are there three '.' characters used to overwrite the end of the string (Answer could go in a footnote)?
Would it be useful to show some examples both with and without the overwrite?

Committee Response:

It adds truncation, here will be words in the Rationale to clarify.

US18

Page 33: 5.7.2.1p2: Is the broken down time before or after the 1900 has been added to it? Why 0 rather than -999 (which fits in a 4 digit field)?

Committee Response:

The Committee believes it is clear that it represents the calendar year. Also see GB11.

US19

Page 37: 5.8.1.1: What happens if the string is longer than the space to store it? Is the first character set to null character?

Committee Response:

The Committee agrees; the wide versions will match the functionality of the narrow versions.

US20

Page 38: Footnote 28 extends too far down the page.

Committee Response:

The Committee agrees.

US21

Pages 39-40: Footnote 29 is referenced from two pages.

Committee Response:

This is intentional. The Editor will look at an alterative approach for this problem.

US22

Page 46: 5.8.2.2.2p4: The 'm' in '-m+n' should be italic.

Committee Response:

Noted, also see US16.

US23

The TR leaves it up to the implementation to determine the value of RSIZE_MAX. The most useful value will often depend on both the implementation and the application, so many/most implementations will provide a way for an application to specify the value (at run-time). On the other hand, some implementations may choose to make the value an unalterable translation-time constant. We would like to see a specified means to set the value of RSIZE_MAX, with a status return indicating whether or not the value was successfully set. One thought might be to do this through a function-like macro in stdint.h, e.g.

errno_t
        SET_RSIZE_MAX(size_t)

. An implementation that did not support an application's setting the value at run-time would not define the macro. Otherwise, an invocation of the macro would return zero if the value was successfully set, or a non-zero value to indicate failure (e.g. an inappropriate value was specified or the application was built in a way to disallow run-time modification of the value).

Committee Response:

The Committee believes that it is premature to standardize this feature at this time. Implementations may experiment with this feature if they wish.

US24

The definition of diagnosed undefined behavior as calling an implementation defined function might benefit from examples of the name ans signature of such a function - not as a requirement, simply to encourage more than one implementation to make the same choices.

Committee Response:

See NL01.

US25

The memcpy_s function does not list overlap between input and output among its diagnosed undefined behaviors. Is that solely because the language does not define pointer comparison between distinct objects, or is it considered a practical difficulty for real implementations? In fact, the wording of "take on unspecified values", seems to preclude overlap from being diagnosed. That's puzzling, as diagnosing memcpy calls that ought to be memmove calls is certainly a useful capability.

Committee Response:

The Committee agrees.

US26

In some cases there seems to be change for the sake of it from what is either already standard or defacto standard.
The common change seems to be return values that were char* with error being NULL being changed to this new errno_t type which is really just an int.

strcpy_s, strlcpy, strcat_s, strlcat, asctime_s, asctime_r, gtime_s, gtime_r, localtime_s and localtime_r

Committee Response:

See CA02.

US27

For many of the functions defined in this TR, it appears that very similar or identical alternatives are already available. In those cases, I'm not sure it makes sense to ignore prior art by defining new replacements. Some examples:

tmpfile_s() is identical to the existing tmpfile() except for the style of returning the resulting FILE *.
Committee Response:

See US03.

tmpnam_s() is almost the same as the existing tmpnam_r() except it includes an argument for the size of the destination buffer. More important, however, is the fact that it is still unsafe due to race conditions with others who might create the same files. Safe usage requires the use of tmpfile() or mkstemp() instead.
Committee Response:

There will be words added to the Rationale to clarify this; the Committee believes having this function in the package helps with mechanical transition.

strcpy_s() is equivalent to strlcpy(). It seems like providing yet another safe version of strcpy() would be confusing at best.
Committee Response:

See CA02.

fscanf_s() and related variants are just like the current routines except they require a size parameter for buffers that hold the result of %c, %s, and %[ formats. That's very useful, but I believe this feature could be added to the existing scanf() family in a compatible way. What's needed is a format modifier to indicate that the size is specified by a parameter, like the * in printf() format strings. Since scanf() already uses * for assignment suppression, a different character would be needed, but one could choose any unused format character and remain compatible with the existing scanf() functions.
Committee Response:

The Committee believes that the optional nature is not desirable.

US28

A comprehensive Rationale for the TR should be provided.

Committee Response:

The committee agrees. Also see US01.

US29

The issues (including missing features) raised in the Secure TR Editor's Report, SC22 WG14 N1089, should be addressed. N1089 is available at http://www.open-std.org/JTC1/SC22/WG14/www/docs/n1089.pdf

Committee Response:

This will be addressed.

US30

The committee should consider adding the following functions described in N1089:

fprintf_s, printf_s, snprintf_s, sprintf, vfprintf_s, vprintf_s, vsnprintf_s, vsprintf_s, fwprintf_s, swprintf_s, vfwprintf_s, vwprintf_s, wprintf_s, vswprintf_s, mbstowcs_s, wcstombs_s, mbsrtowcs_s, wcsrtombs_s and wcrtomb_s

Committee Response:

Yes, these will be considered.

US31

The committee should consider adding support for optional truncation during string copy. See _TRUNCATE in N1089.

Committee Response:

Yes, these will be considered.

US32

A footnote should be added explaining that the tmpfile_s, fopen_s, and freopen_s functions should open their files in a "safe" mode giving exclusive (non-shared) access.

Committee Response:

Yes, this will be done. Also see US03.

US33

The committee should carefully consider the issues around "diagnosed undefined behavior," including:

the name of the term
the model of behavior
where the description of diagnosed undefined behavior should appear in a subclause specifying a function (in the "Description" section, in the "Returns" section, or in a new section labeled "Diagnosed Undefined Behavior").

Committee Response:

Yes, these will be considered. Also see NL01.