[SSSD] [PATCH 0/4]: Actual memory cache implementation

Simo Sorce simo at redhat.com
Tue Jan 10 15:15:16 UTC 2012


On Mon, 2012-01-09 at 20:29 -0500, Stephen Gallagher wrote:
> On Mon, 2012-01-09 at 18:24 -0500, Simo Sorce wrote:

> > Not sure what race condition is there.
> > 
> > sssd_nss is the only thing that can create/manipulate that file, in what
> > case do you see a race condition ?
> > 
> 
> If filesystem protections are ever incorrect, then you have a situation
> where theoretically a user could do something like replace the mmap file
> with a symlink to /etc/passwd in the short gap between the stat() check
> and the file open. We would then fail to validate the file and promptly
> destroy it, hosing the system.

If that happens we have bigger problems, remember we do not store this
file in /tmp so the normal paranoia associated with user writable
directories need not apply.

> > The problem of using fstat() is that it unnecessarily(IMO) complicates
> > the code for this case.
> > 
> 
> I think it's a necessary complication. Please use it.

No, I really think it is unnecessary, and the above explanation is close
to cargo cult security to be honest.

If users can write to /var/lib/sss/mc they can simply create a passwd
file where their user has uid 0 and then login and become root. IE we
have much bigger issues than a race condition, if that dir is writable.

> > > Please use errno_t for the return code of functions that return errno
> > > values.
> > 
> > Ok.
> > 
> > > I think the check_header pieces are unnecessary. When the NSS provider
> > > is started, it should always remove an existing fast cache. This will
> > > make the behavior better match users' expectations.
> > 
> > Not sure about this, it unnecessarily causes cache lookups to go though
> > the pipe to re-populate the fast cache. Perhaps we need to discuss a bit
> > more pros-cons of either approach.
> > 
> 
> Sure, we can talk about it. I'm looking at it from the users'
> perspectives, who I think would generally expect (and be alright with)
> the fast cache being emptied on service restart. Since we still have the
> not-quite-as-fast persistent LDB cache, I think the gain isn't worth the
> user confusion.

Ok, I think I can understand this, it will also simplify the code I
guess so I'll change the patch.

> > > Don't use ftruncate() to generate the cache file. It's not safe on all
> > > platforms or filesystems. Probably better to use fseek() (even though
> > > it's a bit slower. It's a rare operation).
> > 
> > I asked our filesystem gurus and they think ftruncate() is the best way
> > to go. (and the kernel sources confirm it works with every local file
> > systems even VFAT :-)
> > 
> 
> Well, my concern was more with porting to other platforms, but I suppose
> we can burn those bridges when we come to them.

My thinking.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York




More information about the sssd-devel mailing list