Re: history and val-tags locks.

[Mark D. Baushke]

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

Derek Price <derek@ximbiot.com> writes:

> Derek Price wrote:
> 
> >Derek Price wrote:
> >
> >>I see your point. What about `cvs server'? I
> >>can see both setups being useful... an admin
> >>who allowed users access to the CVS repository
> >>would probably prefer not to allow the config
> >>file to be specified whereas an admin who
> >>restriced the command that SSH users could run
> >>to a particular shell script that provided the
> >>-c option wouldn't mind... perhaps it should
> >>be a compile time option, with the default to
> >>disallow it.
> >
> >
> >On further consideration, if we are going to
> >consider a configurable config path with other
> >CVS modes a security risk, then using it with
> >pserver has to be considered a security risk
> >too. There is nothing stopping a creative user
> >with shell access to a machine from using
> >pserver mode to access their repository.
> >
> >I might argue that any administrator worried
> >that much about security should be disabling
> >shell access to the machine anyhow, which would
> >deal with any insecurity resulting from a
> >configurable config path, but I don't feel so
> >strongly about it that I wouldn't happily
> >install it as a compile-time option that
> >defaults to off.
> 
> 
> I've implemented this as an option to server &
> pserver. Installing as a global option would
> have create problems in multiroot mode anyhow.

True.

> Preliminary patch against 1.11.x attached. The
> final version will go into feature - I'm not
> advocating putting this in stable, but this is
> what I have now and I thought I would request a
> review. 

okay.

> This patch also finally disables the
> sourcing of the ~/.cvsrc file for the server
> commands as an added protection against a user
> setting the path to the config file.
> 
> 2005-05-17  Derek Price  <derek@ximbiot.com>
> 
>     * configure.in: Add --enable-config-override.
>     * main.c (main): Don't source .cvsrc in server mode.
>     Remove obsolete comment.
>     * parseinfo.c (ConfigPath): New global.
>     (parse_config): Open ConfigPath when provided.
>     * server.c (server): Parse -c option.
>     * sanity.sh (server_usage): New static global.
>     (sever): Add tests of ConfigPath and .cvsrc.
>   
> 
> I've been thinking about this more, and I'm
> starting to feel that as an option to
> server/pserver/etc, this really isn't so
> insecure. In general, an admin will be able to
> and probably does restrict the arguments to the
> server & pserver commands, and a user with shell
> access to the server could run a hacked CVS
> against a repo or even alter a repo directly
> anyhow, so the argument about security is mostly
> moot.

For pserver, the administrator has full control over
the command line.

For server, if the administrator is using a
restricted shell for users, they may or may not be
able to restrict command-line arguments (it
depends on how the restricted shell is
implemented).

> The only exception would be where the admin only
> used a setuid CVS executable to restrict repo
> access to a specific CVS executable. I'm not
> sure how common this is however, as it also
> disables the ability to use UNIX uids & gids for
> finer control over read & write access.

I have used (am using) a set-gid cvs and it does
not disable the use of UNIX uids at all. Yes, it
does inhibit gids as the entire repository uses
the same gid ('cvs'), but the cvs_acls deals with
permissions for commit and anyone with a login to
the special-purpose CVS server has already been
granted read permissions for checkout in any case.

I will grant you that this is not necessarily a
normal situation, but I make note of it as setuid
is not the only configuration possiblity here.

For the patch, if you want the
- --enable-config-override to be a configure and
compile-time option, I don't have any strong
objections to it as long as it defaults to
the more paranoid configuration. So, the current
enable_config_override=yes is not one that I
believe should be the default.

As an alternative, I would not have a problem with
allowing cvs to be built with a list of
directories that could be searched for a valid
config file. So, a --enable-config-prefix=/etc/cvs
might mean that /etc/cvs/$CVSROOT/config would looked
at before $CVSROOT/config ... doing that means that
the administrator would still have complete control
over the configurations and would not need to rely
on the user to make the right choice.

The downside is that src/sanity.sh tests for this
case would be more painful.

Your change to src/main.c looks fine.

While I would favor another mechanism for config
override, the code you have written for
src/parseinfo.c does appear to work as you suggest

As far as your src/sanity.sh tests, I believe you
should use the -c CONFIG-FILE to determine if cvs
has been configured with this option or not before
you fail the tests (granted STABLE does not have
that available to sanity.sh)

As far as src/server.c, the code looks reasonable
for what you are attempting.

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

iD8DBQFCii4p3x41pRYZE/gRAt0tAJ9zzEWfVpWUuRA6qn6WKkoqzOXTkwCffpoI
q5BKSwEnOKwbXHN2SPtjblc=
=Bmf5
-----END PGP SIGNATURE-----