A report was published recently by the Huawei Cyber Security Evaluation Centre (HCSEC) Oversight Board, a group headed by the UK NCSC/GCHQ and designed to oversee the HCSEC. The report published in March is its 5th annual report acting as a sort of audit of the HCSEC (a Huawei-owned facility in the UK) to help "ensure [its] independence, competence, and overall effectiveness [...] as part of the overall mitigation strategy in place to manage the risks presented by Huawei's presence in the UK[.]"

The purpose of this blog, however, is not to delve into international politics, but to dig deeper into some technical findings of the report. Specifically, we'll be discussing the findings on pages 28 and 29 regarding code quality. Though we don't have access to the same information as the Oversight Board and therefore can't speak with certainty, there do appear to be legitimate complaints with the source code that was reviewed, in particular the finding in 3.38 of bounds-checking APIs being defined away via pre-processor macros to silently not perform bounds checking. What we'll be analyzing instead is section 3.36 of the report, reproduced below:

• There were over 5000 direct invocations of 17 different safe memcpy()-like functions and over 600 direct invocations of 12 different unsafe memcpy()-like functions. Approximately 11% of the direct invocations of memcpy()-like functions are to unsafe variants.
• There were over 1400 direct invocations of 22 different safe strcpy()-like functions and over 400 direct invocations of 9 different unsafe strcpy()-like functions. Approximately 22% of the direct invocations of strcpy()-like functions are to unsafe variants.
• There were over 2000 direct invocations of 17 different safe sprintf()-like functions and almost 200 direct invocations of 12 different unsafe sprintf()-like functions. Approximately 9% of the direct invocations of sprintf()-like functions are to unsafe variants.

As we pointed out elsewhere, the kind of analysis performed above is superficial to the point of irrelevance. While it may be noteworthy in the context of a vendor who claims to only be using "safe" APIs (if they are in fact not meeting those claims), in practice, this kind of name-based API shaming can be harmful. In section 3.35 of the report, it discusses that the Oversight Board analyzed a binary that they described as providing a public-facing service. As such, it should be "expected to be coded in a robust and defensive manner." There are a number of problematic assumptions in the report, but this is one example. The implication here is that anything seemingly-insecure found in a binary used for a public service is a risk, particularly the presence of any form of a non-bounds-checking API. However, a true evaluation of risk would take into account the actual attack surface that exists. Even in binaries providing public-facing services, it's not necessarily the case that all of their functions are part of some exploitable attack surface. Code like that used for parsing administrator-provided config files from disk, etc, cannot reasonably be assumed to be part of any legitimate attack surface. Likewise, what ends up being observable in a binary is highly dependent on a number of factors, one of them being compiler optimizations. Let's dig in to a real example:

In the commented example above, we have a mix of mostly safe uses of strcpy (the length of the source is fully in bounds of the destination) and two unsafe uses. Now let's compile the above code with GCC 6.3 on Debian 8 with a basic -O2 optimization:

To a non-reverse engineer or someone unfamiliar with compilers, the above might be surprising. If we tried to provide a count of the number of calls to strcpy via binary analysis, here we'd have only found two (with one being tail-call optimized into a jump to the API). What happened here is that given the source strings for three of the calls to strcpy were constant, the compiler was able to inline a "builtin" copy of strcpy that directly sets the source contents and nul terminator into the target buffer, without calling out to the actual strcpy implementation in libc. Note that it does this regardless of whether the copy is safe or not (see 'fortify_uncheckable()') and that some of this happens even with optimizations disabled via -O0. When we compile again with -O2 and -fno-builtin (to disable this particular action), we get the following more normal looking result:

