Just posted about this to oss-security@, relevant here. SSH command injection via -o... impacts CVS 1.12.x as well, if anybody still cares. The announcement for git (see bug#627488) mentions CVE-2017-1000117, CVE-2017-9800, and CVE-2017-1000116 for git, Subversion, Mercurial, but makes no mention of CVS. CVS uses SSH for remote repos when CVS_RSH=ssh. In which case specifying a hostname of -o... triggers the same sort of thing: $ strace -f -e execve cvs -d '-oProxyCommand=id;localhost:/bar' co yada 2>&1 | egrep id execve("/usr/bin/cvs", ["cvs", "-d", "-oProxyCommand=id;localhost:/bar", "co", "yada"], 0x7ffe69f75a68 /* 139 vars */) = 0 [snip] [pid 20003] execve("/usr/local/bin/ssh", ["ssh", "-oProxyCommand=id;localhost", "cvs server"], 0x5fb1fc8420 /* 141 vars */ +) = -1 ENOENT (No such file or directory) [pid 20003] execve("/usr/bin/ssh", ["ssh", "-oProxyCommand=id;localhost", "cvs server"], 0x5fb1fc8420 /* 141 vars */) = 0 [pid 20004] execve("/bin/bash", ["/bin/bash", "-c", "exec id;localhost"], 0x32af5f10d0 /* 141 vars */) = 0 [pid 20004] execve("/usr/bin/id", ["id"], 0xec92226ae0 /* 141 vars */) = 0 [pid 20004] +++ exited with 0 +++ [pid 20003] --- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=20004, si_uid=3612, si_status=0, si_utime=0, si_stim +e=0} --- ssh_exchange_identification: Connection closed by remote host [pid 20003] +++ exited with 255 +++ --- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=20003, si_uid=3612, si_status=255, si_utime=0, si_stime=0} --- Tested vanilla 1.12.13, and Gentoo 1.12.12-r11. Of course, the repo specification looks very odd, so tricking a victim may be harder than for SCM tools where it's prefixed by an ssh:// or masked behind a redirect. I don't see any recent activity on CVS's mailing lists relative to this; may attempt posting there and/or adapting the patches from git & svn if nobody beats me to it.
hlein: Go ahead and write a quick patch yourself. You'll need to check: * The -d option * $CVSROOT env var * Repository file when it's read. As for validations, it's going to be somewhat more annoying. It can be: * ":${METHOD}:${SOMETHINGSTR}" * "${ABSPATH}" * "${RELATIVE_PATH}" The relative path is going to pose the most challenge: since it it legal to name a directory with a leading dash. Maybe just blacklist "-o" at the start of those commands for now, if CVS_RSH is in the environment and non-empty (it might not be 'ssh', as there are use cases for having an ssh wrapper there).
I have not had time to try it or integrate into an .ebuild yet, but Thorsten Glaser developed a fix for Debian and MirBSD; references: https://www.mirbsd.org/permalinks/wlog-10_e20170811-tg.htm https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=871810 https://www.mirbsd.org/cvs.cgi/src/gnu/usr.bin/cvs/src/rsh-client.c.diff?r1=1.6;r2=1.7 https://www.mirbsd.org/cvs.cgi/src/gnu/usr.bin/cvs/src/root.c.diff?r1=1.11;r2=1.19
Created attachment 489222 [details, diff] Patch to add -- to SSH arguments, preventing hostname of -oProxyCommand, etc.
Created attachment 489224 [details, diff] Ebuild patch to apply the SSH -o fix.
Added a patch that brings in only the rsh-client.c changes from Thorsten Glaser's patches. His are against CVS 1.12.13, but Gentoo is at 1.12.12+patches, including some Gentoo-local security enhancement patches (command filtering). His fixes don't apply cleanly to 1.12.12-r11, and Gentoo's patches don't apply cleanly to 1.12.13, so this is the minimum change to our existing version to address this issue. His patches don't attempt to filter for unwanted things like -o in CVS/Root / $CVSROOT / -d CVSROOT... parameters, instead simply adds a -- after 'ssh' before hostname specification, so that -o... will be treated as a hostname instead of as options, regardless of how it is ingested. This seems to be a safe approach. [The patchset that I did not bring in, to root.c, adds more sanity checking, but again, doesn't apply cleanly to our version so I skipped for now.]
Is there anything I can do to help move this forward?
Seems cvs currently has no maintainer in Gentoo. I have committed a bump with a fix. I have taken the patch from MirBSD, but leaving out the comment changes (they didn't apply cleanly to the upstream code, probably due to other changes made by MirBSD, and they are comment only, so doesn't really matter). We can wait a few days if any issues show up and then stabilize. For the record, minimalized test case: cvs -d '-oProxyCommand=gnome-calculator:/' co a (if gnome-calculator starts you are vulnerable)
(In reply to Hanno Boeck from comment #7) > Seems cvs currently has no maintainer in Gentoo. > > I have committed a bump with a fix. I have taken the patch from MirBSD, but > leaving out the comment changes (they didn't apply cleanly to the upstream > code, probably due to other changes made by MirBSD, and they are comment > only, so doesn't really matter). We can wait a few days if any issues show > up and then stabilize. > > For the record, minimalized test case: > cvs -d '-oProxyCommand=gnome-calculator:/' co a > > (if gnome-calculator starts you are vulnerable) Hanno, looking for the bump but I am not seeing it...
(In reply to Aaron Bauman from comment #8) > Hanno, looking for the bump but I am not seeing it... Forgot git push at first and you were too fast :-) Should be there now: https://gitweb.gentoo.org/repo/gentoo.git/commit/?id=9aae21baa940cba64b9ca3b26a5cdf69e88fdf2b
ia64 stable
amd64/x86 stable
Stable on alpha.
arm stable
sparc stable (thanks to Dakon)
hppa stable (thanks to Dakon)
New GLSA Request filed. @PPC please finish stabilizing. Gentoo Security Padawan ChrisADR
ppc64 stable
This issue was resolved and addressed in GLSA 201709-17 at https://security.gentoo.org/glsa/201709-17 by GLSA coordinator Aaron Bauman (b-man).
re-opening for ppc stable and cleanup.
ppc stable. Last arch is done here.
(In reply to Hanno Boeck from comment #7) > Seems cvs currently has no maintainer in Gentoo. > > I have committed a bump with a fix. I have taken the patch from MirBSD, but > leaving out the comment changes (they didn't apply cleanly to the upstream > code, probably due to other changes made by MirBSD, and they are comment > only, so doesn't really matter). We can wait a few days if any issues show > up and then stabilize. > > For the record, minimalized test case: > cvs -d '-oProxyCommand=gnome-calculator:/' co a > > (if gnome-calculator starts you are vulnerable) I believe this patchset is incomplete. This hunk was dropped: --- cvs-1.12.12.orig/src/rsh-client.c 2005-03-15 10:45:10.000000000 -0700 +++ cvs-1.12.12/src/rsh-client.c 2017-08-15 13:38:29.136095238 -0600 @@ -54,8 +54,9 @@ ? root->cvs_server : getenv ("CVS_SERVER")); int i = 0; /* This needs to fit "rsh", "-b", "-l", "USER", "host", - "cmd (w/ args)", and NULL. We leave some room to grow. */ - char *rsh_argv[10]; + "--", "host", "cvs", "-R", "server", and NULL. + We leave some room to grow. */ + char *rsh_argv[16]; if (!cvs_rsh) /* People sometimes suggest or assume that this should default But this was not a "comment only" change - it also changed from *rsh_argv[10] to*rsh_argv[16]. I'm not sure how much it matters. I've been using my version which did include the [10] -> [16] change so I can't say that [10] won't be a problem - recall that the rest of the patches included adding an -- argument, probably prompting the increase. I don't know if there's any other CVS users left; maybe so, and [10] must not be a problem or they'd have complained. Or maybe I'm the only one, and my local change meant I never used the fix in the main tree.
Hi Hank, I think this is a valid issue, I'll have a look.
Ultimately the best solution would probably be for us to fetch in Debian's patchset. However this requires sorting out which of our patches are already in there (by a quick check most of them, but not all), and which of the remaining ones should be considered obsolete, kept or maybe submitted to debian as well.
(In reply to Hanno Böck from comment #23) > Ultimately the best solution would probably be for us to fetch in Debian's > patchset. However this requires sorting out which of our patches are already > in there (by a quick check most of them, but not all), and which of the > remaining ones should be considered obsolete, kept or maybe submitted to > debian as well. We can do this from a security perspective if needed, but I'd prefer it if somebody with some CVS experience was able to handle it (especially given it's maintainer-needed right now).
Resetting sanity check; keywords are not fully specified and arches are not CC-ed.
Unable to check for sanity: > no match for package: dev-vcs/cvs-1.12.12-r12
(In reply to Hank Leininger from comment #21) > (In reply to Hanno Boeck from comment #7) > > Seems cvs currently has no maintainer in Gentoo. > > > > I have committed a bump with a fix. I have taken the patch from MirBSD, but > > leaving out the comment changes (they didn't apply cleanly to the upstream > > code, probably due to other changes made by MirBSD, and they are comment > > only, so doesn't really matter). We can wait a few days if any issues show > > up and then stabilize. > > > > For the record, minimalized test case: > > cvs -d '-oProxyCommand=gnome-calculator:/' co a > > > > (if gnome-calculator starts you are vulnerable) > > I believe this patchset is incomplete. > > This hunk was dropped: > > --- cvs-1.12.12.orig/src/rsh-client.c 2005-03-15 10:45:10.000000000 -0700 > +++ cvs-1.12.12/src/rsh-client.c 2017-08-15 13:38:29.136095238 -0600 > @@ -54,8 +54,9 @@ > ? root->cvs_server : getenv ("CVS_SERVER")); > int i = 0; > /* This needs to fit "rsh", "-b", "-l", "USER", "host", > - "cmd (w/ args)", and NULL. We leave some room to grow. */ > - char *rsh_argv[10]; > + "--", "host", "cvs", "-R", "server", and NULL. > + We leave some room to grow. */ > + char *rsh_argv[16]; > > if (!cvs_rsh) > /* People sometimes suggest or assume that this should default The function this code is in is preceeded by this comment: /* This is actually a crock -- it's OS/2-specific, for no one else uses it. If I get time, I want to make piped_child and all the other stuff in os2/run.c work right. In the meantime, this gets us up and running, and that's most important. */ And it's only actually compiled if the macro START_RSH_WITH_POPEN_RW is defined. The only definition for it I could find is here: ./os2/config.h:#define START_RSH_WITH_POPEN_RW 1 So the potentially buggy version of this function seems to be OS/2 specific and only compiled when compiling for that platform. Surely that doesn't affect us, right? :)
(In reply to John Helmert III (ajak) from comment #27) > So the potentially buggy version of this function seems to be OS/2 specific > and only compiled when compiling for that platform. Surely that doesn't > affect us, right? :) Thanks for that analysis. I guess we can close it with that :-)
