back to article Did you hear the one about Cisco routers using strcpy insecurely for login authentication? Makes you go AAAAA-AAAAAAArrg *segfault*

Cisco has patched three of its RV-series routers after Pen Test Partners (PTP) found them using hoary old C function strcpy insecurely in login authentication function. The programming blunder can be exploited to potentially hijack the devices. PTP looked at how the routers' web-based control panel handled login attempts by …

  1. Anonymous Coward
    Anonymous Coward

    I'm thinking...

    ...outsourced coding\0

    1. Anonymous Coward
      Anonymous Coward

      Re: I'm thinking...

      Not sure if it was outsourced coding or an acquisition that hasn't had an adequate code review - it's Linux-based home/Soho level router with a web-based interface.

      1. rwessman

        Re: I'm thinking...

        It's probably a lack of code review. In my experience, these kinds of bugs occur frequently in in-house code as well. I've been telling developers to not use strcpy() for years.

  2. Spamfast Bronze badge

    Null suggested latterday C authors might want to switch to strlcpy instead, "a nonstandard function which takes a third length argument, and always null terminates".

    Or better yet, the strncpy_s function that is standard in C11.

    If using a pre-C11 compiler then write your own (from scratch or by encapsulating any provided non-standard one) until you can upgrade your compiler.

    1. Phil O'Sophical Silver badge

      Or better yet, the strncpy_s function that is standard in C11.

      strncpy isn't a good choice, not least because it doesn't null-terminate excessively-long strings. strlcpy guarantees that there will be no overflow, it's a much safer option.

      1. MrReal

        Even the humble snprintf() is pretty good as long as you remember to use the format string, the nice feature it has is nul termination and you can use it for pretty much everything.

      2. Spamfast Bronze badge

        strncpy isn't a good choice, not least because it doesn't null-terminate excessively-long strings.

        I didn't say strncpy, I said strncpy_s. Passed the correct arguments, it guarantees a trailling NUL and provides simpler error checking in the event of an overflow. It also detects overlapping source & destination. Its behaviour is precisely defined by the standard.

        strlcpy is not portable or defined in any C standard, although one might wish it was given its simpler signature. So its behaviour is not predictable when porting. *shrug* One could add tests of its behaviour to the unit test suite for every project I suppose.

        But we're getting into nerdland here so I'll leave it at that.

    2. Anonymous Coward
      Anonymous Coward

      Original bad designs in C biting back... how many different functions are they going to add trying to address them, before understanding that the lack of a string intrinsic type properly managed will solve most of those problems? Keep on thinking manipulating arbitrary buffers when there's really no need, and this vulnerabilities will keep on surfacing.

      Strings could have been a lesser need in the mainframe era, but today too much became a string (even when not really needed, thanks to the web and its clueless protocols and developers), complicating their manipulations is just a big vulnerabilities source.

    3. Paul Crawford Silver badge

      Sadly strncpy_s is not that standard other than for MS' own compiler. Using it paints you in to an MS-only corner or having to use some unusual gcc varient. Unfortunately the strlcpy() variant mentioned is also not a general standard, so again using it paints you in to the corner for coding.

      As the article points out, there are already standard options for this, even decades ago there was strncpy() that copied with a length-limit, but that has the issue of not enforcing nul-termination if you use the full buffer length. I generally always set the last byte of the known-size array to zero afterwards just in case.

      Tools like the synopsis "Coverity Scan Static Analysis" would pick that sort of thing up, and it is utterly inexcusable that a company of such size and price-tag is not checking code automatically.

      1. DCFusor Silver badge
        Unhappy

        A simple text search

        of the source would have revealed an unsafe use of the function, known to be bad news for decades now.

        Total Irresponsibility To Show Usual Prudence. I've used your approach myself along with a couple of other trivial tricks to totally prevent stack corruption - since long before the big issue was someone malicious trying to do it.

        I mean, that should have been proactively searched for and fixed quite a long time ago....that's plain irresponsible, it wouldn't take a whole dev-day to do all their source that way....unless their system is really an example of utter borkage.

        Either way, it's a sign you should vote with your wallet. No, of course.

      2. gnasher729 Silver badge

        Now can anybody recommend a function that will copy UTF-8 strings and not leave an invalid mess if the first two bytes of a three byte sequence fit, but not the third one?

  3. MiguelC Silver badge

    Meanwhile...

    ..the US have several high ranking official (including FCC's Ajit Pai and Defence, State and Commerce depts.) on an European tour trying to convince / arm-twisting governments into not using Huawei's 5G equipment and instead trust American makers... Yeah, right!

    1. Anonymous Coward
      Anonymous Coward

      Re: Meanwhile...

      Fortunately the Chinese government promotes null terminating all errors, both software and human.

    2. lartwielder

      Re: Meanwhile...

      Yeah. Jump from the Huawei Chinese intelligence taps frying pan to the NSA taps fire . . . Use of the word "trust" should be banned when discussing anything that involves communications devices produced by US companies unless preceded by the word "not." None have provided any evidence that they /*can*/ be trusted and all, at one time or another demonstrated that they cannot be. One might begin by following the breadcrumbs that show up when one Googles "Room 641A."

  4. cornetman

    That should be memcpy(), not memcopy().

    1. Jim Mitchell

      memcpy() isn't quite the same issue as strcpy(). memcpy() has the number of bytes to copy as an input parameter. It is a bit harder to screw that up than strcpy(), which figures when to stop based on the input data to copy. Just apparently not hard enough to not screw up...

  5. JohnFen Silver badge

    Yes, but

    "PTP's Null suggested latterday C authors might want to switch to strlcpy instead, "a nonstandard function which takes a third length argument, and always null terminates""

    That can work unless, like me, you're working on code that must be able to compile on a wide variety of operating systems and platforms. In those cases, nonstandard functions must be avoided, or you need to implement them yourself rather than using library functions (which is rather expensive in such circumstances).

    The better solution is to hire competent programmers and rigorously enforce code reviews to catch this kind of nonsense.

    1. Anonymous Coward
      Anonymous Coward

      Re: Yes, but

      Tried this, doesn't work.

      Even the brightest engineers occasionally make mistakes, and unfortunately when you make a mistake anything can happen from a benign issue to accidentally launching the nukes.

      The solution to this problem is fairly well understood, you design the language to make these sorts of bugs impossible. There is a slight memory overhead to achieve this, but performance wise the impact is fairly minimal because often the compiler can often optimize the bound checks away.

      1. JohnFen Silver badge

        Re: Yes, but

        "Even the brightest engineers occasionally make mistakes"

        Absolutely true. That's why we have a need for code reviews and comprehensive testing.

        "The solution to this problem is fairly well understood, you design the language to make these sorts of bugs impossible."

        That's not really a solution, big picture. It's just playing whack-a-mole. You can design languages to make certain sorts of mistakes impossible (often at a fairly high cost), but you can't design language to make all forms of serious mistakes impossible. So, although using "safer" languages can help, they are not comprehensive solutions. The real solution is to engage in proper engineering practices (one of which is to use the right tool for the job).

    2. Paul Crawford Silver badge

      Re: Yes, but

      Use strncpy() and force the last byte to zero. You could even write a macro to implement that, or an inline function to be used.

      static inline char *strlcpy(char *buf, const char *src, size_t n) {

      strncpy(buf, str, n);

      buf[n - 1]= '\0';

      return buf;

      }

  6. Persona

    Time gentlemen

    Isn't it about time strcpy was removed from the library? . Yes, that would mean that some old legacy code wouldn't link, but it would also mean that old insecure legacy code wouldn't link without much needed maintenance.

    1. JohnFen Silver badge

      Re: Time gentlemen

      "it would also mean that old insecure legacy code wouldn't link without much needed maintenance."

      Just because code is old, or just because it uses strcpy(), doesn't mean that it's insecure or in need of maintenance.

      1. MrReal

        Re: Time gentlemen

        It's extremely rare that strcpy is needed - it's fast but you have to be super careful, 99/100 times the speed is irrelevant and snprintf is the better option.

        1. JohnFen Silver badge

          Re: Time gentlemen

          I think that you're a bit too dismissive about the importance of speed, but that likely depends on the sort of applications you're developing.

          "it's fast but you have to be super careful"

          I agree. But you have to be super careful in all the code you write anyway. Or you should be. Using a more protected function may be safer, but if you think that means you don't have to be as vigilant or cautious in your coding, then I think you're making a big mistake.

    2. cornetman

      Re: Time gentlemen

      The various popular compilers are spewing out warnings these days about using insecure-by-design functions. If you're writing new code, there are better alternatives.

    3. Crazy Operations Guy Silver badge

      Re: Time gentlemen

      My vote would be to allow it, but at least emit a warning (Although I suppose that wouldn't help with the incompetent developers that just disable all warnings, but there is no helping them...)

      1. This post has been deleted by its author

  7. Crazy Operations Guy Silver badge

    At least implement W^X, FFS

    It baffles me how so many developers, even ones writing security-sensitive code, will just turn off all security features and never bother to turn them back on. Like, sure, turn off the NX/XD bit and ASLR, etc during development to get proper debugging, but once the code is working without them, they should be turned back on and the code retested before release.

    1. Baldrickk Silver badge

      Re: At least implement W^X, FFS

      If anything, should only be disabled for debug builds. Release builds should have that left turned on.

      And debug builds should never be released.

  8. Will Godfrey Silver badge
    Facepalm

    Here we go again

    In case anyone forgot...

    Hello buffer my old friend,

    I've come to break you once again,

    'cos my malware soft-ly creeping.

    Broke your code while you were sleep-ing.

    And the spyware that it put there’s just the same,

    It still remains.

    And takes it all,

    in silence.

  9. Wellyboot Silver badge
    Unhappy

    Not Just the RV using linux these days.

    >>>"some form of embedded Linux" instead of Cisco OS<<<

    Quite a lot of Cisco high end tat has also used some form of linux underneath for a while, the rock solid IOS of yesteryear (runtime in years) is just a happy memory. Now I know linux isn't the problem here, I can only assume Cisco have gone cheap with hiring replacement developers as the old crew retire.

    The number of serious bugs we've been dealing with for the last few years is terrible, the craziest one caused the switch CPU to run at 100% constantly and you had a 1 in 10 chance of a stack not returning from a warm boot. (great news if you are in a different continent doing out of hours maintenance)

    1. sitta_europea

      Re: Not Just the RV using linux these days.

      "The number of serious bugs we've been dealing with for the last few years is terrible..."

      It's because the bean counters are running the show now.

      Exactly the same problem at Cadbury.

      I don't buy Crunchie any more, and until I was 50 I probably averaged one a day for my entire life.

      1. WallMeerkat Bronze badge

        Re: Not Just the RV using linux these days.

        Cadbury, where did it all go wrong. (Actually the Kraft buyout was the exact moment)

        Creme eggs now are a shadow of their former self. Even the easter eggs, which used to be proper dairy milk, are now just a cocoa tasting lard hollow ovoid.

        I've been looking over the pond where the bean counters are in charge of Ford. They had a few saloon cars on old platforms, and heavily marketed their SUVs. They've now axed the saloons because they can eke more profit from an SUV which doesn't need to meet stringent regulations as it is technically classed as a truck.

    2. WallMeerkat Bronze badge

      Re: Not Just the RV using linux these days.

      Certainly the Enterprise targeting Nexus devices run Linux natively, with a CLI shell, then a Linux container 'guestshell' as a bit of a sandbox.

      Was recently involved with porting a devops tool away from using a native Linux agent, to instead communicating with the device remotely. Telcos generally don't like 3rd party executables on their gear.

  10. Mike 16 Silver badge

    Important word

    Newspaper editors sometimes called it the "million dollar word":

    "entire contents of a [ALLEGEDLY] zero-terminated buffer"

    Trust, but verify.

POST COMMENT House rules

Not a member of The Register? Create a new account here.

  • Enter your comment

  • Add an icon

Anonymous cowards cannot choose their icon

Biting the hand that feeds IT © 1998–2019