Hi,
while implementing the rsync protocol in one of our projects I found
that the current CVS version still has a MD4 bug. I'm using the FreeBSD
libmd implementation and I still had checksum mismatches with protocol
version 27 for files whose size was a multiple of 64 - 4 ( - 4 due to
checksum_seed). A patch for todays CVS version is attached.
Someone should also review the clean_fname() function in utils.c.
I think it will not produce the intended result for constructs like
"./////foo" or "bar/././". The former case might pose a
security risk.
I think the first two ifs should be whiles.
Regards,
Christoph
PS: Just let me say that having to run clean_flist() on both client and
server _after_ the file list has been transmitted is a real PITA.
--
Christoph Bartelmus mailto:bartelmus@eyeled.de
Eyeled GmbH, Science Park Saar http://www.eyeled.de
Stuhlsatzenhausweg 69 phone: +49-(0)681-3096-114
66123 Saarbr?cken fax: +49-(0)681-3096-119
-------------- next part --------------
Index: checksum.c
==================================================================RCS file:
/cvsroot/rsync/checksum.c,v
retrieving revision 1.25
diff -u -r1.25 checksum.c
--- checksum.c 10 Apr 2003 01:50:12 -0000 1.25
+++ checksum.c 7 May 2003 14:27:06 -0000
@@ -184,7 +184,7 @@
void sum_end(char *sum)
{
- if (sumresidue) {
+ if (sumresidue || remote_version >= 27) {
mdfour_update(&md, (uchar *)sumrbuf, sumresidue);
}
On Thu, 2003-05-08 at 01:02, Christoph Bartelmus wrote:> Hi, > > while implementing the rsync protocol in one of our projects I found > that the current CVS version still has a MD4 bug. I'm using the FreeBSD > libmd implementation and I still had checksum mismatches with protocol > version 27 for files whose size was a multiple of 64 - 4 ( - 4 due to > checksum_seed). A patch for todays CVS version is attached.It is not that simple for rsync... it needs to be backwards compatible with the busted implementations. I believe some patches have been submitted that do this, and they are probably in the TODO list for submitting to CVS. BTW, if you are using the libmd implementation, I have optimized the hell out of the md4 implementation based on the code that came from rsync via librsync, and modified it to use the libmd API. I have submitted it to the maintainer of http://www.penguin.cz/~mhi/libmd/ (Martin Hinner), but I have had no feedback. My version is bug free (passes libmd tests) and nearly 2x as fast on i386 as the RSA implementation in libmd. This implementation is available at; http://minkirri.apana.org.au/~abo/projects/libmd/ Please use/promote/test/feeback etc. -- ---------------------------------------------------------------- Donovan Baarda http://minkirri.apana.org.au/~abo/ ----------------------------------------------------------------
On Thu, May 08, 2003 at 02:39:57AM +1000, Donovan Baarda wrote:> It is not that simple for rsync... it needs to be backwards compatible > with the busted implementations. I believe some patches have been > submitted that do this, and they are probably in the TODO list for > submitting to CVS.No, it has already been checked in. So, it looks like his patch is really all that is needed as long as the change in the algorithm is valid (which I did not attempt to verify) -- i.e. protocol 27 is already using a bug-fixed MD4 algorithm, and we should make it as bug-free as possible before the next release. ..wayne..
On Wed, May 07, 2003 at 10:26:57AM -0700, Wayne Davison wrote:> On Thu, May 08, 2003 at 02:39:57AM +1000, Donovan Baarda wrote: > > It is not that simple for rsync... it needs to be backwards compatible > > with the busted implementations. I believe some patches have been > > submitted that do this, and they are probably in the TODO list for > > submitting to CVS. > > No, it has already been checked in. So, it looks like his patch is > really all that is needed as long as the change in the algorithm is > valid (which I did not attempt to verify) -- i.e. protocol 27 is > already using a bug-fixed MD4 algorithm, and we should make it as > bug-free as possible before the next release.OK. Two issues here. 1. Is this fix correct to give us full md4 compliance? Craig? 2. If so, do we bump to protocol 28? It has been a month since protocol 27. This would break things for those with cvs versions since then. I was going to describe it as a rare corner case (length % 64 == 4) but realized that amounts to more than 1%. The issue is whether to break things for those few who are running cvs that would mix cvs versions or to hasten the day when 2.5.6 becomes incompatable. Lets hear it. -- ________________________________________________________________ J.W. Schultz Pegasystems Technologies email address: jw@pegasys.ws Remember Cernan and Schmitt
On Wed, May 07, 2003 at 03:56:32PM -0700, jw schultz wrote:> 2. If so, do we bump to protocol 28? It has been a month > since protocol 27.I also consider the one protocol bump (from 26 to 27) to be good enough for everything that has or will get put into the next release, including any fine-tuning of the revised MD4 algorithm that we need to do. Yes, this will mean that current testers will have to be careful to upgrade all their CVS-build rsync binaries at the same time in order to avoid any protocol-27 incompatibilities... ..wayne..
> while implementing the rsync protocol in one of our projects I found > that the current CVS version still has a MD4 bug. I'm using the FreeBSD > libmd implementation and I still had checksum mismatches with protocol > version 27 for files whose size was a multiple of 64 - 4 ( - 4 due to > checksum_seed). A patch for todays CVS version is attached.You are right. There are three interfaces to checksum.c: the block checksum (get_checksum2), the file checksum (file_checksum) and the cumulative md4 (sum_init/sum_update/sum_end). I missed sum_end. I tested the block and file checksums for a wide range of sizes, but not sum_end. Ouch. Your patch looks correct. I vote for keeping the protocol version at 27. This might trip up mixed CVS versions, but that is a short term issue and it would be unpleasant to keep indefinite support for yet another slightly broken MD4. Craig