headers
Elad Efrat | 23 Jun 2007 17:23

Re: CVS commit: src/sys

Alistair Crooks wrote:
> On Sat, Jun 23, 2007 at 01:15:21PM +0300, Elad Efrat wrote:
>> David Laight wrote:
>>> Module Name:	src
>>> Committed By:	dsl
>>> Date:		Sat Jun 23 09:02:13 UTC 2007
>>>
>>> Modified Files:
>>> 	src/sys/kern: kern_auth.c
>>> 	src/sys/sys: kauth.h
>>>
>>> Log Message:
>>> Simplify the interfaces needed for sys_setgroups() and sys_getgroups().
>>> Exposed that the kauth code holds groups in an array, but removes some
>>> of the knowledge of the maximum number of groups.
>>> Allows the syscall code to copyin/out directly to/from the cred structure,
>>> this save a lot of faffing about with malloc/free even when compat code
>>> has to use 16bit groups.
>>>
>>>
>>> To generate a diff of this commit:
>>> cvs rdiff -r1.47 -r1.48 src/sys/kern/kern_auth.c
>>> cvs rdiff -r1.37 -r1.38 src/sys/sys/kauth.h
>>>
>>> Please note that diffs are not public domain; they are subject to the
>>> copyright notices on the relevant files.
>>>
>> you are changing a critical kernel interface. where was this brought up
>> for discussion?
>
(Continue reading)

Permalink | Reply | 0 comments |
headers
Bernd Ernesti | 23 Jun 2007 18:04
Picon

Re: RFC: getopt_long(3) change

[tech-security added to the list to get some attention from there]

On Sat, Jun 23, 2007 at 03:25:38PM +0000, Christos Zoulas wrote:
> In article <20070623080627.GA18016 <at> arresum.veego.de>,
> Bernd Ernesti  <tech-userlevel <at> netbsd.org> wrote:
> >On Sat, Jun 23, 2007 at 08:45:07AM +0100, David Laight wrote:
> >> On Sat, Jun 23, 2007 at 09:05:11AM +0200, Alan Barrett wrote:
> >> > On Fri, 22 Jun 2007, Brian Ginsbach wrote:
> >> > > The long options are set so that both "color" and "colour" would
> >> > > be supported and both return the same value when parsed.  There
> >> > > are no other long options that start with "col".
> >> > > 
> >> > > A command is passed the option --col.  The current version of NetBSD
> >> > > getopt_long(3) treats this as an ambiguous argument.  This was the
> >> > > behavior of GNU getopt_long(3) for GNU C library 2.1.3.  Later
> >> > > versions of the GNU C library doesn't treat this as ambiguous.
> >> > 
> >> > Thank you.  So the fix is something like "if it looks like an ambiguous
> >> > abbreviation of two or more long options, but all the possible
> >> > interpretations would return the same value, then just return that value
> >> > without complaining that it's ambiguous."  This seems sensible.
> >> 
> >> Actually it is a stupid idea!
> >> Or rather allowing partial matches is what is stupid.
> >> It is all very well when the commands are typed at the keryboard,
> >> but the shortened forms will end up in shell scripts.
> >> Then, another option will get added that has the same initial part
> >> and the scripts suddenly starts failing after the program gets updated.
> >
> >What happens when I accidentally mistype a char and so this 'feature'
(Continue reading)

Permalink | Reply | 0 comments |
headers
David Laight | 23 Jun 2007 18:31
Picon

Re: CVS commit: src/sys

On Sat, Jun 23, 2007 at 06:23:44PM +0300, Elad Efrat wrote:
> does "compat code with one less malloc" weighs more than "opaque and
> abstract interface allowing various pluggable secmodels"?

Actually the malloc/free was on every call to sys_set/getgroups,
not just those in the compat code.

One benefit of my changes is that the NGROUPS constant is no
longer in the sys_setgroups() functions, possibly allowing it
to be dynamically changable (etc) without changing the interface
to LKM compat code.

	David

--

-- 
David Laight: david <at> l8s.co.uk
Permalink | Reply | 0 comments |
headers
Elad Efrat | 23 Jun 2007 18:39

Re: CVS commit: src/sys

