[PATCH] svn_load_dirs.pl: hide passwords printed to screen
Hello,
I often use svn_load_dirs.pl in a script to load several huge third
party libraries into our SVN repo. This repository uses LDAP
authentication with https and I do not want my password popping up at
times on the console executing the script (for several hours).
Hence my second patch, that hides the password printed to screen with
stars (*). It does that by passing the array of arguments containing
the password to a function sanitize_pwd before printing it. This
function searches for '--password' and hides the following word.
I digress a bit, but my scripts using svn_load_dirs.pl (themselves in
a SVN repo) ask for username/password so that they do not expose
sensitive information. Password is prompted either with `read -s` for
the bash script, or with this SO answer for the batch version:
http://stackoverflow.com/a/20343074/3628160
Please find my patch below. Besides defining sanitize_pwd and changing
the print call sites the attached version of the patch also replaces
the few tabs in source by spaces (as I realized gmail edits the tabs I
omitted this part from the version below, which apart from that
fulfils its duty).
Best regards,
Geoffrey
--- contrib/client-side/svn_load_dirs/svn_load_dirs.pl.in
+++ contrib/client-side/svn_load_dirs/svn_load_dirs.pl.in
@@ -1499,6 +1499,18 @@ sub file_info
return '?';
}
+# Copy arguments and replace what follows --password with '*'s.
+sub sanitize_pwd
+{
+ my @str = @_ ;
+ my $hide_next = 0 ;
+ foreach(@str) {
+$_ = '*' x length if ( $hide_next ) ;
+$hide_next = ($_ eq '--password') ;
+ }
+ @str
+}
+
# Start a child process safely without using /bin/sh.
sub safe_read_from_pipe
{
@@ -1510,7 +1522,7 @@ sub safe_read_from_pipe
my $openfork_available = "MSWin32" ne $OSNAME;
if ($openfork_available)
{
- print "Running @_\n";
+ print join(' ', &sanitize_pwd("Running", @_, "\n") );
my $pid = open(SAFE_READ, "-|");
unless (defined $pid)
{
@@ -1522,7 +1534,9 @@ sub safe_read_from_pipe
open(STDERR, ">&STDOUT")
or die "$0: cannot dup STDOUT: $!\n";
exec(@_)
-or die "$0: cannot exec '@_': $!\n";
+or die "$0: cannot exec '"
+ . join(' ', &sanitize_pwd(@_) )
+ . "': $!\n";
}
}
else
@@ -1559,7 +1573,7 @@ sub safe_read_from_pipe
}
}
- print "Running @commandline\n";
+ print join(' ', &sanitize_pwd("Running", @commandline, "\n") );
if ( $comment ) { print $comment; }
# Now do the pipe.
@@ -1581,7 +1595,9 @@ sub safe_read_from_pipe
my $cd = $result & 128 ? "with core dump" : "";
if ($signal or $cd)
{
- warn "$0: pipe from '@_' failed $cd: exit=$exit signal=$signal\n";
+ warn "$0: pipe from '"
+. join(' ', &sanitize_pwd(@_) )
+. "' failed $cd: exit=$exit signal=$signal\n";
}
if (wantarray)
{
@@ -1604,8 +1620,9 @@ sub read_from_process
my ($status, @output) = &safe_read_from_pipe(@_);
if ($status)
{
- print STDERR "$0: @_ failed with this output:\n", join("\n", @output),
- "\n";
+ print STDERR
+join(' ', &sanitize_pwd("$0:", @_, "failed with this output:\n") ),
+join("\n", @output), "\n";
unless ($opt_no_user_input)
{
print STDERR
svn_load_dirs.pl-secfeat-hidepwd.patch
Description: Binary data
[PATCH] svn_load_dirs.pl: fix rm temporary folder
Hello,
I am contacting you to propose 2 patch files for the script
svn_load_dirs.pl located at the URL below:
http://svn.apache.org/repos/asf/subversion/trunk/contrib/client-side/svn_load_dirs/svn_load_dirs.pl.in
This email discusses the first patch, which avoids failing to delete
the temporary folder with an error like this one:
"cannot remove path when cwd is
/tmp/svn_load_dirs_ATrGGCJoWv/my_import_wc for
/tmp/svn_load_dirs_ATrGGCJoWv"
The error has also been reported at the following link:
http://www.wandisco.com/svnforum/threads/40434-Import-snapshot-folders-requesting-step-by-step-guide-for-using-svn_load_dirs-pl?p=116340&viewfull=1#post116340
I propose a really simple patch, as pasted below (I also send it
attached if you prefer).
Best regards,
Geoffrey
--- contrib/client-side/svn_load_dirs/svn_load_dirs.pl.in
+++ contrib/client-side/svn_load_dirs/svn_load_dirs.pl.in
@@ -2052,6 +2052,7 @@
sub DESTROY
{
print "Cleaning up $temp_dir\n";
+ chdir( $temp_dir . "/.." );
File::Path::rmtree([$temp_dir], 0, 0);
}
svn_load_dirs.pl-bugfix-rmtmp.patch
Description: Binary data
Re: [PATCH] svn_load_dirs.pl: do not print password to screen
Hi Daniel, > > CC: both of the most recent and biggest contributors to this file. > > In principle, same answer as in the other thread (ENOTIME unless it's a > regression I signed off on); but… Ok noted. Thank you for your replies. > > It implements a security feature: to hide the password when printing > > the command line to screen. > > I suggest to add a warning to usage() that passing the password in > a command-line argument may make it visible to other local OS users. Do you mean that showing a warning message would be preferable to actually fixing the problem? If yes, why would that be? Note that being aware of the problem is not enough to prevent (for example) co-workers looking at your screen when you’re checking if the command has completed. Best regards, Geoffrey
Re: [PATCH] svn_load_dirs.pl: do not print password to screen
Hi Brane, Ok I understand, thank you. Best regards, Geoffrey Le dim. 14 juil. 2019 à 19:52, Branko Čibej a écrit : > > On 14.07.2019 09:47, Geoffrey Alary wrote: > > Hi Daniel, > > > >>> CC: both of the most recent and biggest contributors to this file. > >> In principle, same answer as in the other thread (ENOTIME unless it's a > >> regression I signed off on); but… > > Ok noted. Thank you for your replies. > > > >>> It implements a security feature: to hide the password when printing > >>> the command line to screen. > >> I suggest to add a warning to usage() that passing the password in > >> a command-line argument may make it visible to other local OS users. > > Do you mean that showing a warning message would be preferable to > > actually fixing the problem? If yes, why would that be? > > On Unix, the 'ps' command can show the entire command line to other > users, so this consideration is independent of what svn_load_dirs.pl is > doing. > > -- Brane
[PATCH] svn_load_dirs.pl: do not print password to screen (v2)
Hi,
> > > > It implements a security feature: to hide the password when printing
> > > > the command line to screen.
> > >
> > > I suggest to add a warning to usage() that passing the password in
> > > a command-line argument may make it visible to other local OS users.
> >
> > Do you mean that showing a warning message would be preferable to
> > actually fixing the problem?
>
> No. I think the warning and the asterisking are independent changes;
> I suppose we should make both of them.
This is the second version of the patch, including the suggestion from
Daniel.
Log message:
[[[
Do not print password to screen in svn_load_dirs.pl.
* contrib/client-side/svn_load_dirs/svn_load_dirs.pl.in
(sanitize_pwd): New function.
(safe_read_from_pipe, read_from_process): Update the sites printing
the command line to screen to use sanitize_pwd.
(usage): Warn that other local OS users may be able to see the
password passed on the command-line.
Fix indentation; that is, replace the 2 tab occurrences by 8 spaces.
]]]
Index: contrib/client-side/svn_load_dirs/svn_load_dirs.pl.in
===
--- contrib/client-side/svn_load_dirs/svn_load_dirs.pl.in (revision 1863003)
+++ contrib/client-side/svn_load_dirs/svn_load_dirs.pl.in (working copy)
@@ -196,7 +196,7 @@
{
if ( /^global-ignores\s*=\s*(.*?)\s*$/ )
{
- $ignores_str = $1;
+$ignores_str = $1;
last;
}
}
@@ -1344,6 +1344,8 @@
" -p filenametable listing properties to apply to matching files\n",
" -svn_username username to perform commits as\n",
" -svn_password password to supply to svn commit\n",
+ " WARNING: passing the password in a command-line argument\n",
+ " may make it visible to other local OS users\n",
" -t tag_dir create a tag copy in tag_dir, relative to svn_url\n",
" -v increase program verbosity, multiple -v's allowed\n",
" -wc path use the already checked-out working copy at path\n",
@@ -1500,6 +1502,18 @@
return '?';
}
+# Copy arguments and replace what follows --password with '*'s.
+sub sanitize_pwd
+{
+ my @str = @_;
+ my $hide_next = 0;
+ foreach(@str) {
+$_ = '*' x length if ( $hide_next );
+$hide_next = ($_ eq '--password');
+ }
+ @str;
+}
+
# Start a child process safely without using /bin/sh.
sub safe_read_from_pipe
{
@@ -1511,7 +1525,7 @@
my $openfork_available = "MSWin32" ne $OSNAME;
if ($openfork_available)
{
- print "Running @_\n";
+ print join(' ', &sanitize_pwd("Running", @_, "\n") );
my $pid = open(SAFE_READ, "-|");
unless (defined $pid)
{
@@ -1523,7 +1537,9 @@
open(STDERR, ">&STDOUT")
or die "$0: cannot dup STDOUT: $!\n";
exec(@_)
-or die "$0: cannot exec '@_': $!\n";
+or die "$0: cannot exec '"
+ . join(' ', &sanitize_pwd(@_) )
+ . "': $!\n";
}
}
else
@@ -1560,7 +1576,7 @@
}
}
- print "Running @commandline\n";
+ print join(' ', &sanitize_pwd("Running", @commandline, "\n") );
if ( $comment ) { print $comment; }
# Now do the pipe.
@@ -1582,7 +1598,9 @@
my $cd = $result & 128 ? "with core dump" : "";
if ($signal or $cd)
{
- warn "$0: pipe from '@_' failed $cd: exit=$exit signal=$signal\n";
+ warn "$0: pipe from '"
+. join(' ', &sanitize_pwd(@_) )
+. "' failed $cd: exit=$exit signal=$signal\n";
}
if (wantarray)
{
@@ -1605,8 +1623,9 @@
my ($status, @output) = &safe_read_from_pipe(@_);
if ($status)
{
- print STDERR "$0: @_ failed with this output:\n", join("\n", @output),
- "\n";
+ print STDERR
+join(' ', &sanitize_pwd("$0:", @_, "failed with this output:\n") ),
+join("\n", @output), "\n";
unless ($opt_no_user_input)
{
print STDERR
@@ -1658,7 +1677,7 @@
};
find({no_chdir => 1,
preprocess => sub
- {
+ {
grep
{
my $ok=1;
[PATCH] svn_load_dirs.pl: fix broken cleanup
Hi,
When svn_load_dirs.pl is run without the -t option, cleanup on exit
fails with the following error:
"cannot remove path when cwd is
/tmp/svn_load_dirs_ATrGGCJoWv/my_import_wc for
/tmp/svn_load_dirs_ATrGGCJoWv"
The only mention of the bug I have found was at the bottom of this forum post:
https://www.svnforum.org/forum/ubersvn-community/ubersvn-help-and-support/9843-import-snapshot-folders-requesting-step-by-step-guide-for-using-svn_load_dirs-pl?p=49562#post49562
The fix is very simple: change the working directory to the parent of
the directory to remove. Thus we ensure that the cwd is not inside the
path we attempt to remove. This change has no other side effect
because this code only runs just before exiting the program.
Log message:
[[[
Fix broken cleanup in svn_load_dirs.pl.
* contrib/client-side/svn_load_dirs/svn_load_dirs.pl.in (DESTROY):
Change cwd to the parent of the directory to remove.
]]]
Index: contrib/client-side/svn_load_dirs/svn_load_dirs.pl.in
===
--- contrib/client-side/svn_load_dirs/svn_load_dirs.pl.in
+++ contrib/client-side/svn_load_dirs/svn_load_dirs.pl.in
@@ -2073,5 +2073,6 @@
sub DESTROY
{
print "Cleaning up $temp_dir\n";
+ chdir( $temp_dir . "/.." );
File::Path::rmtree([$temp_dir], 0, 0);
}
