Re: svn commit: r1863018 - /subversion/trunk/subversion/libsvn_subr/win32_crypto.c
On 14.07.2019 17:51, Evgeny Kotkov wrote: > Branko Čibej writes: > >> Uh. I don't think so. >> >> First of all, the reference to Chromium source is a red herring ... that >> code disables CRL/OCSP checks *if some caller required it to*. You'll >> find that browsers do, indeed, check CRL or OCSP info, if it's available. > I went through an additional round of fact-checking to ensure that Chromium > browsers have never been doing online revocation checks by default. > > Back in 2014, this could be controlled by a user preference (disabled by > default), but since then the UI option was removed and the current state > is that such checks only operate from cache: > > > https://codereview.chromium.org/250583004/diff/20001/chrome/browser/net/ssl_config_service_manager_pref.cc > > https://chromium.googlesource.com/chromium/src/+/refs/tags/77.0.3852.2/chrome/browser/ssl/ssl_config_service_manager_pref.cc#243 > > On Windows, this can be additionally verified by examining the CryptoAPI > log where I can see the certificate chains being constructed with the > CERT_CHAIN_REVOCATION_CHECK_CACHE_ONLY flag. > >> Your change disables all online verification, regardless, and that seems >> quite wrong. > This change affects the Win32 callback used to *override specific* certificate > validation failures, namely SVN_AUTH_SSL_UNKNOWNCA. So in the current > design, the revocation checks are only supplemental, but not authoritative. > As an example, if the Serf/OpenSSL layer thinks that the certificate is OK > by itself (without performing a revocation check), this callback is not going > to be called at all. > > Since all infrastructure-related failures, stalls and timeouts during online > revocation checks appear to be fairly common, combined, this means that we > are putting the users through all these potential problems, but in the end > the whole checking is still non-exhaustive. Perhaps, the actual result of > that is just adding to the perception that Subversion is slow in general. > > With that in mind, I tend to think that it would be much better to just stick > to the locally available data for this particular callback, that is currently > used to only resolve a subset of certificate validation failures. > >> So ... -1, please revert > Will certainly do, unless this new data alters your opinion on the topic. Hmm ... I wasn't aware that this check wasn't always performed. That changes the impact of you change considerably. Please leave it in for now ... but let's have that discussion anyway. -- Brane
Re: svn commit: r1863018 - /subversion/trunk/subversion/libsvn_subr/win32_crypto.c
Branko Čibej writes: > Uh. I don't think so. > > First of all, the reference to Chromium source is a red herring ... that > code disables CRL/OCSP checks *if some caller required it to*. You'll > find that browsers do, indeed, check CRL or OCSP info, if it's available. I went through an additional round of fact-checking to ensure that Chromium browsers have never been doing online revocation checks by default. Back in 2014, this could be controlled by a user preference (disabled by default), but since then the UI option was removed and the current state is that such checks only operate from cache: https://codereview.chromium.org/250583004/diff/20001/chrome/browser/net/ssl_config_service_manager_pref.cc https://chromium.googlesource.com/chromium/src/+/refs/tags/77.0.3852.2/chrome/browser/ssl/ssl_config_service_manager_pref.cc#243 On Windows, this can be additionally verified by examining the CryptoAPI log where I can see the certificate chains being constructed with the CERT_CHAIN_REVOCATION_CHECK_CACHE_ONLY flag. > Your change disables all online verification, regardless, and that seems > quite wrong. This change affects the Win32 callback used to *override specific* certificate validation failures, namely SVN_AUTH_SSL_UNKNOWNCA. So in the current design, the revocation checks are only supplemental, but not authoritative. As an example, if the Serf/OpenSSL layer thinks that the certificate is OK by itself (without performing a revocation check), this callback is not going to be called at all. Since all infrastructure-related failures, stalls and timeouts during online revocation checks appear to be fairly common, combined, this means that we are putting the users through all these potential problems, but in the end the whole checking is still non-exhaustive. Perhaps, the actual result of that is just adding to the perception that Subversion is slow in general. With that in mind, I tend to think that it would be much better to just stick to the locally available data for this particular callback, that is currently used to only resolve a subset of certificate validation failures. > So ... -1, please revert Will certainly do, unless this new data alters your opinion on the topic. Thanks, Evgeny Kotkov
[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(' ', _pwd("Running", @_, "\n") );
my $pid = open(SAFE_READ, "-|");
unless (defined $pid)
{
@@ -1523,7 +1537,9 @@
open(STDERR, ">")
or die "$0: cannot dup STDOUT: $!\n";
exec(@_)
-or die "$0: cannot exec '@_': $!\n";
+or die "$0: cannot exec '"
+ . join(' ', _pwd(@_) )
+ . "': $!\n";
}
}
else
@@ -1560,7 +1576,7 @@
}
}
- print "Running @commandline\n";
+ print join(' ', _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(' ', _pwd(@_) )
+. "' failed $cd: exit=$exit signal=$signal\n";
}
if (wantarray)
{
@@ -1605,8 +1623,9 @@
my ($status, @output) = _read_from_pipe(@_);
if ($status)
{
- print STDERR "$0: @_ failed with this output:\n", join("\n", @output),
- "\n";
+ print STDERR
+join(' ', _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;
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
CRL and OCSP verification [was: Re: svn commit: r1863018 ...]
On 14.07.2019 00:29, Branko Čibej wrote: > On 13.07.2019 23:31, kot...@apache.org wrote: >> Author: kotkov >> Date: Sat Jul 13 21:31:25 2019 >> New Revision: 1863018 >> >> URL: http://svn.apache.org/viewvc?rev=1863018=rev >> Log: >> Win32: tweak the SSL certificate validation override to avoid hitting the >> wire >> for URL based objects and revocation checks. >> >> The primary purpose of this callback is to resolve SVN_AUTH_SSL_UNKNOWNCA >> failures using CryptoAPI and Windows local certificate stores. To do so, we >> should be fine with just using the immediately available data on the local >> machine. >> >> Doing the opposite of that appears to be troublesome, as always connecting >> to remote CRL and OCSP endpoints can result in spurious errors or significant >> (user-reported) network stalls caused by timeouts if the endpoints are >> inaccessible or unreliable. >> >> The new approach should also be in par with the default basic behavior of >> several major browsers, for example: >> >> https://chromium.googlesource.com/chromium/src/net/+/3d1dad1c17ae3ff59e7c35841af94b66f4bca1ba/cert/cert_verify_proc_win.cc#919 > > Uh. I don't think so. > > First of all, the reference to Chromium source is a red herring ... that > code disables CRL/OCSP checks *if some caller required it to*. You'll > find that browsers do, indeed, check CRL or OCSP info, if it's available. > > Your change disables all online verification, regardless, and that seems > quite wrong. > > If the server certificate contains CRL or OCSP information, the client > SHOULD verify them. If verification services are flaky, users should > report that to the server admin (who can then report it to the > certificate issuer). We don't "fix" that by removing validation > altogether in our client. > > So ... -1, please revert and let's discuss this change on the list > first. It's far more significant than you seem to think. To start off this discussion, I'll note that we don't currently use CRLs or OCSP on other platforms. This is probably because Serf 1.3.9 doesn't give us any support for that (though it does support OCSP stapling). However, the as yet unreleased Serf 1.4.0 does have support for OCSP (I'd have to double-check about CRL). In any case, given what year it is on the calendar, we SHOULD try to add OCSP verification to the client. The server can already do that for client certs, since it's only a matter of httpd configuration. -- Brane P.S.: Yes, I realize this means getting Serf 1.4.0 out the door first ...
Re: [PATCH] svn_load_dirs.pl: do not print password to screen
> > > 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. > 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.
Re: [PATCH] svn_load_dirs.pl: do not print password to screen
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
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
geoffrey.al...@gmail.com wrote on Sun, Jul 14, 2019 at 00:51:54 +1200: > 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… > 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. Cheers, Daniel
Re: [PATCH] svn_load_dirs.pl: fix broken cleanup
geoffrey.al...@gmail.com wrote on Sun, Jul 14, 2019 at 00:42:46 +1200: > CC: both of the most recent and biggest contributors to this file. I've applied some svn_load_dirs patches in the past, but I won't be able to review this one, unless it's about fixing a regression introduced by a changeset I signed off on. Please drop me from the Cc list in discussion of the patch (unless there's some question that's really specifically for me).
