Hi Robert, On Wed, Jan 23, 2008 at 06:36:17PM -0500, Robert Hardy wrote: > Please find attached and included below a patch that fixes rssh > compatibility with rsync protocol 30. This works for me and should be > backwards compatible. It assumes the Posix regex libraries are present.
Thanks for your post. First, let me say that I don't intend to
release a new version of RSSH, ever... perhaps barring a fix for some
undiscovered serious security flaw. I've long since lost interest in
the project. I guess I would hope that if it needed updates to be
compatible with newer software, and people cared about that, that
someone else would pick up maintaining it. It's free code, in the
purest of senses, so anyone who wants to is welcome to do that. So
perhaps it's nice that your patch will remain documented in the
mailing list archives.
That said, were I going to release a new version, at a quick glance I
think your code would need a few small changes before I would
incorporate it into RSSH. CAVEAT: I have not looked at this very
closely, don't remember the specifics of the posix regex libs, and
don't write code much these days. As such, every one of my comments
which follow may be completely wrong. ;-) Use them as you see fit.
It's also very possible that I've overlooked something...
Now:
> +// char *token;
You're using C++ comments, where there are no lines of C++ code
anywhere in the project. I believe there are C compilers which will
choke on this code, including GCC if the user decides to compile with
certain flags. Even if that's not true, I think it's kind of bad
form.
> + char *cl_epattern = "e[0123456789]+.[0123456789]+";
I believe the '.' in this string needs to be quoted with a '\',
otherwise it matches any character. It's probably not a big deal, but
(unless I'm mistaken) it's technically wrong. Also what happens if
the version gets changed to something like 12.34.56? It may be
harmless and also more compact to write the regex as:
"e[0-9.]+" /* no need to quote the '.' in this case */
> + if ( strstr(cl, "--server" ) ){
> + if ( opt_exist(cl, 'e') ){
> + if( regexmatch(cl, cl_epattern) ) {
> +// fprintf(stderr, "\ncl matches
> cl_eparttern allowing access with -e argument");
> +// log_msg("cl matches cl_eparttern
> allowing access with -e arg");
> + } else {
> + fprintf(stderr, "\ninsecure -e
> option not allowed.");
> + log_msg("insecure -e option in rsync
> command line!");
> + return NULL;
> + }
> + }
> + } else {
> + if ( opt_exist(cl, 'e') ){
> + fprintf(stderr, "\ninsecure -e option not
> allowed.");
> + log_msg("insecure -e option in rsync command
> line!");
> + return NULL;
> + }
I believe this if block can more efficiently be written as a single
compound if conditional, with a single copy of the else clause, i.e.:
if ( strstr(cl, "--server") && opt_exist(cl, 'e') &&
regexmatch(cl, cl_epattern){
[...]
Also you have a type-o in your log message ("cl_eparttern"), though I
think probably it would be better to explain in English what this
means, rather than refer to the variable name.
Though, thinking about it as I wrote that, I think what I'd actually
do is leave the if block, but use a different error message for the
first else clause, something like "-e used with --server (allowed) but
insecure argument provided: %s"
FWIW, I think it's kind of sad the rsync folks decided to implement it
this way... I consider it at least slightly brain-damaaged to reuse a
command-line option for a different purpose, especially one as
potentially dangerous as this (depending on the usage) which is used
to specify a program to run on the remote machine... But that's me.
Take my comments (or leave them) as you see fit. :)
--
Derek D. Martin
http://www.pizzashack.org/
GPG Key ID: 0x81CFE75D
pgpMV5cxNsI30.pgp
Description: PGP signature
------------------------------------------------------------------------- This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2008. http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________ rssh-discuss mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/rssh-discuss
