Re: A strong WTF on compiling out plaintext password support by default?!

[Stefan Sperling]

On Thu, Aug 26, 2021 at 04:08:34PM -0400, Nathan Hartman wrote:
> On Thu, Aug 26, 2021 at 6:30 AM Stefan Sperling  wrote:
> > One consequence is that when Alice mistypes the --username option, or
> > mistypes the username or password at the prompt, invalid credentials will
> > be cached. Which should make any regular SVN operation fail and ask for
> > credentials again. I don't think this would be a problem, apart from the
> > possibility that invalid plaintext credentials would not be overwritten
> > by SVN binaries compiled without support for writing plaintext passwords
> > during regular operation.
> 
> That could be mitigated by providing a "svn auth remove" that deletes
> a cached credential.

Yes, removing the credential would also unblock the situation, as would
updating the credential.

Thankfully, svn auth --remove already exists :)

auth: Manage cached authentication credentials.
usage: 1. svn auth [PATTERN ...]
   2. svn auth --remove PATTERN [PATTERN ...]

Re: A strong WTF on compiling out plaintext password support by default?!

[Nathan Hartman]

On Thu, Aug 26, 2021 at 6:30 AM Stefan Sperling  wrote:
> One consequence is that when Alice mistypes the --username option, or
> mistypes the username or password at the prompt, invalid credentials will
> be cached. Which should make any regular SVN operation fail and ask for
> credentials again. I don't think this would be a problem, apart from the
> possibility that invalid plaintext credentials would not be overwritten
> by SVN binaries compiled without support for writing plaintext passwords
> during regular operation.

That could be mitigated by providing a "svn auth remove" that deletes
a cached credential.

Nathan

Re: Broken pipe on the diff command with --diff-cmd

[Vincent Lefevre]

On 2021-08-25 19:18:49 -0400, Nathan Hartman wrote:
> I don't see mention of it in the issue tracker. Please could you file
> it there?

Done: https://issues.apache.org/jira/browse/SVN-4879

> I started looking into it but ran out of time for now. Anyway, SIGPIPE
> is ignored throughout the client, for good reasons, since r848296:
> 
> (relevant bits of log message):
> 
>- Ignore SIGPIPE, to prevent wedging the repos on network errors,
> or when piping the output to a command that quits before all data
> is read, e.g. head, more or less.
> 
> SIGPIPE is disabled in the client (and 2 other programs) by:
> 
> +#ifdef SIGPIPE
> +  /* Disable SIGPIPE generation for the platforms that have it. */
> +  apr_signal(SIGPIPE, SIG_IGN);
> +#endif
> 
> It would be easy to temporarily re-enable it before executing diff-cmd
> but first it's important to make sure that won't break another use
> case which the current disabling is handling. (An example of one
> question that comes to mind: is it possible to use --diff-cmd with
> 'svn log -r --diff' and if so are there any ramifications to
> re-enabling SIGPIPE in that scenario? I haven't checked yet.)

I added a simpler suggestion in the report:

A possible fix (to be tested): instead of ignoring SIGPIPE (using
SIG_IGN), use a handler that does nothing. That way, when a command is
executed, SIGPIPE will be reset to the default action in the command
(i.e. killing the command, unless the command itself changes how it
handles SIGPIPE).

-- 
Vincent Lefèvre  - Web: 
100% accessible validated (X)HTML - Blog: 
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)

Re: A strong WTF on compiling out plaintext password support by default?!

[Johan Corveleyn]

On Thu, Aug 26, 2021 at 4:31 PM Daniel Shahaf  wrote:
>
> Johan Corveleyn wrote on Thu, 26 Aug 2021 12:41 +00:00:
> > On Wed, Aug 25, 2021 at 8:52 PM Daniel Shahaf  
> > wrote:
> > > This thread is on dev@ as opposed to users@, so I'm trying to solve the
> > > problem generically, rather than just your specific $WORK scenario.
> >
> > I get the feeling I'm missing something, but I still don't understand
> > what authz has to do with the problem at hand here (i.e. detecting
> > expired passwords so we can ask the user for the new one).
>
> Your problem statement is "Replace cached passwords that are expired".
>
> I'm solving a more general problem statement, "Replace cached passwords
> that can't be used to commit with", regardless of why.

