[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [Issue 1796] defective or malicious client can corrupt repository log messages

From: Daniel Shahaf <d.s_at_daniel.shahaf.co.il>
Date: Sat, 24 May 2008 18:58:22 +0300 (Jerusalem Daylight Time)

Neels Janosch Hofmeyr wrote on Sat, 24 May 2008 at 17:04 +0200:
> Daniel Shahaf wrote:
> > neels_at_tigris.org wrote on Fri, 22 May 2008 at 22:04 -0000:
> >> http://subversion.tigris.org/issues/show_bug.cgi?id=1796
> >>
> >
> > According to my tests, 'svn' doesn't normalise log messages to CRLF now
> > (though it would be nice if it did), so 'svn ci -m "foo\r\nbar"' or svn
> > ci -F should do.
>
> Well, according to my tests ;), it does. Let's break down the different
> cases, using r31375:
>
> === (1) ===
> Try and pass commandline message.
>
> [trunk] neels_at_dub:~/elego/svn/test/wc/r1796
                                        ^
Interesting directory name :)

> $ svn ci -m "log\n\rmessage\n\r"
                  ^^^^
That's LFCR, not CRLF. I suppose you did that intentionally?

> Sending x
> Transmitting file data .
> Committed revision 5.
>
> [trunk] neels_at_dub:~/elego/svn/test/wc/r1796
> $ svn log x
> - ------------------------------------------------------------------------
> r5 | (no author) | 2008-05-24 16:03:27 +0200 (Sat, 24 May 2008) | 1 line
>
> log\n\rmessage\n\r
>
> So, the `\n\r' are taken literally. They are just backslash, then `n'
> etc, no CR or LF characters. Does your shell do this differently?

No:

    % echo "foo\r\nbar\r\n" | file -
    /dev/stdin: ASCII text, with CRLF, LF line terminators

    % /bin/echo "foo\r\nbar\r\n"
    foo\r\nbar\r\n

> How would I make a -m "message" parameter with CR+LF in it? (like echo
> -e)
>

-m "`printf`" ? -m "`cat CRLF-file`" ? python -c 'system()' ? Or throw
it in a run_svn() call and see if it works :)

> (this case is thus unverified)
>
>
> === (2) ===
> Use a message file. (use invalid/scrambled line endings)
>

I didn't test with mixed line endings before. When I do I get the same
error you get.

> [trunk] neels_at_dub:~/elego/svn/test/wc/r1796
> $ echo -e "log\n\rmessage\n\r" > msgfile
>
> [trunk] neels_at_dub:~/elego/svn/test/wc/r1796
> $ cat -A msgfile
> log$
> ^Mmessage$
> ^M$
>
> [trunk] neels_at_dub:~/elego/svn/test/wc/r1796
> $ svn ci -F msgfile
> subversion/libsvn_client/commit.c:919: (apr_err=135000)
> svn: Commit failed (details follow):
> subversion/svn/util.c:660: (apr_err=135000)
> svn: Error normalizing log message to internal format
> subversion/libsvn_subr/subst.c:735: (apr_err=135000)
> svn: Inconsistent line ending style
>
>
> === (3) ===
> Use external editor and scrambled line endings.
>
> [trunk] neels_at_dub:~/elego/svn/test/wc/r1796
> $ svn ci
> subversion/libsvn_client/commit.c:919: (apr_err=135000)
> svn: Commit failed (details follow):
> subversion/svn/util.c:448: (apr_err=135000)
> svn: Error normalizing edited contents to internal format
> subversion/libsvn_subr/subst.c:735: (apr_err=135000)
> svn: Inconsistent line ending style
> subversion/svn/util.c:581: (apr_err=135000)
> svn: Your commit message was left in a temporary file:
> subversion/svn/util.c:581: (apr_err=135000)
> svn: '/arch/elego/svn/test/wc/r1796/svn-commit.2.tmp'
>
>
> === (4) ===
> There's more still; Above CR/LF sequences are not the usual ones. Line
> ending styles in above messages were mixed/scrambled. When I use the
> usual (dos) sequence for CR+LF consistently, the commit succeeds:
>

That's the case I tested.

> [trunk] neels_at_dub:~/elego/svn/test/wc/r1796
> $ echo -ne "log\r\nmsg\r\n" > msgfile
>
> [trunk] neels_at_dub:~/elego/svn/test/wc/r1796
> $ cat -A msgfile
> log^M$
> msg^M$
>
> [trunk] neels_at_dub:~/elego/svn/test/wc/r1796
> $ echo y >> x
>
> [trunk] neels_at_dub:~/elego/svn/test/wc/r1796
> $ svn ci -F msgfile
> Sending x
> Transmitting file data .
> Committed revision 10.
>
> [trunk] neels_at_dub:~/elego/svn/test/wc/r1796
> $ svn log x | cat -A
> - ------------------------------------------------------------------------$
> r10 | (no author) | 2008-05-24 16:19:00 +0200 (Sat, 24 May 2008) | 3 lines$
> $
> log$
> msg$
> $
> ...
>
> And the log message is normalized correctly.
>

Checked again; same here. (Don't know why I said before that it wasn't
normalized. Maybe I didn't check correctly; this time I checked by
editing the revprops file.)

>
> === (5) ===
> Try just CR line endings, without LF.
>
> [trunk] neels_at_dub:~/elego/svn/test/wc/r1796
> $ echo -ne "log\rmsg\r" > msgfile
> [trunk] neels_at_dub:~/elego/svn/test/wc/r1796
> $ cat -A msgfile
> log^Mmsg^M[trunk] neels_at_dub:~/elego/svn/test/wc/r1796
> $ svn ci -F msgfile
> Sending x
> Transmitting file data .
> Committed revision 11.
> [trunk] neels_at_dub:~/elego/svn/test/wc/r1796
> $ svn log x | cat -A
> - ------------------------------------------------------------------------$
> r11 | (no author) | 2008-05-24 16:50:09 +0200 (Sat, 24 May 2008) | 3 lines$
> $
> log$
> msg$
> $
> ...
>
> So, this also works.
>
>

Okay.

> =====
>
> In summary: the current svn client (trunk) normalizes commit log
> messages that are in pure CR+LF or CR format to become LF format.
> However, if the line ending styles are mixed or have the wrong order,
> the svn client complains about "inconsistent line endings" and aborts
> the commit.
>
>
> >> Can you imagine a nice and easy way of doing this? All I can think of is
> >> netcatting recorded transaction data to svnserve... :P
> >
> > (have you thought of writing a C test?)
>
> Appears that I am ignorant of this option :P
> Could you point me at a, preferably simple, example where C is being
> used for an existing subversion test?
>

subversion/tests/libsvn_subr/*.c (e.g. path-test, time-test)
subversion/tests/README

> > Until normalisation is implemented, see above. After normalisation is
> > implemented, test the normalisation. :)
>
> done :)
>

Thanks.

>
> >> ------- Additional comments from neels_at_tigris.org Thu May 22 15:15:58 -0700 2008 -------
> >> Is there or may there ever be a revprop that is allowed to have CR ('\r') in its
> >> value?
> >
> > (1) user-defined revprops (which are taken as opaque binary strings)
> > (2) Not sure; it might be allowed as 'whitespace' in svn:mime-type and
> > friends (unless they're internally normalized as well?)

(I'm still not sure on (2), so I'll let someone else comment on your
next point.)

>
> I am intending to test only "svn:" props for CR characters and reject
> any that contain some. Maybe I should just write the patch and run some
> tests to see what happens.
>
>

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-05-24 17:58:50 CEST

This is an archived mail posted to the Subversion Dev mailing list.