I am not convinced, changing errno like this is gratuitous. We actually
do do it elsewhere, but IMO that is unnecessary too.
On Tue, Oct 06, 2015 at 08:39:02AM +0200, Benny Lofgren wrote:
> Hi Nicholas,
>
> Thanks for taking the time to review this. I'll try to elaborate on my
> reasoning below. Please excuse my wordiness, I'm just not genetically
> capable of being brief...
>
>
> On 2015-10-06 00:05, Nicholas Marriott wrote:
> > I think it should just return whatever copyinstr does and not go
> > swapping around error numbers, we don't do that anywhere else.
>
> Well, maybe that's because everywhere else that usage of copyinstr()
> makes sense? :-)
>
>
> Think about it; when I first looked at tame() a few months ago, request
> was a bit field. That felt like the most natural way to pass along a set
> of flag parameters to me and I didn't raise an eyebrow to it.
>
> I don't know why those semantics changed, but I'm sure there's a reason
> behind it. I was looking through tech@ and misc@ for a discussion, and
> couldn't find any mention of it so I can only speculate that it might be
> in preparation for some future feature.
>
>
> However, I can't think of a single other system call that takes what (at
> least for now) essentially amounts to a set of binary mode flags, but
> does it through passing them in text form, lined up and space separated
> in a string that is sent as-is to the kernel.
>
> In almost every other case a call passes a string into kernel space it
> is a path name or a similar type of parameter, not the actual "command"
> controling what specific action to take.
>
> You could perhaps argue that fopen() and friends does that with the mode
> parameter, but they are not system calls. And because they are library
> functions the string parsing is done in user space, and it is also a
> much simpler format to parse than that in the current implentation of
> tame().
>
>
> This is actually the reason I first sat down to try to provoke tame() by
> sending it various malformed parameter strings, invalid pointers, too
> long strings, sneakily crafted paths arrays and strings and so on.
>
> If there is an obvious place where there is a risk of introducing a
> vulnerability in a system call, this kind of parameter parsing taking
> place in kernel space would be a prime candidate. It does seem robust
> enough though - and I expect nothing less from carefully written code by
> people who knows what they're doing - but we're all human and humans
> makes mistakes, so I figured an extra set of eyes on the code couldn't
> hurt...
>
>
> And the one thing I came up with so far was this inconsistency in
> returned errno.
>
> It seems a logical and easy enough thing to fix in my mind.
>
> Also, being stringent about keeping errno return values consistent
> lessens the ambiguity for a programmer debugging why his call returns
> this or that errno.
>
>
> I am so far really happy with the idea and implementation of tame(),
> except for this one detail of how the parameter passing is made, which
> concerns me a little. It seems (again, without knowledge of future
> feature set ideas) just like an overly complex solution to what would be
> a straight forward, no-nonsense passing of a bit field. Other than that,
> the concept is just brilliant!
>
>
> As usual, these are only my two cents' worth. :-)
>
> I'm not arguing for the sake of argument but because I see an
> inconsistency compared to how things are usually implemented "unix
> style", and now seem to be the time and place to chip in with ideas,
> before the API settles down and becomes harder to change.
>
>
> Thanks again for everyone's work in the project. tame(), in my mind,
> really has great potential as yet another practically useful (and
> therefore likely to BE used) mitigation layer, protecting us programmers
> against our own mistakes. :-)
>
>
> Regards,
>
> /Benny
>
>
> >
> > On Mon, Oct 05, 2015 at 11:15:33PM +0200, Benny Lofgren wrote:
> >> Hi guys,
> >>
> >> I was playing around with tame() today, and have a couple of minor
> >> suggestions:
> >>
> >>
> >> - Return EINVAL instead of ENAMETOOLONG if the request argument string
> >> is too long. ENAMETOOLONG translates to "File name too long", which I
> >> think is misleading. Maybe E2BIG would be an alternative, but EINVAL
> >> feels better to me since it is clearly an invalid argument being passed.
> >> ENAMETOOLONG is still appropriate for the paths argument of course.
> >>
> >> - Add same [EINVAL] condition to the ERRORS section in tame.2.
> >>
> >> - While there, elaborate a bit on [EFAULT] since there are more error
> >> conditions that can generate that.
> >>
> >>
> >> Regards,
> >>
> >> /Benny
> >>
> >>
> >>
> >> Index: lib/libc/sys/tame.2
> >> ===================================================================
> >> RCS file: /cvs/src/lib/libc/sys/tame.2,v
> >> retrieving revision 1.31
> >> diff -u -p -r1.31 tame.2
> >> --- lib/libc/sys/tame.2 4 Oct 2015 20:47:16 -0000 1.31
> >> +++ lib/libc/sys/tame.2 5 Oct 2015 21:06:06 -0000
> >> @@ -424,6 +424,8 @@ will fail if:
> >> .Bl -tag -width Er
> >> .It Bq Er EFAULT
> >> .Fa paths
> >> +or one of its elements, or
> >> +.Fa request
> >> points outside the process's allocated address space.
> >> .It Bq Er ENAMETOOLONG
> >> An element of
> >> @@ -435,6 +437,9 @@ to it would exceed
> >> bytes.
> >> .It Bq Er EPERM
> >> This process is attempting to increase permissions.
> >> +.It Bq Er EINVAL
> >> +.Ar request
> >> +is malformed or too long.
> >> .It Bq Er E2BIG
> >> The
> >> .Ar paths
> >> Index: sys/kern/kern_tame.c
> >> ===================================================================
> >> RCS file: /cvs/src/sys/kern/kern_tame.c,v
> >> retrieving revision 1.57
> >> diff -u -p -r1.57 kern_tame.c
> >> --- sys/kern/kern_tame.c 4 Oct 2015 17:55:21 -0000 1.57
> >> +++ sys/kern/kern_tame.c 5 Oct 2015 21:06:13 -0000
> >> @@ -255,7 +255,7 @@ sys_tame(struct proc *p, void *v, regist
> >> &rbuflen);
> >> if (error) {
> >> free(rbuf, M_TEMP, MAXPATHLEN);
> >> - return (error);
> >> + return ((error == ENAMETOOLONG) ? EINVAL : error);
> >> }
> >> #ifdef KTRACE
> >> if (KTRPOINT(p, KTR_STRUCT))
> >>
> >
>
> --
> internetlabbet.se / work: +46 8 551 124 80 / "Words must
> Benny Lofgren / mobile: +46 70 718 11 90 / be weighed,
> / fax: +46 8 551 124 89 / not counted."
> / email: benny -at- internetlabbet.se