As we can see above, the new binary has all the calls to strcpy we would expect -- or does it? Note that there are actually more calls to strcpy here (7) than are actually present in the source (5). A careful reader probably noticed this already in the earlier disassembly: where are our calls to "fortify_uncheckable()" and "fortify_uncheckable2()"? Those calls were both inlined from an optimization pass, but since neither function was marked as static (and we didn't compile with Link-Time Optimization (LTO)), they couldn't be eliminated from the binary as standalone functions. It's this inlining in fact that makes it possible for FORTIFY_SOURCE (which we did not demonstrate here) to detect the overflow in "fortify_uncheckable()" and produce a compiler warning.

So the second binary has seven calls to the "unsafe" strcpy function, while the first has only two. This highlights a flaw in the analysis of the report: is the second binary more unsafe than the first? Does the mere presence of code in a binary indicate that it is used and thus "pose[s] real risk" as the report claims?

Let's move on to the crux of the argument of this blog post: that there is no such thing as "safe" APIs or "unsafe" APIs. An API could be designed to be more resilient against programmer mistakes, or provide certain useful security properties, but "safe" APIs are still be able to be used in unsafe ways, just as it's possible for "unsafe" APIs to be used safely. What does "safe" even signify exactly? Many would list strncpy, strlcpy, strscpy, etc on this list, but an experienced developer knows the situation is much more nuanced than such a coarse grouping. If some well-intentioned person replaced all strcpy uses with equivalently-unsafe uses of strncpy (say by using the length of the source for the third argument), would these uses suddenly become safe? According to the level of analysis present in this report, the answer is "yes." This is not to nitpick, but to point out that the details matter. This level of analysis may have passed as acceptable two decades ago, but much more sophisticated and accurate methods of analysis are present today.

Viewing security in terms of "safe" and "unsafe" APIs doesn't just result in hypothetical harm: the mindset has led some to engage in large rewrites from one API to another. What is often missed in such large rewrites (and this is applicable beyond just simple string-copying routines) is the nuance of what is really needed in individual cases.

Let's deal with a concrete example: "strlcpy". This API originating from OpenBSD would be on most people's list of a "safe" strcpy API. It doesn't write more than the destination length provided and nul-terminates the result. There's another API that some might call "safe" as well: strncpy. It has the same behavior as strlcpy when it comes to not copying more than the destination length, but if the source string is longer than the destination buffer, it doesn't nul-terminate. So it's more "unsafe" than strlcpy in that regard. But it has an important additional feature: it zero-pads to the provided length of the destination buffer. In some instances, this isn't needed, but in attack surface working with structured data containing fixed-length strings (the Linux kernel is one large example), it's actually very important. Failure to zero-pad buffers that are copied to userland is a recipe for an information leak. There's another problem with strlcpy in that it effectively performs an strlen() on the source string, regardless of whether it's attacker provided and 500MB large. So it would be unheard of for someone to just make a blanket replacement of uses of strncpy in some codebase and switch them all to strlcpy, right?

We've provided this example elsewhere before, but it bears repeating here. The exact scenario played out in multiple stages starting in 2013 and ending last year. Back in 2013 a series of infoleaks were fixed in the crypto subsystem related to lack of zero-padding on strings copied into fixed-length buffers copied to userland. These were fixed by Mathias Krause in this commit. Four years later, some of those strncpy uses got replaced with strlcpy, by the Linux kernel crypto maintainer, to deal with the lack of nul-termination on truncation issue. This reintroduced some of the information leaks that had been fixed in 2013. Less than a year after that, this replacement got extended to more of some newer strncpy uses to silence a compiler warning, introducing even more information leaks. That second attempt was spotted by Eric Biggers, who then asked that other similar patches be reviewed for the same issue, but the initial incorrect changes made in 2017 persisted. It wasn't until several months later, in November of 2018, when a similar strncpy to strlcpy conversion was attempted again, that this commit was merged, reverting the original erroneous conversions from a year and a half prior. Our grsecurity patches were not affected by these vulnerabilities.

If there's one takeaway from this blog, it should be: while binary analysis is very interesting, actionable information should be derived through modern analysis techniques, not through coarse-grained name-based groupings of APIs into "safe" and "unsafe" variants. Reject that mindset and use the right tool for each job.