Hacker News Reader

Comment by Noxmiles | Hacker News Reader

"On dozens of occasions, Huawei engineers disguised known unsafe functions (such as memcpy) as the “safe” version (memcpy_s) by creating wrapper functions with the “safe” name but none of the safety checks. This leads to thousands of vulnerable conditions in their code."

Things like this everywhere. Just stupid programmers or method?

View on HN

This is the "expected" outcome of most big tech organizations. I doubt there was malice or stupidity from the actual people coding this (but the malice still exist, it is just a few layers up)

You have two kind of elite at the top: a bunch of very smart people, completely disconnected from daily work, setting up automated code checks and style rules. Let's call those architects/Engineering VPs. Then you also have a bunch of short term revenue people in a completely different side of the company who owns all the money and set the targets. Let's call those Product VPs

The daily work will be full of agile-cargo-cult (that is, all the meetings agile impose, without any the actual communication), with people in the teams reporting to different chains. Engineers having to abide by the crazy ineffective coding style (usually because nobody cared to provide the tools to help achieve the code quality required), and product/managers abiding by the short term revenue churn.

And since the Product org is the one who decides if target goals were met for bonuses and promotions you see why the actual coders might do that bullshit (e.g. which one ever got you a promotion: "I contributed to hugely announced feature X" vs "I followed all the best practices set up by the architects i have never met and prevented a security report two years in the future from dragging the company in the mud"?)

Most companies denounce the problems of having silos, but silos actually make the smaller org/teams responsible for the entire stack and tools. It is hated by investors because it is more expensive, but in the end, it is the real cost. A very vertical big corp will be cheaper but the reason it is cheaper is because less work is being done and always end up like this for ignoring the human nature of the people that it is made of.

...and that right there is Huawei, Boeing, Exxon, BP, and a thousand other big corporate dumpster fires in a nutshell.

Dumpster fires are everywhere not just large companies: contractors, restaurants, marriages etc.

I would argue that the natural state of things is variable levels of dumpster fire.

He didn't say anything about big companies being exclusive to disasters.

agree.i didnt mean to seem like i thought he/she/they said only big companies.

but isnt it sufficiently common public language to focus on large /corporate/capitalism etc? i only hoped to offer a note to support thia this conversation from falling into that.

thanks for following up on that, good catching!

True, but greenshifting, cya driven development and derailment by metrics come natural with a multilayered middlemanager organization.

Engineers having to abide by the crazy ineffective coding style

This is usually a sign of "metrics-driven management". They are then surprised why all their measures of "code quality" and "compliance" on their awesome dashboards and analytics are showing 100% while the actual product continues to spew forth bugs and disappoint their customers.

Any rules perceived as stupid will naturally be worked around. "You get what you measure."

I think the analysis glosses over compile time checks.

Take the "VOS_memcpy_s" function they condemn for not checking one of the arguments on page 35.

If the source contains a check that the destination length is no less than the source length, for instance in an assert(), and the compiler is able to conclude that will always be the case, it is perfectly justified in not generating code for the check.

The trouble is that the check code would also be missing if the "production code" was compiled with asserts disabled.

To make the conclusion they do, they have to have checked that:

A) All implementations lack the check and all are in not-link-time-optimized library locations. (in which case asserts was probably disabled)

