[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-nfs
Subject:    Re: [NFS] [PATCH 11/11] nfs-utils: mount: Fixed collision between
From:       Neil Brown <neilb () suse ! de>
Date:       2007-02-28 23:33:53
Message-ID: 17894.4449.797539.255376 () notabene ! brown
[Download RAW message or body]

On Tuesday February 27, kzak@redhat.com wrote:
> On Tue, Feb 27, 2007 at 05:35:58PM +1100, Neil Brown wrote:
> > On Monday February 26, SteveD@redhat.com wrote:
> > > commit 96a3ceb3d35bf5edcb9446aded8375d3b98b4f5b
> > > Author: Cory Olmo <colmo@TrustedCS.com>
> > > Date:   Sat Feb 24 16:20:40 2007 -0500
> > > 
> > >     This patch avoid the collision between commas in security contexts and the
> > >     delimiter betweeen mount options.
> > >     
> > >     Signed-off-by: Karel Zak <kzak@redhat.com>
> > >     Signed-off-by: Cory Olmo <colmo@TrustedCS.com>
> > > 
> > > diff --git a/utils/mount/mount.c b/utils/mount/mount.c
> > > index b3d3696..f22747b 100644
> > > --- a/utils/mount/mount.c
> > > +++ b/utils/mount/mount.c
> > > @@ -285,18 +285,30 @@ static void parse_opts (const char *options, int *flags, char **extra_opts)
> > >  {
> > >  	if (options != NULL) {
> > >  		char *opts = xstrdup(options);
> > > -		char *opt;
> > > -		int len = strlen(opts) + 20;
> > > -
> > > +		char *opt, *p;
> > > +		int len = strlen(opts) + 256;
> > 
> > This is a worry.  If 20 isn't big enough, why do you thing 256 will
> 
>  The 20 was big enough for conversion from usernames to UIDs, but it's
>  not enough for selinux context names. I didn't found any definition
>  for maximal length of selinux context name, so we need to select any
>  range and assume that it's enough (or completely rewrite this part of
>  the mount command and allocate all dynamically).
> 
> > be? and where do we check that the buffer doesn't overflow (in this
> > setuid program).
> 
>  Please, read the code. We check the buffer size everywhere. I don't
>  see there any place where the buffer can overflow.

OK, so I did read the code a bit more carefully, and now I'm even more
confused and concerned.

Firstly, I didn't notice that 'len' was being passed into parse_opt.
However the way it is being passed makes it totally useless.
parse_opt is called in a loop from parse_opts and each time it reduces
'len' by the amount that is added to 'extra_opts'.
However 'len' is passed by value, not by reference, so each time
around it starts at the same value and so is not effective in making
sure that extra_opts doesn't overflow it's buffer.

But further, I cannot see how extra_opts ever needs to be large than
'options'.  All that is ever appended to it are sections that have
been removed from 'options'.  So why was the +20 there in the first
place.

You mention "conversion from usernames to UIDs", but I cannot find
that anywhere.  Could you please help me understand exactly why
"extra_opts" needs to be larger than "options" ??

Thanks,
NeilBrown

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs
[prev in list] [next in list] [prev in thread] [next in thread]