David Laight wrote:
> On Sat, Jun 23, 2007 at 06:23:44PM +0300, Elad Efrat wrote:
>> does "compat code with one less malloc" weighs more than "opaque and
>> abstract interface allowing various pluggable secmodels"?
> 
> Actually the malloc/free was on every call to sys_set/getgroups,
> not just those in the compat code.

how common are calls to syscalls? can you show performance tests that
indicate a clear measurable system performance improvement as a result
of removing the malloc/free?

> One benefit of my changes is that the NGROUPS constant is no
> longer in the sys_setgroups() functions, possibly allowing it
> to be dynamically changable (etc) without changing the interface
> to LKM compat code.

your changes may introduce this benefit, but are overshadowed by the
simple fact that you broke a critical kernel interface and exposed its
internals, eliminating one of its key design goals of opacity -- and
you did so without any prior discussion.

while the changes to get/setgroups syscall internals and compat calls
will not change the user experience in any way, breaking kauth's opacity
have direct and immediate implications in the form of not allowing much
flexibility when implementing new security models that expand beyond
what is currently allowed by bsd44.

additionally, it is well worth pointing out that the benefit you
introduced is orthogonal to breaking the interface's opacity, and could
(Continue reading)

Permalink | Reply | 0 comments |
headers
Elad Efrat | 23 Jun 2007 18:43

Re: CVS commit: src/sys

Elad Efrat wrote:
> David Laight wrote:
>> On Sat, Jun 23, 2007 at 06:23:44PM +0300, Elad Efrat wrote:
>>> does "compat code with one less malloc" weighs more than "opaque and
>>> abstract interface allowing various pluggable secmodels"?
>>
>> Actually the malloc/free was on every call to sys_set/getgroups,
>> not just those in the compat code.
> 
> how common are calls to syscalls? can you show performance tests that

obviously a very important "the set/getgroups" was omitted here. :)

> indicate a clear measurable system performance improvement as a result
> of removing the malloc/free?
> 
>> One benefit of my changes is that the NGROUPS constant is no
>> longer in the sys_setgroups() functions, possibly allowing it
>> to be dynamically changable (etc) without changing the interface
>> to LKM compat code.
> 
> your changes may introduce this benefit, but are overshadowed by the
> simple fact that you broke a critical kernel interface and exposed its
> internals, eliminating one of its key design goals of opacity -- and
> you did so without any prior discussion.
> 
> while the changes to get/setgroups syscall internals and compat calls
> will not change the user experience in any way, breaking kauth's opacity
> have direct and immediate implications in the form of not allowing much
> flexibility when implementing new security models that expand beyond
(Continue reading)

Permalink | Reply | 0 comments |
headers
Elad Efrat | 23 Jun 2007 19:11

Re: CVS commit: src/sys

Jachym Holecek wrote:
> [Stripping CC somewhat]

adding cc back. this is very relevant to all lists, and potentially
current-users <at>  and netbsd-users <at>  as well, as this damages a framework
in -current and future releases if it stays in the tree.

> 
> # Elad Efrat 2007-06-23:
>> while the changes to get/setgroups syscall internals and compat calls
>> will not change the user experience in any way, breaking kauth's opacity
>> have direct and immediate implications in the form of not allowing much
>> flexibility when implementing new security models that expand beyond
>> what is currently allowed by bsd44.
> 
> Could you provide some specific examples of what was possible before
> but will be impossible because of David's change?
> 
>> additionally, it is well worth pointing out that the benefit you
>> introduced is orthogonal to breaking the interface's opacity, and could
>> have been introduced either way.
> 
> I don't quite see how opacity gets harmed -- the group list was a flat
> array before and it's still a flat array now...

you're now getting a pointer to an internal buffer where you can change
it directly without going through the interface.

and again, my frustration is not only with random developers breaking
critical kernel interfaces for stupid reasons with implications beyond
(Continue reading)

Permalink | Reply | 0 comments |
headers
Alistair Crooks | 23 Jun 2007 19:37

Re: CVS commit: src/sys

On Sat, Jun 23, 2007 at 06:23:44PM +0300, Elad Efrat wrote:
> I don't understand how kauth not being maintained means it's okay to
> expose its internals.

If kauth is not being maintained, why are you bothering to copy the
world and its dog on your email?

I know it's much easier to stand on the sidelines and shout whenever
anyone touches a file with the letters "kauth" anywhere in its name,
but it's not getting you or the project any further along the road.