B) All distributed implementations (because VOS_memcpy_s was either static inline or #defined) all lack the check and at least one of them is called with arguments where the check does not hold or cannot be determined at compile time. (in which case asserts was probably also disabled.)

If they cannot prove either of these two points, it may just be that the code is in fact correct, and that the compiler was able to determine this and did not need to emit the checks.

For all their bragging about how state of the art their tools are, I see no indication that they have analyzed that deep and comprehensive.

That said, Huawei's code is still has atrocious quality.

Or as the C-team calls it: "Industry Best Practice"

For both A and B, why do they need to check all locations? Wouldn't finding a single location be enough to prove that it's happening?

Also in A and B you seem to be saying it's difficult for them to see whether all the locations lack the check. But it sounds to me like their static analysis makes that fairly easy, and that that's what they found.

You are saying that VOS_memcpy_s's source code could be safe but have its safety checks hidden by the compiler if the compiler has strong optimizations enabled while simultaneously having asserts enabled and also every single call to VOS_memcpy_s has an assert before it. That seems like a pretty rare configuration, because AFAIK, asserts are usually disabled when strong optimizations are enabled. And it seems unlikely to me that Huawei would put an assert before every single call to VOS_memcpy_s, for multiple reasons, (1) because Huawei is sloppy, (2) because there's no reason to put an assert before VOS_memcpy_s if it has safety guarantees. So I conclude there's very little likelihood there are any checks in VOS_memcpy_s's source code.

No, you would put the assert inside the VOS_memcpy_s() implementation, and you would not make that a library function, but a static inline function, so the compiler can see the arguments every time it is called.

As for disabling asserts in the code you ship: That is such a quaint 1980'ies thing to do, and people should have stopped doing it long time ago.

First, asserts are usually free, in the sense that the values checked are already in registers.

Second, any assert the compiler can evaluate is free, because no code will be generated.

Third, asserts can often enable the compiler to produce better and faster code, because it knows more about the values it produces code for.

And this is not theory: Nobody has ever accused Varnish Cache of being slow, despite the fact that approx 10% of all source lines are asserts which cannot be compiled out.

My money is on Huaweis function doing the check with an assert which was compiled out for deliverable code.

Why else would the have the function in the first place ?

The fact that code is generated for the function as shown, indicates that it was not a static inline function, because the compiler would have reduced that to just the regular memcpy call, so it was probably in a library.

So yeah: Industry Best Practice Incompetence.

>As for disabling asserts in the code you ship: That is such a quaint 1980'ies thing to do, and people should have stopped doing it long time ago.

Sqlite makes all its asserts noop in release mode[1].

Chrome disables DCHECK() in release mode[2], and it's used many times[3].

RE2 has debug only checking[4].

In Rust debug mode, signed integer overflow panics, in release mode it wraps[5].

Firefox disables MOZ_ASSERT() in release mode[6], and it's used many times[7].

I doubt Huawei is more modern and less 1980s than all these other projects.

>My money is on Huaweis function doing the check with an assert which was compiled out for deliverable code.

Are you saying it was compiled out because asserts were disabled, or because the compiler could reason about the arguments at every callsite? It seems unlikely to me that the compiler could reason about the arguments at every callsite.

[1] https://www.sqlite.org/assert.html

[2] https://chromium.googlesource.com/chromium/src/+/master/styl...

[3] https://cs.chromium.org/search/?q=DCHECK%5C(+case:yes&sq=pac...

[4] https://github.com/google/re2/blob/master/util/logging.h

[5] http://huonw.github.io/blog/2016/04/myths-and-legends-about-...

[6] https://dxr.mozilla.org/mozilla-central/source/mfbt/Assertio...

[7] https://dxr.mozilla.org/mozilla-central/search?q=MOZ_ASSERT(...

Yes, the FOSS industry needs to grow up about software quality.

It is natural to have very expensive checks in development and test-environments, they help and aid development and QA to no end, and of course they should be disabled in production.

But the run-of-the-mill assert which documents integer ranges, pointer non-null-ness, data structure relationships and so on, cost nothing in performance and should be left in production code.

And as I said: We developed Varnish Cache to prove this point, and we overwhelmingly have: One bad CVE since 2006, and "only" about 20% of all web-traffic goes through Varnish at some point or other.

Re Huawei:

The report claims that there where no checks at all, and I'm saying they have not documented evidence for that claim, because the lack of checking code could be either disabled asserts in production code or the compiler reducing it as dead code.

This is highly disingenuous. They had no source code access and are just looking at whatever their static analyzer tool is spitting out, with looking at optimized assembly for these specific functions.

Counting references to libc functions then suggesting these are "unsafe" because they are not calls to the exactly as unsafe Microsoft _s incompatibility functions is the definition of stupidity.

Indeed. memcpy_s is certainly the definition of stupidity (as I heard someone once say, "that's what the _s stands for") because there is nothing "unsafe" about memcpy() as oppposed to e.g. the infamous gets(). It copies exactly the number of bytes you tell it to, no more and no less.

The length calculation always has to be done before the copy. Introducing an extra check in the way memcpy_s does is futile if you didn't get it right in the first place, and if you did, like you should, then it's nothing but bureaucratic bloat that can even make "safety" worse (because now there are two length checks, the one that actually does the work and makes the decisions, and this implicit one that someone in the future updating the code might miss.)

Almost all of the _s shit is just pure paranoia-FUD propagated by basically clueless "security" experts, mainly at Microsoft. To see the depths of this insanity, consider that they even have a "safe coding" standard where memcpy() is "banned", but memmove() is not, and there is no memmove_s() --- despite the fact that with the exception of overlapping areas, memmove() behaves exactly the same as memcpy(). You could simply search-and-replace all occurences of the latter with the former, and suddenly code that is considered "unsafe" is not.

That sounds as much like "hey the qa/code review kicked out back and said you have to use memcpy_s not just memcpy. We're on a deadline, fix this so we can get it back to them by [short deadline]."

Kind of like melamine letting something pass protein content testing.

So extremely, criminally awful?

Except that forcing use of memcpy_s does nothing to actually fix the real problem. Enforce arbitrary rules, get arbitrary results...

If they copied or used code that relied on using the safe memcpy_s, instead of having to change that code to use the unsafe versions, this could just be a proxy layer that lets that code run. This might be done so it's easier to keep the other code up to date if it's getting updated else where.

Right, but why not take thirty seconds to do the appropriate checks in the wrappers?

Because it's far from being thirty seconds (as seen in the post by ploxiln here):

http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1967.htm

The most interesting quote regarding these "safer" interfaces from the report:

"The design of the Bounds checking interfaces, though well-intentioned, suffers from far too many problems to correct. Using the APIs has been seen to lead to worse quality, less secure software than relying on established approaches or modern technologies. ... Therefore, we propose that Annex K be either removed from the next revision of the C standard, or deprecated and then removed."

I don’t understand. Your link is talking about the difficulty of using these functions. But here, the functions are already being used. What’s missing is implementations of them. They chose to implement these functions without the security checks, but implementing them with the security checks is not hard.

That whole article is basically saying that those extra checks are useless anyway in correctly written code, as otherwise it would be code that can be tested and reached:

On the other hand, in code that does check for and attempts to handle errors reported by the APIs, the new error handling paths tend to be poorly tested (if at all) because the runtime-constraint violation can typically be triggered only once, the first time it is found and before it's fixed. After the flaw is removed, the handling code can no longer be tested and, as the code evolves, can become a source of defects in the program.

It's possible that they are calling the functions, but passing incorrect length arguments so frequently that they are best ignored.

> Therefore, we propose that Annex K be either removed from the next revision of the C standard, or deprecated and then removed.

This irked me, because along with useless junk like memcpy_s, Annex K had memset_s, which in particular (and unlike memset(3)) was guaranteed not to be optimized out by the compiler.

Meanwhile Spectre has made it more important than ever for programs to clear any sensitive data out of memory as soon as it's no longer needed, so we actually need that. And it's troublesome to either have half a dozen ifdefs for all the different platform-specific functions that do the same thing, with as many possibilities to call one of them wrong, or have to pick up a dependency on a crypto library just to get e.g. sodium_memzero().

Not arguing with your useless junk attitude. Useless junk is the glibc and its FORTIFY_SOURCE "maybe we catch it or maybe not" attempt.

But sodium_memzero() is not safer than memset_s. If only provides a compiler barrier, but no memory barrier. AFAIK only my safeclib memset_s is "safe", but I don't provide a clflush there. Maybe I should.

All the more reason there should be a function in the C standard that does it properly and uniformly across platforms.

Because that might add a bunch of code that may or may not be useful. It might be that this is inlined everywhere and so would bump some of code to a different alignment. Perhaps it's timing critical and this saved a few calls. Perhaps they use static analysis only. Or they might have debug versions where they do use the safety checks and then for production versions it's preprocessed out.

There could be plenty of different reason.

> Or they might have debug versions where they do use the safety checks and then for production versions it's preprocessed out.

Why would you do that? Most attacks will probably target your production environment rather than your development environment.

One could fuzz test the debug versions to ensure it's safe. And they really need the extra performance for prod. Perhaps they are confident that is enough testing? We know it can never be 100%

To actually fix this, you have to call memcpy_s instead of memcpy, and then not create the wrapper, which seems like almost the same amount of work, but slightly less. So why create the wrapper? Their library didn't have a memcpy_s function?

Performance reasons maybe? Just spitballing here.

Huawei is actually one of the very few vendors which do provide the Annex K functions. I can only name Cisco, Intel, Embarcadero, HP and Microsoft who also do so. But Intel very badly.

The report is right to call out Huawei engineers to bypass these checks. They probably had the same attitude to bounds checks as all open source developers and industry practioners.

> Just stupid programmers or method?

Likely just stupid. I can easily see:

Manager: "Hey, review kicked your code back. You need to use the foo_s versions."

Minion: "But the foo_s versions are in libsecure and the system doesn't build and link that. I need someone with the authority to add it to the build to do that."

Manager: "Not my problem. Get your code clean by the end of the day."

Minion: <decides that renaming the function locally looks pretty good right now>

I bet that's just the usual chinese quality in manufacturing in general, the actual backdoor is just the hardcoded ssh key.

And thats their enterprise infrastructure equipment stuff, can you even imagine how their android firmware must look like, holy shit.

Didn't know Cisco is Chinese...

https://nvd.nist.gov/vuln/detail/CVE-2019-1804 https://www.theregister.co.uk/2019/05/02/cisco_vulnerabiliti...

Reply on news.ycombinator.com