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
