13 ms·

"On dozens of occasions, Huawei engineers disguised known unsafe functions (such as memcpy) as the “safe” version (memcpy_s) by creating wrapper functions with

"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?

dalore 7y ago
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.
- mikeash 7y ago
  Right, but why not take thirty seconds to do the appropriate checks in the wrappers?
  - acqq 7y ago
    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 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."
    - mikeash 7y ago
      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.
      - userbinator 7y ago
        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.
      - microtherion 7y ago
        It's possible that they are calling the functions, but passing incorrect length arguments so frequently that they are best ignored.
    - zrm 7y ago
      > 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().
      - rurban 7y ago
        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.
        zrm 7y ago
        All the more reason there should be a function in the C standard that does it properly and uniformly across platforms.
  - dalore 7y ago
    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.
    - ptx 7y ago
      > 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.
      - dalore 7y ago
        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%
ru999gol 7y ago
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.
- rndgermandude 7y ago
  Didn't know Cisco is Chinese... https://nvd.nist.gov/vuln/detail/CVE-2019-1804 https://nvd.nist.gov/vuln/detail/CVE-2019-1804 https://www.theregister.co.uk/2019/05/02/cisco_vulnerabilities/ https://www.theregister.co.uk/2019/05/02/cisco_vulnerabiliti...
  - ru999gol 7y ago
    no, NSA backdoors are of course ok, they are the good guys
gcbw2 7y ago
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.
- rossdavidh 7y ago
  ...and that right there is Huawei, Boeing, Exxon, BP, and a thousand other big corporate dumpster fires in a nutshell.
  - flattone 7y ago
    Dumpster fires are everywhere not just large companies: contractors, restaurants, marriages etc.
    - solotronics 7y ago
deleted 7y ago
[deleted]
fencepost 7y ago
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.
- JohnJamesRambo 7y ago
  So extremely, criminally awful?
- userbinator 7y ago
  Except that forcing use of memcpy_s does nothing to actually fix the real problem. Enforce arbitrary rules, get arbitrary results...
AnimalMuppet 7y ago
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?
- nindalf 7y ago
  Performance reasons maybe? Just spitballing here.
- deleted 7y ago
  [deleted]
- rurban 7y ago
  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.
stefan_ 7y ago
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.
- userbinator 7y ago
  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.
bsder 7y ago
> 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>
phkamp 7y ago
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) or 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"
- Buge 7y ago
  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.

dalore 7y ago

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.

mikeash 7y ago
Right, but why not take thirty seconds to do the appropriate checks in the wrappers?
- acqq 7y ago
  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 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."
  - mikeash 7y ago
    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.
    - userbinator 7y ago
      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.
    - microtherion 7y ago
      It's possible that they are calling the functions, but passing incorrect length arguments so frequently that they are best ignored.
  - zrm 7y ago
    > 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().
    - rurban 7y ago
      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.
      - zrm 7y ago
        All the more reason there should be a function in the C standard that does it properly and uniformly across platforms.
- dalore 7y ago
  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.
  - ptx 7y ago
    > 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.
    - dalore 7y ago
      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%

ru999gol 7y ago

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.

rndgermandude 7y ago
Didn't know Cisco is Chinese... https://nvd.nist.gov/vuln/detail/CVE-2019-1804 https://nvd.nist.gov/vuln/detail/CVE-2019-1804 https://www.theregister.co.uk/2019/05/02/cisco_vulnerabilities/ https://www.theregister.co.uk/2019/05/02/cisco_vulnerabiliti...
- ru999gol 7y ago
  no, NSA backdoors are of course ok, they are the good guys

gcbw2 7y ago

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.

rossdavidh 7y ago
...and that right there is Huawei, Boeing, Exxon, BP, and a thousand other big corporate dumpster fires in a nutshell.
- flattone 7y ago
  Dumpster fires are everywhere not just large companies: contractors, restaurants, marriages etc.
  - solotronics 7y ago

deleted 7y ago

[deleted]

fencepost 7y ago

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.

JohnJamesRambo 7y ago
So extremely, criminally awful?
userbinator 7y ago
Except that forcing use of memcpy_s does nothing to actually fix the real problem. Enforce arbitrary rules, get arbitrary results...

AnimalMuppet 7y ago

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?

nindalf 7y ago
Performance reasons maybe? Just spitballing here.
deleted 7y ago
[deleted]
rurban 7y ago
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.

stefan_ 7y ago

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.

userbinator 7y ago
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.

bsder 7y ago

> 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>

phkamp 7y ago

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) or 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"

Buge 7y ago
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.