[Cvs-dev] Re: cvs-passwd patch

[Mark D. Baushke]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Prasad,

Be advised that I am answering some of your questions and also
continuing with the passwd.c code review below.

P J P <address@hidden> writes:

>    Hello Mark, I value your remarks, and am really thankful to you for
> the time & efforts, you are putting in.

I understand that this is frustrating for you, but I would rather see
good code added for this feature than a hack posted to the net that
gives folks problems because it is not in the supported code base.

> On Thu, 12 Oct 2006, Mark D. Baushke wrote:
> > - configure and configure.in
> >   You didn't bother to incorporate my fix to your configure.in file to
> >   use a tab instead of four spaces for the flockfile and funlockfile
> >   ditto for the change to configure.
> 
>      What? No! I'm sorry!! But, I'd made the very changes to it. I'll
> just double check with it.
> 
> > - passwd.c ... formatting content
> >   * Missing a Copyright (C) 2006 ... line
> 
>      What 'exactly' should I write there? I've added the following
> 
>      " Copyright (C) 2006 The Free Software Foundation, Inc. "
> 
>      Do I have to mention my name also??

The code is yours. You may assign the copyright to the Free Software
Foundation if you wish, or you may release it under your own copyright
and license according to the GPL. If you have questions about this legal
issue, please consider consulting a lawyer. I would just like to see
some assertion made lest it be encumbered by the Berne Convention and
treaty law before we consider adding the code to CVS.

http://www.fsf.org/licensing/licenses/gpl.html
http://www.fsf.org/licensing/licenses/gpl-howto.html

One of the following is may be intersting as an example:

/*
 * Copyright (C) 2006 The Free Software Foundation, Inc.
 *
 * Portions Copyright (C) 2006, Prasad J Pandit
 *
 * You may distribute under the terms of the GNU General Public License as
 * specified in the README file that comes with the CVS source distribution.
 *
 * Facilitate user changes to cvs password from the client side using
 * cvs command line interface. Permit cvs administrator password
 * management.
 */

OR

/*
 * Copyright (C) 2006 Prasad J Pandit
 *
 * You may distribute under the terms of the GNU General Public License as
 * specified in the README file that comes with the CVS source distribution.
 * 
 * Facilitate user changes to cvs password from the client side using
 * cvs command line interface. Permit cvs administrator password
 * management.
 */

I cannot tell you which copyright you should use. I can say that the
code would best be licensed under the GNU GPL as given in the above
text if you expect to see it incorporated in CVS.

For what it may be worth, I have assigned all of my copyrights for CVS
code to the Free Software Foundation. Your goals and situation may be
different than mine.

> > - passwd.c implementation
> >   * check_option
> >     You should not just assign NULL to rusr and dpass. Instead you should
> >     free any memory that might be in those pointers first. It is entirely
> >     possible for a conforming implementation to send more than one
> >     passwd command through the client/server connection OR to have more
> >     than one option on the command line.
> 
>     Ummmn, I'm afraid, I didn't get it quite.

The patch included at the end of my message shows one possible approach.

The big problem is on the server side, but you might also see problems
on the client side if a user does something like this:

    cvs passwd -D domain1 -D domain2

where your original could would see a memory leak due to your not
free()ing the previous rusr or dpass values if they are non-NULL.

Consider the case where a client connects to the server and sends one
passwd command which completes and then before disconnecting from your
server, sends another passwd command. Yes, the current CVS client does
not do this. However, the protocol allows a client to do this should it
so desire, so your server code needs to be paranoid about the
possibility.

I hope this describes the problem for you.

>    - The check_option() is called by both, the client and the server, in
>      the very begining, in-fact it's the *first* function called by them,
>      and that too only once in their life time.

True, but a client/server connection may issue many different passwd
command operations in one connection.

>    - The 'rusr' & 'dpass' are not expected to point to any legitimate
>      memory area, that early in the program, and explicit assignment of
>      NULL just ensures the same.

You do not free them at the end of your command processing AND you do
not deal with the possibility that a client might send multiple -D
options to the server during the same session. This is known as a
potential memory leak.

>    - Sending multiple options(switches), at the same time, is okay. I've
>      checked it, one can even send a command like
> 
>      '$ cvs -d $CVSROOT passwd -a -D xxxx -e -r xxxx -R -x -X username'
> 
>      but, sending more that one 'passwd' command? at the same time, how??

You yourself can write a server test that does this

"$servercvs --allow-root=${CVSROOT_DIRNAME} pserver" \
"${DOTSTAR}I LOVE YOU
ok" << EOF
BEGIN AUTH REQUEST
${CVSROOT_DIRNAME}
testme
Ay::''d
END AUTH REQUEST
Root ${CVSROOT_DIRNAME}
Argument -a
Argument $tmpusr
passwd 
Argument -D domain
Argument $tmpusr
passwd 
Argument -D domain2
Argument -D domain3
Argument -D domain4
Argument -D domain5
Argument $tmpusr
EOF

