Re: [Audit-test-developer] [design] How to write / run / audit syscall wrappers?

[Jiri J. <jja...@re...>]

On 08/04/2014 09:26 PM, Linda Knippers wrote:
> On 7/29/2014 7:22 AM, Jiri Jaburek wrote:
>> On 07/28/2014 06:39 PM, Linda Knippers wrote:
>>> Hi Jiri,
>>>
>>> On 07/28/2014 09:33 AM, Jiri Jaburek wrote:
>>>> Hello Linda & others,
>>>>
>>>> I've been doing some syscall work recently and while doing it,
>>>> I decided to "clean up" the do_ipc and do_socketcall wrappers, which
>>>> seemed like a duplicated functionality, since they call exactly the same
>>>> library functions as normal do_* wrappers. Also, I really wanted to get
>>>> rid of the ipc headerhack. :)
>>>
>>> That's a good goal.:-)
>>>
>>>> This was somewhat amplified by the fact that the code in ipc_common.c
>>>> was over-shared, meaning that ie. semget was using flags for semctl
>>>> or semop. So I made a series of ~6 commits, carefully moving all the
>>>> functionality from ipc_common.c into separate wrappers and removing the
>>>> do_ipc wrapper. I did the same for do_socketcall, which just calls bind,
>>>> using a library function, like do_bind. All this with removing
>>>> respective sections from syscalls/*.conf, of course.
>>>>
>>>> I was quite happy with the series, since - functionality wise - it was
>>>> transparent. However when I tested it on MODE=32, the syscalls bucket
>>>> started throwing ERRORs. 
>>>
>>> Right.  The 32-bit x86 syscalls add a lot of complexity.
>>> Did you see any problems on non-x86 architectures?
>>
>> I didn't test other architectures, but the code suggests that all 32bit
>> variants are affected, for ipc(2) at least.
>>
>>>
>>>> Some investigation uncovered that the syscalls
>>>> bucket was actually using these "duplicated" wrappers for proper
>>>> auditing - because auditctl works with real syscalls, not libc
>>>> functions. The extra wrappers were therefore nothing more than a name,
>>>> simplifying logic in the syscalls bucket.
>>>>
>>>> This goes against some other approaches used in the suite - in the
>>>> network bucket, for example, which - based on the architecture - selects
>>>> proper syscall name for auditctl, while still calling the original
>>>> syscall wrapper (which uses library functions).
>>>>
>>>> -----------------------------------------------------------------------
>>>>
>>>> This led me into a certain design question I'd like to ask here; how to
>>>> design syscall wrappers and the execution and auditing infrastructure
>>>> around them? What would be the best approach?
>>>>
>>>> I've identified 3 most obvious ways to write a syscall wrapper:
>>>>
>>>>   A) use syscall(__NR_syscallname, ...) directly, bypassing libc
>>>>   B) use libc functions
>>>>   C) use (A), but simulate libc using #ifdefs manually
>>>
>>> Today we use both A) and B), depending on the syscall.  B) is easiest
>>> from a coding perspective.  A) is sometimes necessary because libc
>>> might not actually be using the syscall we want in the mode we want
>>> or may be doing error checking of it's own that prevent some of the case
>>> we want to test.  I'm not sure I understand C).
>>
>> (C) for do_chmod could look like
>>
>>   #ifdef ARM
>>     exitval = syscall(__NR_fchmodat, ...);
>>   #else
>>     exitval = syscall(__NR_chmod, ...);
>>   #endif
>>
>> essentially simulating glibc in a controlled manner.
> 
> I thought that was A).

(A) was using just one in each wrapper and doing conditional build
based on an external syscall-to-arch mapping file (or autodetection).

> 
> In this example, I'm not sure you'd have an ifdef in do_chmod to map
> it to fchmodat on ARM because do_chmod is to test chmod, and there isn't
> one for arm.  You'd just use do_fchmodat, which exists for other arches too.

Probably a bad example, fstatat / newfstatat would be better here, ie.
something already done by glibc.

> 
>>>
>>>> These approaches solve the "which syscalls to run where" problem
>>>> somewhat differently and therefore have different benefits and
>>>> drawbacks in various situations:
>>>>
>>>>   1) compile time
>>>>
>>>>     (A) is a bit problematic, since we would need to come up with a full
>>>>     logic of what syscalls to compile on what architectures. We already
>>>>     do that on some level, but this option calls for a separate mapping
>>>>     file (or so) instead of simple Makefile-based conditions,
>>>>     integrating it somehow into the make system.
>>>
>>> Right - we seem to be reinventing libc.
>>
>> Please note that (A) isn't about reinventing libc - it isn't about
>> providing abstractions for syscalls, since it's these abstractions that
>> later cause "hacky" auditing code in tests.
> 
> Ok, but using the example from above, what would the audit test test for?
> You can't test a chmod syscall on ARM if there isn't one.  The auditctl
> to select that syscall for auditing would fail too, right?

The (A) "way" would be to test both fstatat and newfstatat by calling
a bash function, which checks whether a given syscall is "relevant" to
the current architecture and compile/execute/audit it if it is.

The (B) "way" would be to always compile and execute whatever glibc
hides under fstatat() and selectively audit (guess) the syscall behind
it.

The (C) "way" would be the same as (B), except that we wouldn't have to
guess, we would explicitly say which syscall to execute/audit where.

Of all three options, only (A) allows us to test multiple syscalls
potentially hidden under a single glibc function. However we might as
well simply use __NR_name in these special cases, like we do with
fork/clone, for example. If these syscalls are arch-specific, this might
become more ugly on the (bash) test side and the Makefile side.

>>
>>>
>>>>     (B) and (C) are really easy - the libc (or custom #ifdefs) take care
>>>>     of which syscall should be called on which architecture. (C) has
>>>>     the disadvantage of actually doing the mapping from (A), just in
>>>>     a less visible way.
>>>>
>>>>   2) run (execution) time
>>>>
>>>>     (A) again needs to re-use the mapping file (or logic) in most cases,
>>>>     since - if we want to call ie. open-like syscall, we need to call
>>>>     explicitly either do_open (non-arm) or do_openat (arm)
>>>
>>> And in some cases, we need to do both when the arch supports both.
>>>
>>>>     (B) and (C) simplify this case a lot, but may fall short if we
>>>>     *really* want to call openat instead of open on non-arm
>>>
>>> We do that today.  We test both open and openat using the do_* programs.
>>>
>>>>   3) auditing time
>>>>
>>>>    (A) shines here, IMO, since the auditctl mapping is 1:1 to the do_*
>>>>    wrappers. It still needs to re-use the mapping file (and therefore
>>>>    needs to manually specify which syscalls to audit on which archs),
>>>>    but it's very straightforward and clear.
>>>>
>>>>    (B) and (C) have a significant problem here - we don't know which
>>>>    syscalls are being called "under the hood" of the do_* wrappers, 
>>>
>>> Well, we do for all the existing calls.
>>
>> *We* do, but the tests don't, which is why we have to create ad-hoc
>> conditions to tell them.
>>
>>>
>>>>    so
>>>>    we need to try them out and then create per-arch hacks in the code
>>>>    similar to what we've seen in the arm patchset recently, ie.
>>>>    "set up auditing for fchmodat and call do_chmod", which can be
>>>>    somewhat confusing. The other option is to duplicate wrappers like
>>>>    do_socketcall, but the per-arch hacks still persist. We could create
>>>>    a mapping file for them, but then we might as well use (A).
>>>>
>>>>
>>>> So what would be the best approach for new (and existing, over time)
>>>> syscall wrappers?
>>>>
>>>> I personally really like (A) due to its clean design - there are no
>>>> "Note: There is no glibc wrapper for this system call" exceptions
>>>> and it's clear what syscalls run on which architectures and 32/64bit
>>>> variations. The mapping file, with its helper bash functions doing ie.
>>>> "is this syscall relevant for current arch/mode?" or "list all relevant
>>>> syscalls for current arch/mode", along with some documentation, should
>>>> be mostly easy to implement.
>>>> A practical example in the syscalls bucket would be checking for arch
>>>> relevancy in the `+' function with no per-arch or per-mode conditions
>>>> in the .conf files. The rollup log (or run.bash --list) would then show
>>>> which syscalls were actually run.
>>>
>>> I don't think there is one "best approach".  Some of what we have today
>>> is because the suite has evolved over time but some if it is because for
>>> the most part, using the libc functions is simple and appropriate.
>>> Where's it's not simple or appropriate, we drop back to the the syscall(_NR...)
>>> function.  Sometimes due to legacy syscalls, like the multiplexed 32-bit
>>> syscalls, it gets a bit complicated but hey, we've written that code already. :-)
>>
>> I agree that using libc is often better and it would be perhaps cleaner
>> if auditctl supported syscall translation (ie. chmod->fchmodat on arm),
>> but it doesn't, it needs a direct syscall name for a given arch / mode.
> 
> Maybe fixing auditctl is the better solution.  If we don't know what
> the syscalls are, how are audit users supposed to know?

Even if we were able to make auditctl use glibc wrapper names instead
of syscalls, the kernel-based audit log won't care for them and will
always use the syscalls themselves (numbers). Though I guess the
translation layer could be used for ausearch as well.

> 
>> My points for (A) go mostly towards the auditing code, see the current
>> state of augrok_default and auwatch_default in network/run.conf.
>> Or the per-arch conditions in syscalls/*-run.conf.
> 
> The multiplexed system calls are a pain, but it's mostly legacy pain.
> Are there new syscalls that are implemented that way?

Not that I'm aware of, but the legacy pain didn't end, the PPC arch now
(since 2010, 86250b9d12caa1a3dee12a7cf638b7dd70eaadb6, in RHEL7)
supports the "normal" network syscalls *in addition* to the socketcall
syscall. On both 64 *and* 32bits.
Not sure which of them is used by glibc, but our suite expects
socketcall.

Also, there's no technical reason why 64bit versions of PPC/S390
couldn't support the "normal" ipc syscalls instead of just ipc(2),
meaning somebody will probably add those in the future.

>>
>> With a unified way of telling which syscalls are relevant for which
>> architectures, the conditions in syscalls/ could go away. The entire
>> case/esac structure in network/ could as well, since $syscall would
>> always be the tested syscall and no other.
> 
> I'm not sure how unified it can really be though.  In the case of
> these multiplexed system calls, we still have to test the various
> options to the syscalls because they go down different security
> relevant paths.  I don't think we could have just one test for
> socketcall(), for example.

It's true that different variants of the same syscall could cause
problems since we're checking for arguments (a0, ..) of the syscalls
when searching the audit log. If some arch decided to move the ipc
operation type in ipc(2) from a0 to a1, it would be a big problem for
generic testing as done by (A).
I'm not currently sure how to deal with this in a reasonably simple
manner.

> 
>> The "unified way" doesn't have to be a static file, it can be generated
>> using gcc from unistd.h dynamically.
>>
>> Also, "we've written that code already" doesn't mean there won't be more
>> of it in the future (well, near future).
>>
>>>
>>>> This also means that I would have to throw away most of my series on
>>>> do_ipc and do_socketcall, possibly re-implementing them in the future,
>>>> but I'm fine with that.
>>>>
>>>> What's your opinion?
>>>
>>> If it ain't broke....?
>>
>> Right, I was wondering whether a "clean" solution wouldn't be an
>> overkill, since there aren't that much differences aside from do_ipc
>> and do_socketcall. For me, it's just another cleanup, like the netfilter
>> or networking-related code.
>>
>>>
>>>> Thanks,
>>>> Jiri
>>>>
>>>>
>>>> PS: I'm asking because we currently have ~70 new syscall wrappers staged
>>>>     for review and there's not much consistency in terms of (A) vs (B)
>>>>     vs (C).
>>>
>>> I'm interesting in knowing more about your new syscalls and why you have
>>> a mix of A), B) and C) (which I still don't understand but I'm sure a simple
>>> example would clear things up).  Is it because 3 different people wrote them
>>> or because there really is no "best approach"?
>>
>> Various reasons, really. Written mostly by a single person, (A) was used
>> where using (B) would pull in another suite dependency (NUMA),
>> (B) was used where (A) would result in two independent wrappers (fstatat
>> vs newfstatat), and various mix of both. Yes, because there's no defined
>> policy of "best approach".
> 
> If fstatat and newfstatat are both syscalls that are both callable by an
> architecture, then you might need two since you'd have to test both.

It seems that 64bit variants of all supported architectures use
newfstatat and all 32bit variants use fstatat64, there's no overlap.
There is "sys_fstatat" defined in the kernel, but it doesn't seem to be
in unistd.h, so it's probably not exported as a syscall.

> 
>>> I'm not trying to say that we have to keep doing things the way we've always done it.
>>> This suite has clearly evolved and improved as you and others have worked on it so I'd
>>> like to continue this discussion.
>>
>> Sure, I'd like to get as much feedback as possible before implementing
>> anything (or deciding to leave it be in the current state).
> 
> I see you've sent another message now that I haven't read yet but
> I'll read next.
> 
> -- ljk
>>
>>>
>>> -- ljk
>>>
>>>>
>>>> ------------------------------------------------------------------------------
>>>> Infragistics Professional
>>>> Build stunning WinForms apps today!
>>>> Reboot your WinForms applications with our WinForms controls. 
>>>> Build a bridge from your legacy apps to the future.
>>>> http://pubads.g.doubleclick.net/gampad/clk?id=153845071&iu=/4140/ostg.clktrk
>>>> _______________________________________________
>>>> Audit-test-developer mailing list
>>>> Aud...@li...
>>>> https://lists.sourceforge.net/lists/listinfo/audit-test-developer
>>>
>>>
>>> ------------------------------------------------------------------------------
>>> Infragistics Professional
>>> Build stunning WinForms apps today!
>>> Reboot your WinForms applications with our WinForms controls. 
>>> Build a bridge from your legacy apps to the future.
>>> http://pubads.g.doubleclick.net/gampad/clk?id=153845071&iu=/4140/ostg.clktrk
>>> _______________________________________________
>>> Audit-test-developer mailing list
>>> Aud...@li...
>>> https://lists.sourceforge.net/lists/listinfo/audit-test-developer
>>>
>>
>> Jiri
>>
> 
> 
> ------------------------------------------------------------------------------
> Infragistics Professional
> Build stunning WinForms apps today!
> Reboot your WinForms applications with our WinForms controls. 
> Build a bridge from your legacy apps to the future.
> http://pubads.g.doubleclick.net/gampad/clk?id=153845071&iu=/4140/ostg.clktrk
> _______________________________________________
> Audit-test-developer mailing list
> Aud...@li...
> https://lists.sourceforge.net/lists/listinfo/audit-test-developer
>