Re: Usage of C11 Annex K Bounds-checking interfaces on GCC

[David Brown <david.brown@hesbynett.no>]

On 15/12/2019 02:57, Jeffrey Walton wrote:
> On Sat, Dec 14, 2019 at 12:36 PM Martin Sebor <msebor@gmail.com> wrote:
>>
>> On 12/9/19 8:15 PM, li zi wrote:
>>> Hi All,
>>> We are using gcc in our projects and we found some of the C standard functions (like memcpy, strcpy) used in gcc may induce security vulnerablities like buffer overflow. Currently we have not found any instances which causes such issues.

(This post is all "In My Humble Opinion".)

The correct use of memcpy, strcpy, etc., does not introduce any security 
vulnerabilities.  /Incorrect/ use does - just as incorrect use of any 
function can risk bugs, and therefore security vulnerabilities.

>>> But we feel better to change these calls to Cll Annex K Bounds-checking interfaces like memcpy_s, strcpy_s etc. By defining a secure calls method (list of func pointers) and allowing application to register the method. I understand that this affects performance because of return value check added for xxxx_s calls, but this will relieve overflow kind of issues from code. And also currently using bounds-checking interfaces is a general industry practice.
>>> Please share your opinion on it, and if any discussion happened in community to do some changes in future.

Thoughtless "change all standard <string.h> functions to Annex K 
functions" is management arse-covering as an alternative to proper 
quality development techniques.  It adds nothing to the software 
quality, it has no reduction in the risk of errors or their 
consequences.  It is nothing more than a way to try to blame other 
people when something has gone wrong.

If you find that using these functions really does give you better 
software with lower risks of problems, then by all means use them.  But 
don't use them blindly just because someone says they are "safer".

>>
>> GCC's Object Size Checking is a non-intrusive solution to
>> the problem.  It avoids the considerable risk of introducing
>> bugs while replacing existing calls with those to the _s
>> functions.  The implementation is restricted to constant
>> sizes so its effectiveness is a limited, but we have been
>> discussing enhancing it to non-constant sizes as well, as
>> Clang already does.  With that, it should provide protection
>> with an effectiveness comparable to the _s functions but
>> without any of the downsides.  (Note that GCC's buffer
>> overflow warnings are not subject to the same limitation.)
>>
>> Besides Object Size Checking, I would suggest making use of
>> the new attribute access.  It lets GCC detect (though not
>> prevent) out-of-bounds accesses by calls to user-defined
>> functions decorated with the attribute.
> 
> The safer functions have three or four security goals. The workarounds
> don't meet the goals.
> 

Let's call them "Annex K" functions, rather than "safer functions", 
until it is demonstrated that they actually /are/ safer in some way. 
And let's talk about other tools in the toolbox, to use in addition or 
as alternatives, rather than calling them "workarounds".

> The safer functions require the destination size to ensure a buffer
> overflow does not occur.
> 

That is useless unless you know the destination size.  And if you know 
the destination size, you can use that to make sure your calls to 
memcpy, strcpy, etc., are safe and correct.

The use of the Annex K functions therefore gives you no technical 
benefits in this aspect.  They may, however, give you the non-technical 
benefit of forcing the programmer to think about destination sizes - 
they can't be lazy about it.  However, if you are dealing with code that 
needs to be of good quality then you should already have development 
practices that cover this - such as code reviews, programmer training, 
code standards, etc.

> The safe functions always terminate the destination buffer.

So do the standard functions when used correctly.

I'm quite happy to agree that strncpy has a silly specification, and it 
can surprise people both in its inefficiency and in the fact that it 
does not always null-terminate the destination.  Creating a "strncpy_s" 
with redundant parameters and confusingly different semantics is /not/ 
the answer.  The right solution would be to deprecate strncpy, and make 
a replacement with a different name and better semantics, such as:

char * strscpy(char * restrict s1, const char * restrict s2, size_t n)
{
     if (n > 0) {
         *s1 = '\0';
         strncat(s1, s2, n - 1);
     }
     return s1;
}

/That/ would have been a useful, clear, and consistent function that 
copies at most "n" characters from the string, and always terminates the 
copy.

> 
> The safe functions provide a consistent return value. There is only
> one success code.

There is only one success code from the Annex K functions - but no 
guarantees about failures.  It will call the constraint handler - which 
might return, or might not.  Frankly, you have no idea what it might do 
in the general case, since the constraint handler is a global state 
variable and not even thread-safe.

The only way to write good, safe and reliable code using the Annex K 
"_s" functions is to make sure they are called with sizes that can never 
cause run-time constraint errors.  And when you can do that, you can do 
it just as well with the standard library functions.

> 
> It is easy to teach a novice user how to use the safe functions
> correctly because there is only one rule for return values.

But there are multiple and unclear parameters to the functions.  How is 
the constraint handler "easy to teach" ?

You'd to a lot better teaching people to think about the size of buffers 
that they use - both for the source and for the destination.  (The Annex 
K functions have mostly forgotten about checks on the size of the 
source.)  Teach them to think about these things correctly and it will 
work for /all/ code - teach them to use the "_s" functions and they'll 
just get a false sense of security without understanding the real issue.

> 
> The safer functions should provide portability across platforms. Write
> once, run everywhere.

That is a joke, surely.  Even if there were many C implementations that 
supported them (there aren't), even if they were efficient enough to be 
a sensible choice for important code (usually they are not), they are so 
full of fundamentally implementation-dependent behaviour that they are 
not portable.

> 
> Consider, if object size checking were a complete replacement, then
> glibc should not make so many appearances on BugTraq for out-of-bound
> reads and writes. Confer,
> https://cve.mitre.org/cgi-bin/cvekey.cgi?keyword=glibc.
> 

Out of bounds accesses are a problem and a source of error in C coding - 
no one disputes that.  There is scope for improvement of the C standard 
str and mem functions, in terms of making them easier to use safely, 
making them harder to use incorrectly, and making them more efficient - 
I doubt if anyone will dispute that either.  But the Annex K functions 
are most certainly not the answer.

> The glibc folks have done more harm then good with their politics.
> 

That is an opinion that is not universal, I think.