There are many implementations of the CVS client. CVSNT, JCVS, CVS to
name a few. In addition, you never know when some black-hat will be
attempting to cause a Denial of Service against your :pserver: server
just to see if they can do it.

In the past, vulnerabilities in :pserver: gave crackers inappropriate
access through CVS to the server running it. It would be nice if your
addition did not add fuel to the fire.

> >   * passwd () I still do not see a check for
> >     'supported_request ("passwd-encrypt")' or whatever it is to be called,
> >     before you allow the -e flag to be sent to the server. I think this
> >     has been mentioned in every code review so far. It is quite frustrating
> >     that you have done nothing about this to date. And yes, this means
> >     adding more the server.c file in aid of it. I believe this is only
> >     needed for the -e switch at present.
> 
>    - I'm sorry! I tried to find, if anyone have done such a check before,
>      but didn't find any. And also couldn't figure this out myself.

Mostly this is because we have avoiding extending existing protocols in
ways that would be problematic. However, the 'global-list-quiet' is a
case where the protocol was extended in a similar manner.

Other similar options include: 'Base-diff' and 'Empty-conflicts' ...
I hope this helps.
  

>      Now it's done, thanks to you.

Okay. I regret that you did not ask for clarification when you did not
understand something I wrote previously.

Generally, it will be something like

  REQ_LINE("passwd-e", serve_noop, RQ_ROOTLESS),

in server.c

If your encryption code needed to have a handshake of some kind, then
you might also need to add an RSP_LINE() entry to client.c, however, it
looks like your code does not need to worry about that at present.

> >   * spasswd ()
> >     You need to return some kind of an error if you are not processing
> >     the '-D domain' request passed by a CVS/CVSNT client.
> 
>     Okay, done!
> 
> >   * init_pass () and exit_pass ()
> >     You really should check the return value from fclose (pass)
> >     and report problems that may have arisen due to filesystem troubles
> >     reading or writing the file.
> >   ... I ran out of time to continue the review with exit_pass ()
> >   ... I'll have to find time later to do more.
> 
>     done!

More code review follows here:

      * generate_salt()
        I see you are still using srandom() and random(). You do know
        that this is not a very good random number source right? It is
        also not available on all platforms that CVS uses.

        You may find this of interest:
        http://www.delorie.com/djgpp/v2faq/faq22_23.html

        Generally, you may find that more of your code needs to be
        conditional on the existence of srandom/random in the host
        environment.

      * alias()
        Generally, "user '%s' not found" should be "user `%s' not found"
        that is, use "`" (0x60 grave accent) for the opening quote and "'"
        (0x27 single quote) as the closing quote.

      * gettok
        You are using malloc() and are not checking that it did not
        return NULL. This is a bug. Use xmalloc() if you do not want
        to worry about this problem. I also wonder that the *ret is
        not being free()'d if it is a non-NULL value... That said,
        I believe this code:

            *ret = malloc (j + 1);
            strcpy (*ret, buf);

        should be replaced with

            *ret = xstrdup (buf);

        unless I do not understand what the code is attempting to do...

      * read_pass () & write_pass () & insert ()
        Please advise why these functions always returns 0. If there is
        never a non-zero return, why not just make them void functions?

> 
> > - sanity.sh
> >   * In many of your tests, you have the cd $testdir and other lines
> >     improperly indented. I believe I also fixed those problems in the
> >     last pastch I sent you. You seem to have ignored my patch back to
> >     you. I do not understand why you are refusing help to get this right.
> 
>    - No, I'm not refusing! It's the same 'tab' problem, I guess! I'll
>      double check this as well.
> 
> >   * '"${DOTSTAR} LOVE YOU' is wrong. It should be '"${DOTSTAR}I LOVE YOU'
> >     in all of those cases.
> 
>    - Okay! But, you know, at some other places, it is '${DOTSTAR}'
>      only.

Yeah, those are probably bugs that should be fixed. Feel free to point
them out for us if/when you find such problems. The intent is to improve
the quality of the code and the testing over time.

> >   * The hint about writeprox-0 was that you should FIX that test. It is
> >     having problems because you have added "passwd" to the REQ_LINE in
> >     server.c and have not properly updated the sanity.sh tests which
> >     are impacted by your change.
> 
>     hmmn, okay! I'll do that.
> 
> > - server.c
> >   * You are still missing the extra REQ_LINE to indicate that you
> >     support encrypted passwords via the -e switch to the passwd
> >     command. How many times do I need to mention this is a problem?
> 
>     Okay, done! I've named the request as 'passwd-e', is that fine?

Sure, that works.

        -- Mark

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.4 (FreeBSD)

iD8DBQFFL2GICg7APGsDnFERAtHhAJ9R5pTUD+lfsPFwzR4M3jG8L6VJdACgyfaE
ce3P0hVL24lZXEWjsO8nMfo=
=52Wm
-----END PGP SIGNATURE-----