Okay, sure, but that's another question than what we started with.

BTW, I don't really follow how you can replace just a cached password
for getting "write access". Doesn't "upgrading from read-only to
read-write access" also imply using another username? Or can I have
two passwords for one user, where one gives me read-only access and
one gives me write access?

I.e. shouldn't the more general problem statement be "Replace cached
username+passwords that can't be used to commit with"?

(hence my first response with "huh, shouldn't a --username make that
problem moot"? I.e. the user knows which user he's willing to connect
with, and he knows the authz rules, he just wants the password to be
checked and "re-cached" if need be)

-- 
Johan

Re: [PATCH] Issue #4711

[Daniel Shahaf]

Daniel Sahlberg wrote on Thu, Aug 26, 2021 at 14:44:04 +0200:
> Den lör 21 aug. 2021 kl 05:18 skrev Daniel Shahaf :
> 
> > Daniel Sahlberg wrote on Fri, 20 Aug 2021 10:30 +00:00:
> > > Den fre 20 aug. 2021 kl 12:11 skrev Daniel Shahaf <
> > d...@daniel.shahaf.name>:
> > > > Daniel Sahlberg wrote on Thu, Aug 19, 2021 at 23:23:49 +0200:
> > > > >  [[[
> > > > ⋮
> > > > >
> > 
> > > > > r4 | dsahlberg | 2021-08-19 21:58:28 +0200 (Thu, 19 Aug 2021) | 1
> > line
> > > > > Merged via: r7, r5
> > > > > ]]]
> > > > > As can be seen here, it is not visible in the plain text output that
> > r7 was
> > > > > actually a reverse merge.
> > > >
> > > > Hmm.  There's an interesting question of whether we can amend the
> > output
> > > > to make that clear.  The CLI output is an API, and generally we try not
> > > > to change it unless the user opts in to new features (e.g., when
> > > > changelists were added, the CLI output for users who don't use
> > > > changelists wasn't changed), but there are exceptions (e.g., adding the
> > > > seventh column to `svn status` non-XML output).
> > >
> > > The two options today are:
> > > - Reverse merged via: r7
> > > or:
> > > - Merged via: r7, r5
> > >
> > > The code is looking at the the way r4 was merged and then prints the
> > > revision numbers from the merge stack. With the patch we have enough
> > > information on the merge stack to print "(reverse)" on each revision
> > > that was subtractive, for example:
> > > - Merged via: r7 (reverse), r5
> > >
> > > Another option would be to print both (and each revision only in the
> > > line where it belongs).
> > > - Reverse merged via: r7
> > > - Merged via: r5
> > > To do this one would have to loop through the merge_stack twice (at
> > > least) or keep a reasonably long buffer to store the list of revisions
> > > in case of a long stack.
> > >
> >
> > Hmm.  In favour of the second option is that it doesn't require CLI-
> > parsing tools to learn about new syntax, but only to learn about two
> > existing syntaxes being able to appear concurrently.  With a little
> > luck, some of them will DTRT.
> >
> > Is there anything the first option can represent that the second one
> > can't?  Perhaps some tree-like cases (rN was a merge of both rFOO and
> > rBAR), but those aren't representable in the plain output anyway.
> >
> 
> > For a buffer, we have svn_stringbuf_t that automatically reallocates
> > itself as needed; and in any case, a two-pass algorithm would be easy to
> > implement and to read and performant (it'd still be O(N)).
> >
> > > Both these changes might mess it up for someone consuming the CLI
> > > output and expecting to find one or the other of Merged via: or Reverse
> > > merged via:, or expecting to find just a simple list of revisions. I
> > > can't judge the risk/reward for changing this.
> >
> > Would it be accurate to say that the incumbent output is wrong,
> > misleading, a bug?
> >
> 
> It would at least be misleading, since it isn't possible to figure out if a
> merge some way down the tree was a forward or reverse merge. But that would
> be easy to discover when looking at the code or inspecting the merges more
> closely.

I'm not sure if "that would be easy to discover" is good enough
a reason.  That's an argument could also be used to justify, say,
deleting the «svn diff» command.  Determining reverse merges is
something we can do easily and would be useful, so why not do it?

> If so, we could make the change, as a bugfix.  The release notes could
> > point it out and point to the XML output as a more-stable alternative.
> >
> > If not, we could record the idea as a milestone=2.0 issue.
> >
> 
> I have let this question soak for a few days hoping someone else with more
> experience would chime in.
> 

Personally, I think I'm leaning towards "it's misleading".  I'd probably
prefer this alternative —

> > > Another option would be to print both (and each revision only in the
> > > line where it belongs).
> > > - Reverse merged via: r7
> > > - Merged via: r5

— since it requires the minimal change to parsers: only to be aware that
if a "Reverse merged" line is found, there may be a "Merged via" line
before the next blank line.  I expect few parsers would need to be
adjusted for this, and it would be more friendly to humans.

However, this _would_ be an incompatible change for anyone parsing only
the "Merged via" line, so I can certainly see the logic behind relegating
this to 2.0…

… except that there's nothing preventing us from adding _another_ line
to the output, alongside the existing «Merged via: r7, r5» line, to
indicate the merge directions with.

> I'm leaning towards setting this as a milestone=2.0 issue and for the time
> leaving it alone not to risk destabilising anything.

That's an option.  Or we could include this change in the next alpha or
beta release to get feedback on it.  (We might even be able to use
a preprocessor 

Re: A strong WTF on compiling out plaintext password support by default?!

[Daniel Shahaf]

Johan Corveleyn wrote on Thu, 26 Aug 2021 12:41 +00:00:
> On Wed, Aug 25, 2021 at 8:52 PM Daniel Shahaf  wrote:
> > This thread is on dev@ as opposed to users@, so I'm trying to solve the
> > problem generically, rather than just your specific $WORK scenario.
> 
> I get the feeling I'm missing something, but I still don't understand
> what authz has to do with the problem at hand here (i.e. detecting
> expired passwords so we can ask the user for the new one).

Your problem statement is "Replace cached passwords that are expired".

I'm solving a more general problem statement, "Replace cached passwords
that can't be used to commit with", regardless of why.

Re: A strong WTF on compiling out plaintext password support by default?!

[Daniel Shahaf]

Branko Čibej wrote on Thu, 26 Aug 2021 12:49 +00:00:
> On 26.08.2021 14:10, Daniel Shahaf wrote:
> > Branko Čibej wrote on Thu, 26 Aug 2021 08:11 +00:00:
> >> On 25.08.2021 21:01, Mark Phippard wrote:
> >>> Solving with svn auth is a nice idea but I do not see it working
> >>> unless we have a way to authenticate for write access without writing
> >>> something.
> >> There isn't in general, since authz can complicate matters. And there
> >> isn't currently, we don't have server-side support for that. I'm not
> >> even sure we could add a server-side method for this check, since the
> >> check for write access can be done entirely outside of Subversion. "svn
> >> authz write-check $url" sounds plausible until you consider all the
> >> various possible authn/authz checking combinations.
> > I don't see the problem.  What's implausible about writing an RA API
> > that authenticates the client, takes a path and an "is recursive?" bit,
> > and returns the result of «svnauthz accessof» on that path?  That's
> > basically what the revprop edit codepath will do in the default
> > configuration (with the pre- hook not existing).
> 
> That part is not implausible. It would have to be implemented in a way 
> that works when part of the authz processing is done outside of 
> Subversion, e.g., it should use an HTTP method that requires write 
> access.

An HTTPv2 POST, then?

> It's also not backward-compatible, I'd expect "svn auth add" to 
> work reasonably well against older servers.

It's not possible to test write access against older servers, but that
doesn't mean we have to wait until 2.0 to make that possible: it's a new
feature, not an incompatible change.

> I'm not comfortable with the idea of updating the authn cache without 
> contacting the server when we could.

…why?

Cheers,

Daniel

Re: A strong WTF on compiling out plaintext password support by default?!

[Stefan Sperling]

On Thu, Aug 26, 2021 at 04:17:16PM +0200, Daniel Sahlberg wrote:
> Den tors 26 aug. 2021 kl 16:10 skrev Stefan Sperling :
> 
> > On Thu, Aug 26, 2021 at 02:41:44PM +0200, Johan Corveleyn wrote:
> > > I get the feeling I'm missing something, but I still don't understand
> > > what authz has to do with the problem at hand here (i.e. detecting
> > > expired passwords so we can ask the user for the new one).
> >
> > The problem is that some repositories (like our own) do not require
> > any authentication in order to read data.
> >
> > Your case where 'svn ls' asks for a password is not applicable for
> > public repositories on svn.apache.org, for example. The 'svn auth add'
> > command would not get an authentication challenge by running the
> > equivalent of what 'svn ls' is doing. We do not have a way to trigger
> > the challenge without modifying the repository somehow.
> >
> 
> Is it possible to have the client "throw" the username/password at the
> server even if the server doesn't issue a challenge? Would the server
> validate the username/password (even though authz would allow anonymous
> access)?
> 
> /Daniel Sahlberg

Unfortunately, it is not. There are many authentication schemes and
at least two protocols to consider (HTTP + svn).
Some authentication schemes even require data that is generated on the
server when it sends the authentication request, such as a nonce.

Re: A strong WTF on compiling out plaintext password support by default?!

[Daniel Sahlberg]

Den tors 26 aug. 2021 kl 16:10 skrev Stefan Sperling :

> On Thu, Aug 26, 2021 at 02:41:44PM +0200, Johan Corveleyn wrote:
> > I get the feeling I'm missing something, but I still don't understand
> > what authz has to do with the problem at hand here (i.e. detecting
> > expired passwords so we can ask the user for the new one).
>
> The problem is that some repositories (like our own) do not require
> any authentication in order to read data.
>
> Your case where 'svn ls' asks for a password is not applicable for
> public repositories on svn.apache.org, for example. The 'svn auth add'
> command would not get an authentication challenge by running the
> equivalent of what 'svn ls' is doing. We do not have a way to trigger
> the challenge without modifying the repository somehow.
>

Is it possible to have the client "throw" the username/password at the
server even if the server doesn't issue a challenge? Would the server
validate the username/password (even though authz would allow anonymous
access)?

/Daniel Sahlberg

Re: A strong WTF on compiling out plaintext password support by default?!

[Stefan Sperling]

On Thu, Aug 26, 2021 at 12:15:39PM +, Daniel Shahaf wrote:
> Stefan Sperling wrote on Thu, 26 Aug 2021 10:30 +00:00:
> > And while we are considering read-only vs. read-write access:
> > Plaintext passwords or not, in my contrived scenario Eve could always
> > trick Alice into using a different user account by caching a set of
> > valid credentials which Eve knows. Apart from not caching credentials
> > at all I don't see a way to prevent this.
> 
> That scenario is called an "evil maid attack".  I don't think we should
> try to prevent it.  We are not in the business of posting guards to watch
> over unattended laptops.

The plaintext password pishing scenario also requires access to
local configuration files. We could simply declare it out of scope,
but that means we'd be ignoring users who are unhappy that plaintext
storage is even allowed. Just as they are unhappy about TortoiseSVN's
decryption shortcut in its cached password dialog (note that in this
case the windows domain password is often the same as the SVN password,
so leaving a laptop unlocked means anyone can get at domain creds).

Re: A strong WTF on compiling out plaintext password support by default?!

[Stefan Sperling]

On Thu, Aug 26, 2021 at 02:41:44PM +0200, Johan Corveleyn wrote:
> I get the feeling I'm missing something, but I still don't understand
> what authz has to do with the problem at hand here (i.e. detecting
> expired passwords so we can ask the user for the new one).

The problem is that some repositories (like our own) do not require
any authentication in order to read data.

Your case where 'svn ls' asks for a password is not applicable for
public repositories on svn.apache.org, for example. The 'svn auth add'
command would not get an authentication challenge by running the
equivalent of what 'svn ls' is doing. We do not have a way to trigger
the challenge without modifying the repository somehow.

Re: A strong WTF on compiling out plaintext password support by default?!

[Mark Phippard]

On Thu, Aug 26, 2021 at 6:30 AM Stefan Sperling  wrote:

> The answer might be that 'svn authz add' should simply not contact the
> server to check credentials. Which means we cannot check upfront whether
> the user running 'svn auth add' knows valid credentials.

Yeah that seems reasonable. Basically an "official" version of what
the scripts are doing.

All that said, I still favor restoring the previous behavior where
this is enabled by default and a packager can compile it out if they
so choose.

Mark

Re: A strong WTF on compiling out plaintext password support by default?!

[Branko Čibej]

On 26.08.2021 14:10, Daniel Shahaf wrote:

Branko Čibej wrote on Thu, 26 Aug 2021 08:11 +00:00:

On 25.08.2021 21:01, Mark Phippard wrote:

Solving with svn auth is a nice idea but I do not see it working
unless we have a way to authenticate for write access without writing
something.

There isn't in general, since authz can complicate matters. And there
isn't currently, we don't have server-side support for that. I'm not
even sure we could add a server-side method for this check, since the
check for write access can be done entirely outside of Subversion. "svn
authz write-check $url" sounds plausible until you consider all the
various possible authn/authz checking combinations.

I don't see the problem.  What's implausible about writing an RA API
that authenticates the client, takes a path and an "is recursive?" bit,
and returns the result of «svnauthz accessof» on that path?  That's
basically what the revprop edit codepath will do in the default
configuration (with the pre- hook not existing).


That part is not implausible. It would have to be implemented in a way 
that works when part of the authz processing is done outside of 
Subversion, e.g., it should use an HTTP method that requires write 
access. It's also not backward-compatible, I'd expect "svn auth add" to 
work reasonably well against older servers.


I'm not comfortable with the idea of updating the authn cache without 
contacting the server when we could.


-- Brane

Re: [PATCH] Issue #4711

[Daniel Sahlberg]

Den lör 21 aug. 2021 kl 05:18 skrev Daniel Shahaf :

> Daniel Sahlberg wrote on Fri, 20 Aug 2021 10:30 +00:00:
> > Den fre 20 aug. 2021 kl 12:11 skrev Daniel Shahaf <
> d...@daniel.shahaf.name>:
> > > Daniel Sahlberg wrote on Thu, Aug 19, 2021 at 23:23:49 +0200:
> > > >  [[[
> > > ⋮
> > > >
> 
> > > > r4 | dsahlberg | 2021-08-19 21:58:28 +0200 (Thu, 19 Aug 2021) | 1
> line
> > > > Merged via: r7, r5
> > > > ]]]
> > > > As can be seen here, it is not visible in the plain text output that
> r7 was
> > > > actually a reverse merge.
> > >
> > > Hmm.  There's an interesting question of whether we can amend the
> output
> > > to make that clear.  The CLI output is an API, and generally we try not
> > > to change it unless the user opts in to new features (e.g., when
> > > changelists were added, the CLI output for users who don't use
> > > changelists wasn't changed), but there are exceptions (e.g., adding the
> > > seventh column to `svn status` non-XML output).
> >
> > The two options today are:
> > - Reverse merged via: r7
> > or:
> > - Merged via: r7, r5
> >
> > The code is looking at the the way r4 was merged and then prints the
> > revision numbers from the merge stack. With the patch we have enough
> > information on the merge stack to print "(reverse)" on each revision
> > that was subtractive, for example:
> > - Merged via: r7 (reverse), r5
> >
> > Another option would be to print both (and each revision only in the
> > line where it belongs).
> > - Reverse merged via: r7
> > - Merged via: r5
> > To do this one would have to loop through the merge_stack twice (at
> > least) or keep a reasonably long buffer to store the list of revisions
> > in case of a long stack.
> >
>
> Hmm.  In favour of the second option is that it doesn't require CLI-
> parsing tools to learn about new syntax, but only to learn about two
> existing syntaxes being able to appear concurrently.  With a little
> luck, some of them will DTRT.
>
> Is there anything the first option can represent that the second one
> can't?  Perhaps some tree-like cases (rN was a merge of both rFOO and
> rBAR), but those aren't representable in the plain output anyway.
>

> For a buffer, we have svn_stringbuf_t that automatically reallocates
> itself as needed; and in any case, a two-pass algorithm would be easy to
> implement and to read and performant (it'd still be O(N)).
>
> > Both these changes might mess it up for someone consuming the CLI
> > output and expecting to find one or the other of Merged via: or Reverse
> > merged via:, or expecting to find just a simple list of revisions. I
> > can't judge the risk/reward for changing this.
>
> Would it be accurate to say that the incumbent output is wrong,
> misleading, a bug?
>

It would at least be misleading, since it isn't possible to figure out if a
merge some way down the tree was a forward or reverse merge. But that would
be easy to discover when looking at the code or inspecting the merges more
closely.

If so, we could make the change, as a bugfix.  The release notes could
> point it out and point to the XML output as a more-stable alternative.
>
> If not, we could record the idea as a milestone=2.0 issue.
>

I have let this question soak for a few days hoping someone else with more
experience would chime in.

I'm leaning towards setting this as a milestone=2.0 issue and for the time
leaving it alone not to risk destabilising anything.

Have you reviewed the patch from a coding style perspective? I would
appreciate a +1 from someone before committing.

Kind regards,
Daniel

Re: A strong WTF on compiling out plaintext password support by default?!

[Johan Corveleyn]

On Wed, Aug 25, 2021 at 8:52 PM Daniel Shahaf  wrote:
> Johan Corveleyn wrote on Wed, 25 Aug 2021 07:16 +00:00:
> > On Tue, Aug 24, 2021 at 7:03 PM Daniel Shahaf  
> > wrote:
> > > Johan Corveleyn wrote on Tue, 24 Aug 2021 15:22 +00:00:
> > > > On Tue, Aug 24, 2021 at 4:45 PM Daniel Shahaf  
> > > > wrote:
> > > > > Johan Corveleyn wrote on Tue, 24 Aug 2021 14:25 +00:00:
> > > > > > OTOH, if this kind of behaviour is too far-fetched for a single
> > > > > > subcommand, I might be able to do it by invoking two commands, if I
> > > > > > could succesfully (and invisibly) detect that a cached password is 
> > > > > > no
> > > > > > longer correct. As in:
> > > > > >
> > > > > > svn ls --non-interactive $URL >/dev/null
> > > > > > # if exit-code != 0, parse error code to see if it indicates 
> > > > > > "auth failed"
> > > > > > if ("auth failed"):
> > > > > > svn auth add $URL
> > > > > >
> > > > >
> > > > > What happens if a valid username/password are cached that have read-
> > > > > only access and one wants to preseed a username/password that have 
> > > > > read-
> > > > > write access?
> > > >
> > > > I don't know. We don't have that use case at $company, trying to keep
> > > > things simple :-).
> > > >
> > > > Ah but shouldn't 'svn auth' carry an optional --username parameter? In
> > > > that case, I don't see the problem, I guess.
> > >
> > > My point here is that that pseudocode doesn't handle ensuring that the
> > > cached credentials have read-write access.  Existence of «svn auth 
> > > --username»
> > > won't change that, because the «svn auth» call is inside an if() block
> > > whose condition will false negative.
> > >
> > > Is there a way to test whether one has rw access without actually doing
> > > a commit or a revprop edit?  It's possible with hooks, of course, but is
> > > it also possible without hooks?
> >
> > I'm not sure I understand: why would I need to know that the cached
> > credentials have read-write access?
> >
>
> Your pseudocode detects credentials that have no read access, whether
> for lack of authz or for failing to authn.

Oh? I was assuming that we would be able to discern failed authn (as
in "incorrect username or password") from failed authz (as in "403
Forbidden"). Is that not the case? Do both failed authn and failed
authz yield the same error code / message?

I'm only interested in failed authn, because this entire thread is
about the disabling of the plaintext password cache, and I just want
to know if the cached password is now invalid (with failed authn) so I
have to ask the user for a new one (to potentially use another tool to
inject that new password into the plaintext cache).

> I was asking how to
> generalize this to detecting credentials that successfully authn and
> authz but only for read-only access rather than for full access.  That
> would be necessary if the "preseeded" credentials are to be used for
> a write operation (a commit or a revprop edit).
>
> > I only want to verify the authentication part,
>
> This thread is on dev@ as opposed to users@, so I'm trying to solve the
> problem generically, rather than just your specific $WORK scenario.

I get the feeling I'm missing something, but I still don't understand
what authz has to do with the problem at hand here (i.e. detecting
expired passwords so we can ask the user for the new one).

-- 
Johan

Re: A strong WTF on compiling out plaintext password support by default?!

[Daniel Shahaf]

Stefan Sperling wrote on Thu, 26 Aug 2021 10:30 +00:00:
> And while we are considering read-only vs. read-write access:
> Plaintext passwords or not, in my contrived scenario Eve could always
> trick Alice into using a different user account by caching a set of
> valid credentials which Eve knows. Apart from not caching credentials
> at all I don't see a way to prevent this.

That scenario is called an "evil maid attack".  I don't think we should
try to prevent it.  We are not in the business of posting guards to watch
over unattended laptops.

Re: A strong WTF on compiling out plaintext password support by default?!

[Daniel Shahaf]

Branko Čibej wrote on Thu, 26 Aug 2021 08:11 +00:00:
> On 25.08.2021 21:01, Mark Phippard wrote:
> > Solving with svn auth is a nice idea but I do not see it working
> > unless we have a way to authenticate for write access without writing
> > something.
> 
> There isn't in general, since authz can complicate matters. And there 
> isn't currently, we don't have server-side support for that. I'm not 
> even sure we could add a server-side method for this check, since the 
> check for write access can be done entirely outside of Subversion. "svn 
> authz write-check $url" sounds plausible until you consider all the 
> various possible authn/authz checking combinations.

I don't see the problem.  What's implausible about writing an RA API
that authenticates the client, takes a path and an "is recursive?" bit,
and returns the result of «svnauthz accessof» on that path?  That's
basically what the revprop edit codepath will do in the default
configuration (with the pre- hook not existing).

Re: A strong WTF on compiling out plaintext password support by default?!

[Stefan Sperling]

On Thu, Aug 26, 2021 at 10:11:44AM +0200, Branko Čibej wrote:
> On 25.08.2021 21:01, Mark Phippard wrote:
> > On Wed, Aug 25, 2021 at 3:16 AM Johan Corveleyn  wrote:
> > 
> > > > Is there a way to test whether one has rw access without actually doing
> > > > a commit or a revprop edit?  It's possible with hooks, of course, but is
> > > > it also possible without hooks?
> > > I'm not sure I understand: why would I need to know that the cached
> > > credentials have read-write access?
> > I think it was a good question. It is hard to predict if a build
> > process just needs read access or read-write. If it needs the latter
> > it could complicate the effectiveness of a solution that goes down
> > this path. For example, imagine a scenario where the server allows
> > anonymous read access .. it will not even be possible to check
> > credentials unless you attempt a write operation.
> > 
> > I was never super excited about this change to disallow plain text
> > passwords but I figured fighting against a security issue is a losing
> > battle. I personally prefer the suggestion of making it a compile
> > option to disallow plain text passwords and have them enabled by
> > default and just turned off in the default configuration. The
> > alice/eve scenario while valid just does not concern me.
> > 
> > Solving with svn auth is a nice idea but I do not see it working
> > unless we have a way to authenticate for write access without writing
> > something.

Thanks for bringing this up, Daniel and Mark.
This is indeed an important point.

> There isn't in general, since authz can complicate matters. And there isn't
> currently, we don't have server-side support for that. I'm not even sure we
> could add a server-side method for this check, since the check for write
> access can be done entirely outside of Subversion. "svn authz write-check
> $url" sounds plausible until you consider all the various possible
> authn/authz checking combinations.

The answer might be that 'svn authz add' should simply not contact the
server to check credentials. Which means we cannot check upfront whether
the user running 'svn auth add' knows valid credentials.

I think this may still be better than the alternative where configuration
files can be tweaked to trick Alice into unknowingly saving her password
in plaintext while running regular SVN operations. Having 'svn auth' be
the only command which would write a plaintext password does provide some
protection in this scenario, regardless of whether credentials are checked
against the server before they get cached.

One consequence is that when Alice mistypes the --username option, or
mistypes the username or password at the prompt, invalid credentials will
be cached. Which should make any regular SVN operation fail and ask for
credentials again. I don't think this would be a problem, apart from the
possibility that invalid plaintext credentials would not be overwritten
by SVN binaries compiled without support for writing plaintext passwords
during regular operation.

In the absence of non-plaintext auth caches SVN would then keep asking
for a password until 'svn authz add --plaintext' is used again to store
valid credentials.

This problem also applies to any helper scripts that people might be
running right now in order to add plaintext credentials to the auth cache.

To mitigate this problem a bit, we could have 'svn auth add' ask for the
password twice and repeat the prompt in a loop in case of a mismatch.

And while we are considering read-only vs. read-write access:
Plaintext passwords or not, in my contrived scenario Eve could always
trick Alice into using a different user account by caching a set of
valid credentials which Eve knows. Apart from not caching credentials
at all I don't see a way to prevent this.

Re: A strong WTF on compiling out plaintext password support by default?!

[Branko Čibej]

On 25.08.2021 21:01, Mark Phippard wrote:

On Wed, Aug 25, 2021 at 3:16 AM Johan Corveleyn  wrote:


Is there a way to test whether one has rw access without actually doing
a commit or a revprop edit?  It's possible with hooks, of course, but is
it also possible without hooks?

I'm not sure I understand: why would I need to know that the cached
credentials have read-write access?

I think it was a good question. It is hard to predict if a build
process just needs read access or read-write. If it needs the latter
it could complicate the effectiveness of a solution that goes down
this path. For example, imagine a scenario where the server allows
anonymous read access .. it will not even be possible to check
credentials unless you attempt a write operation.

I was never super excited about this change to disallow plain text
passwords but I figured fighting against a security issue is a losing
battle. I personally prefer the suggestion of making it a compile
option to disallow plain text passwords and have them enabled by
default and just turned off in the default configuration. The
alice/eve scenario while valid just does not concern me.

Solving with svn auth is a nice idea but I do not see it working
unless we have a way to authenticate for write access without writing
something.


There isn't in general, since authz can complicate matters. And there 
isn't currently, we don't have server-side support for that. I'm not 
even sure we could add a server-side method for this check, since the 
check for write access can be done entirely outside of Subversion. "svn 
authz write-check $url" sounds plausible until you consider all the 
various possible authn/authz checking combinations.


-- Brane