If you care about kauth, agree to the same rules that every other
developer does, and maintain it. If you don't, it will bitrot, and
will have to be thrown out. And, as I said, that would be a pity.

> question to you, al, as a core member and tnf president: will it be
> as easy as it was to implement various security models on top of kauth
> now that its internals have been exposed?

As a software developer, my answer to your question would be "no - if
the complete abstraction has been violated, then it will be harder to
build models on top of kauth". Has the complete abstraction been violated,
or just a part of it? Where is the documentation dealing with the
abstractions, the ways it fits into other kernel code, and the direction
forward for kauth?

As President of TNF and a member of the core team, I would ask that
you consider that it might be more fun for you if you were a part of
the project.  You can't change anything if you're not part of it - the
best that you could hope for is to stand around on the sidelines and
(Continue reading)

Permalink | Reply | 0 comments |
headers
Thor Lancelot Simon | 23 Jun 2007 20:05

Re: CVS commit: src/sys

On Sat, Jun 23, 2007 at 06:37:20PM +0100, Alistair Crooks wrote:
> 
> As a software developer, my answer to your question would be "no - if
> the complete abstraction has been violated, then it will be harder to
> build models on top of kauth". Has the complete abstraction been violated,
> or just a part of it? Where is the documentation dealing with the
> abstractions, the ways it fits into other kernel code, and the direction
> forward for kauth?

The documentation is poor, but I think the design principle that's been
violated here is pretty obvious: don't expose kauth internals or security
model internals to other code in the kernel, because they will inevitably
abuse it.  Authentication data should only *ever* be handled via accessors.

We had that (albeit not in an ideally documented state) and changes like
the current one break it.  We should find a way to gain the performance
advantage of the current change without exposing knobs code outside kauth
has no business turning.

Thor
Permalink | Reply | 0 comments |
headers
Elad Efrat | 23 Jun 2007 20:15

Re: CVS commit: src/sys

Alistair Crooks wrote:
> On Sat, Jun 23, 2007 at 06:23:44PM +0300, Elad Efrat wrote:
>> I don't understand how kauth not being maintained means it's okay to
>> expose its internals.
> 
> If kauth is not being maintained, why are you bothering to copy the
> world and its dog on your email?
> 
> I know it's much easier to stand on the sidelines and shout whenever
> anyone touches a file with the letters "kauth" anywhere in its name,
> but it's not getting you or the project any further along the road.

what are you attacking here? me or the points I brought up? I think
the former. I cc "the world and its dog" because the commits in
question have the potential to affect the world and its dog. it is a
simple matter of responsibility.

> If you care about kauth, agree to the same rules that every other
> developer does, and maintain it. 

what rules do I not agree to? :)

> If you don't, it will bitrot, and
> will have to be thrown out. And, as I said, that would be a pity.

throw it out then. it's better to not ship it at all than to ship it
broken.

>> question to you, al, as a core member and tnf president: will it be
>> as easy as it was to implement various security models on top of kauth
(Continue reading)

Permalink | Reply | 0 comments |
headers
Elad Efrat | 23 Jun 2007 20:24

Re: CVS commit: src/sys

Thor Lancelot Simon wrote:
> On Sat, Jun 23, 2007 at 06:37:20PM +0100, Alistair Crooks wrote:
>> As a software developer, my answer to your question would be "no - if
>> the complete abstraction has been violated, then it will be harder to
>> build models on top of kauth". Has the complete abstraction been violated,
>> or just a part of it? Where is the documentation dealing with the
>> abstractions, the ways it fits into other kernel code, and the direction
>> forward for kauth?
> 
> The documentation is poor, but I think the design principle that's been
> violated here is pretty obvious: don't expose kauth internals or security
> model internals to other code in the kernel, because they will inevitably
> abuse it.  Authentication data should only *ever* be handled via accessors.
> 
> We had that (albeit not in an ideally documented state) and changes like
> the current one break it.  We should find a way to gain the performance
> advantage of the current change without exposing knobs code outside kauth
> has no business turning.
> 
> Thor

thor,

while I agree with what you're saying, I am very interested in hearing
what exactly is "poor" about kauth's documentation. this is the first
time I hear about it.

here is the kauth man page:

http://netbsd.gw.com/cgi-bin/man-cgi?kauth++NetBSD-current
(Continue reading)

Permalink | Reply | 0 comments |

Gmane