Thanks for the code review!
On 02/13/2008 10:54 PM, David Glasser wrote:
> Big security hole: you take the "username" parameter directly from
> user input and interpolate it into a regexp. Don't do that :-)
>
Right you are. Fixed.
> Also, you probably want to update the file atomically (with a temp
> file and a rename). Wouldn't want svnserve to read a half-written
> file...
>
I had resisted doing this because I didn't want to require that the
password file be in its own directory that's writeable by apache, but I
suppose you're right that it's necessary, so fixed.
> And looks like if the user doesn't pass in a "repo" parameter,
> $passwd_file isn't defined... exciting times.
Not really exciting times, just an internal error, but I've now made
this more explicit.
> Not to mention the
> errors that happen if various parameters are the string "0" (though
> admittedly that's a rather poor username or password).
>
Wow, you're paranoid :-). Fixed, I believe.
I also added a few additional test cases and made the test suite work
again (some last-minute changes I made before posting the script broke
the tests).
I also added "-T" to the #! line to enable taint checks, for a slightly
higher level of paranoia.
A new version is attached, along with a diff. Please take a look.
Is there any interest in shipping this script with the distribution? If
there is, I'd be happy to commit to maintaining it.
jik
#!/usr/bin/perl -T
=head1 NAME
svn-passwd.cgi - CGI script for self-service Subversion password changes
=head1 SYNOPSIS
svn-passwd.cgi [--repo=name=passwd-file-path [...]] [--norequire-ssl]
=head1 DESCRIPTION
The svn-passwd.cgi CGI script provides a simple Web interface for
users to change their password in Subversion password files.
The Subversion password files administered by this script are the ones
used by Subversion's built-in authentication, i.e., the authentication
mechanism used when your users connect to svnserve directly, e.g.,
through xinetd, rather than through http, https or SSH.
If you are using Subversion's built-in authentication, your Subversion
repository should have a password file specified in the "password-db"
setting of the Subversion configuration file. If it doesn't, then you
probably aren't using Subversions' built-in authentication, and you
therefore probably don't need this script.
=head1 INSTALLATION
To use this script:
=over
=item 1. Enable httpd on your Subversion server.
=item 2. (Recommended but not required.) Enable SSL in your httpd.
=item 3. Save this script in a directory of its own.
=item 4. Make your password files writeable by httpd.
=item 5. Configure the script.
=item 6. Modify httpd to be able to execute the script.
=back
The following sections explain these steps in more detail.
=head2 Enable httpd on your Subversion server.
Installing and enabling httpd is beyond the scope of this
documentation. Consult the documentation for your Linux distribution.
However...
On a system running Red Hat Linux of some sort, you probably need to
install the "httpd" package, run "/sbin/chkconfig httpd on", and run
"/sbin/service httpd start".
=head2 Enable SSL in your httpd.
SSL is recommended so that passwords don't travel over the network in
plaintext. Of course, it's entirely possible that Subversion itself
sends the password in plaintext when logging the user in, but there's
no reason to make the problem any worse.
Installing and configuring SSL in your httpd are beyond the scope of
this document. Consult the documentation for your Linux distribution.
However...
On a system running Red Hat Linux of some sort, you probably need to
install the "mod_ssl" package and run "/sbin/service httpd restart".
This should be sufficient to enable SSL on your server with a
self-signed SSL certificate.
=head2 Save this script in a directory of its own.
Create a new directory (e.g., underneath "/var/www", perhaps) in which
to save this script, and save it there. Make sure that the directory
and script are readable and executable by the httpd process (probably
by making them owned by user or group "apache" and making them owner
or group readable and executable).
=head2 Make your password files writeable by httpd.
For users to be able to change their passwords using this script, the
script needs to be able to write to the password files and to the
directories in which they reside (because it creates a temporary file
in the directory so that the password file can be updated
atomically).
To grant this access to the script without compromising security, you
should put your password file in a directory of its own and make sure
that both the directory and the password file are owned by group
apache and group-writeable.
Make the dedicated directory and file to be owned by the user under
which Subversion runs, so that Subversion will be able to read them.
Remember to update your Subversion configuration file to point at the
correct new location for the password file.
=head2 Configure the script.
Now things start to get interesting. To configure the script, you
need to tell it the names and password file locations of each
repository and whether you want the script to enforce SSL (i.e.,
reject attempts to use it through a non-SSL connection). You can do
this either by editing the script itself to add the appropriate
configuration settings, or by writing a wrapper script which calls
this script with the appropriate configuration command-line options.
If you want to edit the script:
=over
=item 1. Search for "$require_ssl = 1" below and change the "1" to "0"
if you don't want the script to enforce SSL.
=item 2. Search for "Repo1" below and replace the two sample
repository configurations there with your own repositories. For each
repository whose passwords you want this script to administer, you
should have a line that looks like this:
C<$repos{"I<name>"} = "I</password/file/path>";
=back
If you would rather write a wrapper script, then create a shell script
in the same directory as this script which calls "exec
I<directory>/svn-passwd.cgi" with the appropriate arguments:
=over
=item 1. Specify "--norequire-ssl" if you don't want the script to
enforce SSL.
=item 2. Specify "--repo=I<name>=I</password/file/path>" for each
repository whose passwords you want the script to administer.
=back
=head Modify httpd to be able to execute the script.
Add something like the following to your httpd configuration:
ScriptAlias /svn-passwd/ "/var/www/svn-passwd/"
<Directory "/var/www/svn-passwd">
RewriteEngine on
RewriteRule ^$ /svn-passwd/svn-passwd.cgi
</Directory>
In this example, the script was saved as svn-passwd.cgi in the
directory /var/www/svn-passwd, and it can be invoked as
<http://I<hostname>/svn-passwd/>. Then reload or restart your httpd
and give it a try!
=head1 AUTHOR
Jonathan Kamens <Jonathan.Kamens_at_tamalesoftware.com>
=head1 COPYRIGHT
Copyright (C) 2008 Tamale Software, Inc.
This program is free software: you can redistribute it and/or modify
it under the terms of the GNU General Public License as published by
the Free Software Foundation, either version 3 of the License, or
(at your option) any later version.
This program is distributed in the hope that it will be useful, but
WITHOUT ANY WARRANTY; without even the implied warranty of
MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
General Public License for more details.
The GNU General Public License may be viewed at
L<http://www.gnu.org/licenses/>.
=cut
use strict;
use warnings;
use CGI qw/:standard *center *table https/;
use CGI::Carp qw(fatalsToBrowser);
use Fcntl qw/:flock :seek/;
use Getopt::Long;
use IO::File;
use Sys::Syslog;
my %repos;
my $require_ssl = 1;
die if (! GetOptions("repo=s%" => \%repos,
"require-ssl!" => \$require_ssl));
if (! %repos) {
$repos{"Repo1"} = "/usr/local/testrepo/conf/passwd/passwd";
$repos{"Repo2"} = "/usr/local/testrepo2/conf/passwd/passwd";
}
if ($require_ssl && ! https()) {
my $url = url();
print header();
print start_html("SSL Required");
print h1("SSL Required");
print p("This Web application can only be accessed via SSL.");
if ($url =~ s/^http:/https:/) {
print p("Please try " . a({"href" => $url}, $url) . " instead.");
}
print end_html();
exit;
}
my $repo = param("repo");
my $user = param("username");
my $old_password = param("oldpassword");
my $new_password = param("newpassword");
my $confirm_password = param("confirmpassword");
my $passwd_file;
if ($repo) {
if (! ($passwd_file = $repos{$repo})) {
die "Invalid repo: $repo\n";
}
}
my($success, $error, $fh);
if (defined $user) {
if (defined $old_password) {
if (defined $new_password) {
if (defined $confirm_password) {
$success = &doit($user, $old_password, $new_password,
$confirm_password);
}
else {
$error = "Please specify your new password twice.";
}
}
else {
$error = "Please specify your new password.";
}
}
else {
$error = "Please specify your old password.";
}
}
elsif (defined $old_password || defined $new_password ||
defined $confirm_password) {
$error = "Please specify your Subversion username.";
}
elsif (param("Change password")) {
$error = "Please fill out the form completely.";
}
print header();
print start_html("Subversion password changer");
print h1("Subversion password changer");
if ($success) {
print p("Password for \"$user\" updated successfully.");
print p(a({"href" => url()}, "Click here") . " to change another password.");
print end_html();
exit;
}
print start_form();
if (keys %repos == 1) {
my($repo_name) = keys %repos;
print p("Use this form to change your password for the \"$repo_name\" Subversion repository.");
print hidden("repo", $repo_name);
}
else {
print p("Use this form to change your password for a Subversion repository.");
}
if ($error) {
print p(font({"color" => "red"}, $error));
}
print start_center();
print start_table({"border" => 1});
if (keys %repos > 1) {
print Tr(td({"align" => "left"}, b("Repository:")),
td({"align" => "left"}, popup_menu("repo", [sort keys %repos])));
}
print Tr(td({"align" => "left"}, b("Username:")),
td({"align" => "left"}, textfield("username")));
print Tr(td({"align" => "left"}, b("Old password:")),
td({"align" => "left"}, password_field("-name" => "oldpassword",
"-value" => "",
"-override" => 1)));
print Tr(td({"align" => "left"}, b("New password:")),
td({"align" => "left"}, password_field("-name" => "newpassword",
"-value" => "",
"-override" => 1)));
print Tr(td({"align" => "left"}, b("Confirm new password:")),
td({"align" => "left"}, password_field("-name" => "confirmpassword",
"-value" => "",
"-override" => 1)));
print Tr(td({"colspan" => 2}, reset() . " " . submit("Change password")));
print end_table();
print end_center();
print end_form();
print end_html();
sub doit {
my($username, $old_password, $new_password, $confirm_password) = @_;
if (! $repo) {
$error = "Internal error: No repository defined\n";
&log(1, "\&doit called with \$repo unset");
return undef;
}
if (! $passwd_file) {
$error = "Internal error: Missing repository configuration\n";
&log(1, "\&doit called with \$repo=\"$repo\" and \$passwd_file unset");
return undef;
}
if ($new_password ne $confirm_password) {
$error = "New passwords do not match.";
return undef;
}
if ($new_password =~ /\s/) {
$error = "New password may not contain whitespace.";
return undef;
}
elsif ($new_password =~ /[^[:print:]]/) {
$error = "New password may not contain non-printing characters.";
return undef;
}
&check_password($username, $old_password) or return undef;
&lock_password_file or return undef;
&update_password_file($username, $old_password, $new_password) or
return undef;
return $username;
}
sub check_password {
my($username, $old_password) = @_;
open(PASSWD, "<", $passwd_file) or die "open($passwd_file): $!\n";
while (<PASSWD>) {
next if (! /^\s*([^\s\#]\S*)\s*=\s*(\S+)\s*$/);
next if ($1 ne $username);
if ($old_password ne $2) {
&log(1, "$username: Password mismatch");
# Message is ambiguous to prevent username guessing attacks.
$error = "Username or password incorrect.";
return undef;
}
close(PASSWD);
return 1;
}
close(PASSWD);
# Message is ambiguous to prevent username guessing attacks.
# Don't log username because user may have accidentally specified password
# as username.
&log(1, "Invalid user");
$error = "Username or password incorrect.";
return undef;
}
sub lock_password_file {
$fh = new IO::File or die;
$fh->open("+< $passwd_file") or die "open($passwd_file): $!\n";
flock($fh, LOCK_EX) or die "flock($passwd_file): $!\n";
return 1;
}
sub update_password_file {
my($username, $old_password, $new_password) = @_;
my(@lines);
$fh->seek(0, SEEK_SET) or die "seek($passwd_file): $!\n";
my $found = undef;
while (<$fh>) {
if (/^(\s*)([^\s\#]\S*)(\s*=\s*)(\S+)(\s*)$/ && $username eq $2) {
if ($old_password ne $4) {
$error = "Password file was modified unexpectedly. Please try again.";
return undef;
}
$_ = $1 . $2 . $3 . $new_password . $5;
$found = 1;
}
push(@lines, $_);
last if ($found);
}
if (! $found) {
die "Internal error: could not find $username in password file.\n";
}
push(@lines, <$fh>);
if ($fh->error) {
die "Error reading from $passwd_file: $!\n";
}
open(NEWPASSWD, ">", "$passwd_file.$$") or
die "open(>$passwd_file.$$): $!\n";
print(NEWPASSWD @lines) or die "print(>$passwd_file.$$): $!\n";
close(NEWPASSWD) or die "close(>$passwd_file.$$): $!\n";
rename("$passwd_file.$$", $passwd_file) or
die "rename($passwd_file.$$, $passwd_file): $!\n";
$fh->close or die "close($passwd_file): $!\n";
&log(0, "$username: Password successfully changed");
return 1;
}
sub log {
my($error, $msg) = @_;
openlog("svn-passwd-cgi", "", "LOG_AUTHPRIV");
syslog($error ? "LOG_ERR" : "LOG_NOTICE", "request from %s: %s",
remote_host(), $msg);
}
__END__
#!/bin/sh -ex
# Test script
PF=/tmp/svn-passwd-test
cat > $PF <<EOF
# comment here
[users]
freeble = toodle
foo = bar
tuber = frobnicator
# comment here
EOF
F="--repo=Repo=$PF --norequire-ssl repo=Repo"
# SSL is required
./svn-passwd.cgi ${F/--norequire-ssl/} | grep "This Web application can only be accessed via SSL"
# No username specified
./svn-passwd.cgi $F oldpassword=foo | grep -q -s "Please specify your Subversion username"
./svn-passwd.cgi $F newpassword=foo | grep -q -s "Please specify your Subversion username"
./svn-passwd.cgi $F confirmpassword=baz | grep -q -s "Please specify your Subversion username"
# No old password specified
./svn-passwd.cgi $F username=foo newpassword=baz confirmpassword=baz | grep -q -s "Please specify your old password"
# No new password specified
./svn-passwd.cgi $F username=foo oldpassword=bar | grep -q -s "Please specify your new password"
./svn-passwd.cgi $F username=foo oldpassword=bar confirmpassword=baz | grep -q -s "Please specify your new password"
# No confirmation password specified
./svn-passwd.cgi $F username=foo oldpassword=bar newpassword=baz | grep -q -s "Please specify your new password twice"
# Password mismatch
./svn-passwd.cgi $F username=foo oldpassword=bar newpassword=baz confirmpassword=screlt | grep -q -s "New passwords do not match"
# Whitespace in password
./svn-passwd.cgi $F username=foo oldpassword=bar newpassword=" b" confirmpassword=" b" | grep -q -s "New password may not contain whitespace"
# Non-printing character in password
./svn-passwd.cgi $F username=foo oldpassword=bar newpassword="" confirmpassword="" | grep -q -s "New password may not contain non-printing characters"
# Wrong old password
./svn-passwd.cgi $F username=foo oldpassword=baz newpassword="f" confirmpassword="f" | grep -q -s "Username or password incorrect"
# Invalid username
./svn-passwd.cgi $F username=nonesuch oldpassword=bar newpassword="f" confirmpassword="f" | grep -q -s "Username or password incorrect"
# repo is unset
./svn-passwd.cgi ${F/ repo=Repo/} username=nonesuch oldpassword=bar newpassword="f" confirmpassword="f" | grep -q -s "Internal error: No repository defined"
# Correct password change
./svn-passwd.cgi $F username=foo oldpassword=bar newpassword="baz" confirmpassword="baz" | grep -q -s "Password for \"foo\" updated successfully"
diff - $PF <<EOF
# comment here
[users]
freeble = toodle
foo = baz
tuber = frobnicator
# comment here
EOF
echo "All tests succeeded."
--- /tmp/svn-passwd.cgi.orig 2008-02-14 11:34:54.000000000 -0500
+++ /tmp/svn-passwd.cgi 2008-02-14 11:31:07.000000000 -0500
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/perl -T
=head1 NAME
@@ -83,10 +83,21 @@
=head2 Make your password files writeable by httpd.
For users to be able to change their passwords using this script, the
-script needs to be able to write to the password files. To ensure
-this, it's probably sufficient to assign the "apache" group to the
-password file ("chgrp apache I<file-name>") and make sure it is
-group-readable and group-writeable ("chmod g+rw I<file-name>").
+script needs to be able to write to the password files and to the
+directories in which they reside (because it creates a temporary file
+in the directory so that the password file can be updated
+atomically).
+
+To grant this access to the script without compromising security, you
+should put your password file in a directory of its own and make sure
+that both the directory and the password file are owned by group
+apache and group-writeable.
+
+Make the dedicated directory and file to be owned by the user under
+which Subversion runs, so that Subversion will be able to read them.
+
+Remember to update your Subversion configuration file to point at the
+correct new location for the password file.
=head2 Configure the script.
@@ -217,10 +228,10 @@
my($success, $error, $fh);
-if ($user) {
- if ($old_password) {
- if ($new_password) {
- if ($confirm_password) {
+if (defined $user) {
+ if (defined $old_password) {
+ if (defined $new_password) {
+ if (defined $confirm_password) {
$success = &doit($user, $old_password, $new_password,
$confirm_password);
}
@@ -236,7 +247,8 @@
$error = "Please specify your old password.";
}
}
-elsif ($old_password || $new_password || $confirm_password) {
+elsif (defined $old_password || defined $new_password ||
+ defined $confirm_password) {
$error = "Please specify your Subversion username.";
}
elsif (param("Change password")) {
@@ -300,6 +312,18 @@
sub doit {
my($username, $old_password, $new_password, $confirm_password) = @_;
+ if (! $repo) {
+ $error = "Internal error: No repository defined\n";
+ &log(1, "\&doit called with \$repo unset");
+ return undef;
+ }
+
+ if (! $passwd_file) {
+ $error = "Internal error: Missing repository configuration\n";
+ &log(1, "\&doit called with \$repo=\"$repo\" and \$passwd_file unset");
+ return undef;
+ }
+
if ($new_password ne $confirm_password) {
$error = "New passwords do not match.";
return undef;
@@ -324,8 +348,9 @@
open(PASSWD, "<", $passwd_file) or die "open($passwd_file): $!\n";
while (<PASSWD>) {
- next if (! /^\s*$username\s*=\s*(\S+)\s*$/o);
- if ($old_password ne $1) {
+ next if (! /^\s*([^\s\#]\S*)\s*=\s*(\S+)\s*$/);
+ next if ($1 ne $username);
+ if ($old_password ne $2) {
&log(1, "$username: Password mismatch");
# Message is ambiguous to prevent username guessing attacks.
$error = "Username or password incorrect.";
@@ -357,13 +382,12 @@
$fh->seek(0, SEEK_SET) or die "seek($passwd_file): $!\n";
my $found = undef;
while (<$fh>) {
- if (/^\s*$username\s*=\s*(\S+)\s*$/o) {
- if ($old_password ne $1) {
+ if (/^(\s*)([^\s\#]\S*)(\s*=\s*)(\S+)(\s*)$/ && $username eq $2) {
+ if ($old_password ne $4) {
$error = "Password file was modified unexpectedly. Please try again.";
return undef;
}
- }
- if (s/^(\s*$username\s*=\s*)(\S+)(\s*)$/$1$new_password$3/) {
+ $_ = $1 . $2 . $3 . $new_password . $5;
$found = 1;
}
push(@lines, $_);
@@ -376,10 +400,13 @@
if ($fh->error) {
die "Error reading from $passwd_file: $!\n";
}
- $fh->seek(0, SEEK_SET) or die "seek($passwd_file): $!\n";
- $fh->print(@lines) or die "write(>$passwd_file): $!\n";
- $fh->truncate($fh->tell) or die "truncate($passwd_file): $!\n";
- $fh->close or die "close(>$passwd_file): $!\n";
+ open(NEWPASSWD, ">", "$passwd_file.$$") or
+ die "open(>$passwd_file.$$): $!\n";
+ print(NEWPASSWD @lines) or die "print(>$passwd_file.$$): $!\n";
+ close(NEWPASSWD) or die "close(>$passwd_file.$$): $!\n";
+ rename("$passwd_file.$$", $passwd_file) or
+ die "rename($passwd_file.$$, $passwd_file): $!\n";
+ $fh->close or die "close($passwd_file): $!\n";
&log(0, "$username: Password successfully changed");
return 1;
@@ -409,31 +436,35 @@
# comment here
EOF
-F="--passwd-file $PF"
+F="--repo=Repo=$PF --norequire-ssl repo=Repo"
+# SSL is required
+./svn-passwd.cgi ${F/--norequire-ssl/} | grep "This Web application can only be accessed via SSL"
# No username specified
-./passwd.cgi $F oldpassword=foo | grep -q -s "Please specify your Subversion username"
-./passwd.cgi $F newpassword=foo | grep -q -s "Please specify your Subversion username"
-./passwd.cgi $F confirmpassword=baz | grep -q -s "Please specify your Subversion username"
+./svn-passwd.cgi $F oldpassword=foo | grep -q -s "Please specify your Subversion username"
+./svn-passwd.cgi $F newpassword=foo | grep -q -s "Please specify your Subversion username"
+./svn-passwd.cgi $F confirmpassword=baz | grep -q -s "Please specify your Subversion username"
# No old password specified
-./passwd.cgi $F username=foo newpassword=baz confirmpassword=baz | grep -q -s "Please specify your old password"
+./svn-passwd.cgi $F username=foo newpassword=baz confirmpassword=baz | grep -q -s "Please specify your old password"
# No new password specified
-./passwd.cgi $F username=foo oldpassword=bar | grep -q -s "Please specify your new password"
-./passwd.cgi $F username=foo oldpassword=bar confirmpassword=baz | grep -q -s "Please specify your new password"
+./svn-passwd.cgi $F username=foo oldpassword=bar | grep -q -s "Please specify your new password"
+./svn-passwd.cgi $F username=foo oldpassword=bar confirmpassword=baz | grep -q -s "Please specify your new password"
# No confirmation password specified
-./passwd.cgi $F username=foo oldpassword=bar newpassword=baz | grep -q -s "Please specify your new password twice"
+./svn-passwd.cgi $F username=foo oldpassword=bar newpassword=baz | grep -q -s "Please specify your new password twice"
# Password mismatch
-./passwd.cgi $F username=foo oldpassword=bar newpassword=baz confirmpassword=screlt | grep -q -s "New passwords do not match"
+./svn-passwd.cgi $F username=foo oldpassword=bar newpassword=baz confirmpassword=screlt | grep -q -s "New passwords do not match"
# Whitespace in password
-./passwd.cgi $F username=foo oldpassword=bar newpassword=" b" confirmpassword=" b" | grep -q -s "New password may not contain whitespace"
+./svn-passwd.cgi $F username=foo oldpassword=bar newpassword=" b" confirmpassword=" b" | grep -q -s "New password may not contain whitespace"
# Non-printing character in password
-./passwd.cgi $F username=foo oldpassword=bar newpassword="" confirmpassword="" | grep -q -s "New password may not contain non-printing characters"
+./svn-passwd.cgi $F username=foo oldpassword=bar newpassword="" confirmpassword="" | grep -q -s "New password may not contain non-printing characters"
# Wrong old password
-./passwd.cgi $F username=foo oldpassword=baz newpassword="f" confirmpassword="f" | grep -q -s "Username or password incorrect"
+./svn-passwd.cgi $F username=foo oldpassword=baz newpassword="f" confirmpassword="f" | grep -q -s "Username or password incorrect"
# Invalid username
-./passwd.cgi $F username=nonesuch oldpassword=bar newpassword="f" confirmpassword="f" | grep -q -s "Username or password incorrect"
+./svn-passwd.cgi $F username=nonesuch oldpassword=bar newpassword="f" confirmpassword="f" | grep -q -s "Username or password incorrect"
+# repo is unset
+./svn-passwd.cgi ${F/ repo=Repo/} username=nonesuch oldpassword=bar newpassword="f" confirmpassword="f" | grep -q -s "Internal error: No repository defined"
# Correct password change
-./passwd.cgi $F username=foo oldpassword=bar newpassword="baz" confirmpassword="baz" | grep -q -s "Password for \"foo\" updated successfully"
+./svn-passwd.cgi $F username=foo oldpassword=bar newpassword="baz" confirmpassword="baz" | grep -q -s "Password for \"foo\" updated successfully"
diff - $PF <<EOF
# comment here
[users]
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-02-14 17:36:52 CET