back to article Get ready to patch Git servers, clients – nasty-looking bugs surface

A chap who found two serious security bugs in Git servers and clients has urged people to patch their software. The flaws are present in Git including the 2.x, 1.9 and 1.7 branches, meaning the vulnerabilities have been lurking in the open-source version control tool for years. It is possible these two programming blunders …

  1. Awil Onmearse
    Flame

    strcpy()

    FFS.

  2. Yag
    Joke

    Time to get downvoted to hell...

    Those bugs would have been spotted earlier If Git was open source.

    More seriously... The use of "int" type should be cause for flogging.

    1. Anonymous Coward
      Anonymous Coward

      Re: Time to get downvoted to hell...

      So what is https://github.com/git/git then?

      1. Chronos Silver badge
        Trollface

        Re: Time to get downvoted to hell...

        @AC: Irony. It's a bit like silvery but much cheaper.

    2. Anonymous Coward
      Anonymous Coward

      Re: Time to get downvoted to hell...

      I'd even have said, Linus should let go another volley of insults aimed at some poor developer, but well, he himself did not notice the issues, even though he did patch that very function.

      https://github.com/git/git/commit/cf2ab916afa4231f7e9db31796e7c0f712ff6ad1

  3. James Hughes 1

    I'm not expert in these matters, but how would someone go about finding this exploit if they DIDNT have access to the source code (i.e. grep -R strcpy *). To me, this seems almost impossible to figure out without source code access, but clearly people find stuff in closed source code. How?

    1. Paul Crawford Silver badge

      Fuzzing tools - throw all sorts of sh*t at the program until it breaks then take a look at what the breakage reveals.

  4. Mark Simon

    There are still bugs in such routine tasks?

    The function looks like a pretty hard-coded way of performing what should have been a routine task — concatenating strings.

    I though good coding included packaging routine tasks into reusable functions or methods, so that we can move on from debugging the umpteenth incarnation of the same code.

    1. James Hughes 1

      Re: There are still bugs in such routine tasks? @Mark Simon

      But tell that to the youth of today - and they won't believe you.

    2. oneguycoding

      Re: There are still bugs in such routine tasks?

      Exactly, there's no excuse for us to still be living in a world where C code is written like this. This is some kind of Path.join() or File.join() functionality that is handled in one place by modern languages/libraries. Rewriting this kind of crap, even with safe functions like snprintf(), is absurd in this day and age.

      1. Anonymous Coward
        Anonymous Coward

        Re: There are still bugs in such routine tasks?

        > This is some kind of Path.join() or File.join() functionality that is handled in one place by modern languages/libraries

        Exactly. And those library function implementations may potentially suffer from the same or other vulnerabilities. Just because it's part of the language / standard lib it doesn't make it intrinsically safe. Look elsewhere for the article on Visual Basic's date handling for an example.

  5. Chris Gray 1
    Meh

    question

    In this situation, the space for the final file name is computed using "strlen", so it seems to me that copying the file name into that space using "strcpy" is valid. Both go until a NUL byte is found, and so operate consistently. Am I missing something?

    The issue with using signed ints is valid. However, on today's 64 bit systems you are not going to be able to allocate enough virtual memory to hit the problem. A Google search tells me that non-standard "xmalloc" will abort if it cannot allocate the needed memory, so there isn't a hole there.

    Again, what am I missing here?

    1. Mr Flibble

      Re: question

      So far as I can tell, the allocation of (lengfh+1) bytes for the leafname is fine, as is using strcpy() for that. And yes, size_t or unsigned int should be used.

      As for what you're missing: after each addition, check that the new value is greater than the previous value; if not, bail out, raise the alarm, panic, abort() or something. That said, if the lengths are guaranteed to have been checked at the point at which the pathname list was constructed, you can probably get away without that – however, belt and braces…

  6. Paul Mitchell

    strlen()

    I'd not be too happy about using strlen() directly on user supplied data either.

    1. Will Godfrey Silver badge

      Re: strlen()

      Agreed.

      For strings from an external source I use my own simple unsigned counting scheme, and if it looks like it's about to hit the buffers the remainder all goes to /dev/null. The fact is flagged so the program can decide what to do next.

  7. CommodorePet

    Static Analysis ftw

    Stratic analysis tools would raise a bunch of issues here.

    strlen returns a size_t, which is unsigned. Mixing signed and unsigned with an inequality if statement is a big red flag.

    I agree with an earlier point, concating strings and general path manipulation should be a solved problem.

  8. Stevie Silver badge

    Bah!

    On noes, my Raspberry Pi's knickers are down!

    Luckily, Mr Bonaparte, my boss, refuses to allow us to install Git in order to get control of our script shards.

    We only have the usual exposure to running daily_script_new.sh or new_daily_script.sh instead of daily_new_script.sh.

    1. Stevie Silver badge

      Re: Bah! (no Git alllowed)

      It turns out to be ridiculously easy to write a poor man's Git.

      vi pmgit

      i

      #!/bin/ksh

      ls ${1}*.old | sort -r > filelist

      while read myfile

      do

      cp myfile myfile.old

      done < myfile

      cp $1 $1.old

      (esc)

      ZZ

      chmod 777 pmgit

      Then, as needed: pmgit [filename]

      Job done.

      I was going to add code to make it work in different directories but then I thought "why not just put a copy of pmgit in every directory on the computer and be done?"

  9. Richard Lloyd

    Cygwin git needs updating...

    Yes, the irony of it - Cygwin source code is git-hosted, but their copy of git is version 2.7.0 without any of the fixes mentioned in the article:

    https://cygwin.com/packages/x86_64/git/

    Gitlab's restriction to a minimum of 2.7.3 is a little harsh when a) 2.8 will contain one of the fixes (so another raise of the min version then?) and b) Cygwin git is languishing at 2.7.0.

    On a similar note, CentOS 7 git is actually way back at version 1.8.3.1 (with backported fixes) - does the latest Gitlab lock out CentOS 7 users then? One trick is to rebuild the Fedora Rawhide 2.7.3 src.rpm on CentOS 7 and use it to replace 1.8.3.1 (though it means you have to track Rawhide for updates...).

  10. JoeF

    Slackware apparently backported the fix to 2.7.3

    http://www.slackware.com/security/viewer.php?l=slackware-security&y=2016&m=slackware-security.499727

  11. This post has been deleted by its author

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