[PATCH 1/2] s3fs-popt: Add function to burn the commandline password.

[Michael Adam]

Hi Andreas,

I agree with Andrew: the patch certainly does not harm, but
it might create a false sense of safety for specifying passwords
on the command line. We should not recommend that for production use.
So I am not quite certain what the patch is supposed to achieve.
Could you explain?

Cheers - Michael

On 2012-11-05 at 11:48 +1100, Andrew Bartlett wrote:
> On Sat, 2012-11-03 at 12:06 +0100, Andreas Schneider wrote:
> > Signed-off-by: Andreas Schneider <asn at samba.org>
> > ---
> >  source3/include/popt_common.h |  1 +
> >  source3/lib/popt_common.c     | 40 ++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 41 insertions(+)
> > 
> > diff --git a/source3/include/popt_common.h b/source3/include/popt_common.h
> > index 2125ed6..5266f36 100644
> > --- a/source3/include/popt_common.h
> > +++ b/source3/include/popt_common.h
> > @@ -49,5 +49,6 @@ extern const struct poptOption popt_common_dynconfig[];
> >  #define POPT_COMMON_OPTION { NULL, 0, POPT_ARG_INCLUDE_TABLE, popt_common_option, 0, "Common samba commandline config:", NULL },
> >  
> >  void popt_common_set_auth_info(struct user_auth_info *auth_info);
> > +void popt_burn_cmdline_password(int argc, char *argv[]);
> >  
> >  #endif /* _POPT_COMMON_H */
> > diff --git a/source3/lib/popt_common.c b/source3/lib/popt_common.c
> > index 94e551d..bc1bac7 100644
> > --- a/source3/lib/popt_common.c
> > +++ b/source3/lib/popt_common.c
> > @@ -605,6 +605,46 @@ void popt_common_set_auth_info(struct user_auth_info *auth_info)
> >  	global_auth_info = auth_info;
> >  }
> >  
> > +/**
> > + * @brief Burn the commandline password.
> > + *
> > + * This function removes the password from the command line so we
> > + * don't leak the password e.g. in 'ps aux'.
> > + *
> > + * It should be called after processing the options and you should pass down
> > + * argv from main().
> > + *
> > + * @param[in]  argc     The number of arguments.
> > + *
> > + * @param[in]  argv[]   The argument array we will find the array.
> > + */
> 
> G'day Andreas,
> 
> I certainly appreciate why you are doing this, but I'm not very
> comfortable with this patch.
> 
> The reason is that if we start on this, we only open ourselves up to a
> never-ending chase to try and make the insecure secure.  That is, your
> patch only handles -U, but not --user and only handles source3 C
> binaries, not python scripts nor other C binaries.  (The python scripts
> will be the most difficult).
> 
> We also start a race - the password is visible for a time, until
> overwritten - that we can't close.  However, the administrator might
> *think* it is secure, because they won't be persistent enough to win
> that race.
> 
> If we instead say 'this is, and always has been insecure', use -A if you
> want to avoid exposing credentials this way, then we have a simple
> consistent message that applies equally across all our tools.
> 
> I do realise the opposite view is 'we can do this much', and I
> appreciate the casual safety this provides against accidents - I just
> think we will just create a long-term headache in trying to 'close' this
> hole.
> 
> Andrew Bartlett
> 
> -- 
> Andrew Bartlett                                http://samba.org/~abartlet/
> Authentication Developer, Samba Team           http://samba.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 206 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20121105/af6bc2e8/attachment.pgp>