Page MenuHomeFreeBSD

Add audit(4) support to NFS(v3)
Needs ReviewPublic

Authored by shivank on Tue, Jul 28, 8:25 PM.

Details

Summary

Security event auditing permits the selective and fine-grained configurable logging of security-relevant system events for the purpose of post-mortem analysis, intrusion detection, and run-time monitoring and is intended to meet the requirements of the Common Criteria(CC)/Common Access protection profile(CAPP) evaluation. The audit subsystem in FreeBSD, audit(4), can record a variety of system events like user-logins, file system activities, network activities, process creations, etc.

The auditd(8) on the server doesn’t generate any record trails for the NFS activities as the audit works mostly on the syscall level and the NFS server is implemented within the kernel.

To audit NFS activities within the network, it will require to run the auditd(8) on each NFS client. This arrangement works perfectly fine in case of secure networks. But In the case of an insecure network, running auditd(8) on each client is not an option. The audit(4) support to the NFS server is a missing feature for such networks. Thus, the aim of this project is to audit each NFS RPC. This would allow audit of all NFS activities within the network by just running auditd(8) on the server.

Test Plan

A FreeBSD Port based on net/libnfs: https://github.com/shivankgarg98/NFSAuditTestSuite

mkdir /usr/local/tests/nfs-audit
make && make install
cd usr/local/tests/nfs-audit
kyua test -k Kyuafile

Note: Please make sure net/libnfs is already installed.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

shivank created this revision.Tue, Jul 28, 8:25 PM
shivank requested review of this revision.Tue, Jul 28, 8:25 PM
asomers added inline comments.Wed, Jul 29, 12:09 AM
sys/fs/nfsserver/nfs_nfsdkrpc.c
388

This code will only audit one of the two nd_nam fields. But both fields can be used at the same time. Can you audit both instead?

sys/security/audit/audit.c
476

As below, I suggest moving this line into audit_record_ctor

493

Instead of allocating an audit record for every RPC, could you instead store an audit record for every nfsd thread, and reuse the storage between RPCs? That would be more efficient.

498

I can't understand what you mean here. In any case, this must be cleaned up before merging.

508

Can you move this line to audit_nfsrecord_ctor? That would eliminate the possibility of creating a record and freeing it while the kaudit_record_type field is still invalid.

828

Is this lookup table valid for NFS protocols 2, 3, and 4?

sys/security/audit/audit_arg.c
1064

I recommend a KASSERT in the default position.

sys/security/audit/audit_bsm.c
1786

Users will want to see the file name. Can you log that, or at least log enough information that the user can reconstruct it?

gbe added a subscriber: gbe.Wed, Jul 29, 1:30 PM
shivank updated this revision to Diff 75138.Wed, Jul 29, 6:35 PM
shivank marked 6 inline comments as done.

follow up on changes suggested by asomers@

shivank added inline comments.Wed, Jul 29, 6:38 PM
sys/fs/nfsserver/nfs_nfsdkrpc.c
388
nd.nd_nam = svc_getrpccaller(rqst);
nd.nd_nam2 = rqst->rq_addr;

and,

#define svc_getrpccaller(rq)					\
	((rq)->rq_addr ? (rq)->rq_addr :			\
	    (struct sockaddr *) &(rq)->rq_xprt->xp_rtaddr)

So, when(TCP) nd_nam2 == NULL, nd_nam is client sockaddr. And, when(UDP) nd_nam2 != NULL, nd_nam and nd_nam2 are same. Therefore, Only one need to be audited.

sys/security/audit/audit.c
493

Can you please give some hints on how it can be done?

828

No, It's valid for v2 and v3 only. v4 is a little different in terms of nd_procnum, Hence, the table is not valid for NFSv4. Also, NFSv4 audit support is yet to be implemented.

sys/security/audit/audit_arg.c
1064

I use panic(9) since no expression to evaluate?

sys/security/audit/audit_bsm.c
1786

Not all RPCs have a filename in their arguments. How can I extract the filename rpc request in NFS server-side? for instance, some nfsrvd_read/write have filehandle as an argument for referencing the file.

asomers added inline comments.Wed, Jul 29, 7:19 PM
sys/fs/nfsserver/nfs_nfsdkrpc.c
388

Hm, it looks like nd_nam2 is really just a glorified boolean: it's either null or not null. So we could audit it that way. The comment says that it indicates whether the mount uses TCP. But from a security auditing perspective, who cares? I think you should just ignore it.

sys/security/audit/audit.c
493

I was hoping that there would already be some kind of struct describing each nfsd worker thread. But I can't find one. Do you know of any?

sys/security/audit/audit_arg.c
1064

Good. Now we'll get to find out if somebody has figured out how to mount NFS over bluetooth ;)

sys/security/audit/audit_bsm.c
1786

It's hard and not always possible. And we want to avoid doing too much work while auditing. That's why it would be acceptable merely to log enough information that a user can reconstruct the file name later. For example, if LOOKUP supplies a filename and returns some kind of opaque file handle, then READ supplies that same file handle, the user can deduce what was read as long as the audit trail contains the filename for LOOKUP and the file handle for both LOOKUP and READ.

In summary, locking and unlocking vnodes in this code is dangerous
and I am not in a position to make sure what you do is safe.

As noted before, the exception is before the operation call
is done. (ie. The code that calls nfsrvd_XXX() found in
sys/fs/nfsserver/nfs_nfsdsocket.c.

Doing calls there would also avoid needing to put them all
through nfs_nfsdserv.c, although I do not understand what
you are trying to do well enough to say if they are ever needed
in the nfsrvd_XXX() functions.

sys/fs/nfsserver/nfs_nfsdkrpc.c
388

Yep. If you really want the history behind this, it goes something like this...
A few years ago when this was written (about 1987), UDP passed the client
address up through soreceive() as an mbuf of type MT_SONAME, so nd_nam2
was an mbuf ptr.

For TCP (it was the first NFS over TCP implementation ever done as far as I know),
nd_nam was filled in by the kernel variant of getpeername() and was a struct sockaddr *.

Since the code used the client address for exports checking, the one in the mbuf
(nd_nam2) was copied to nd_nam to simplify this checking.

This was all long before the kernel RPC came along and nd_nam2 has just lived on.
(It is filled in from a slightly different place in the krpc code, but I don't believe it
can ever be different than nd_nam for UDP and is just NULL for TCP.)

sys/fs/nfsserver/nfs_nfsdserv.c
245

Yes. All of these functions check for the ESTALE error case at
the beginning of them (line#247 below for example).

It might be better to put this call after the error check.
(after line#248 below, for example)

1123

As noted in a separate discussion, unlocking and relocking of a directory
vnode needs to be very carefully evaluated.

For example, at this time, the path has already been put through a "lookup"
and the file object does not exist here.

However, unlocking/relocking allows another thread to create an object of
the same name (not necessarily the same object or type of object).

In general, I am opposed to unlocking/relocking vnodes in the code,
except at the very beginning of an RPC/operation.

1520

Same as above w.r.t. unlocking/relocking a directory vnode.

1616

Same as above...

sys/security/audit/audit_arg.c
1064

bz@ likes anything based on AF_INET or AF_INET6 to be #ifdef INET
or #ifdef INET6 even if it builds without that.
(It documents where the code fragments are and he dreams of getting
rid of INET someday, although I doubt that will be in my lifetime.)

1172

I'll admit I do not understand what this is trying to do.

I do not see any reason to vref()/vrele() the vnode, since
there must be a v_usecount held by the function where this
is called and that will not go away.

Also, VOP_ISLOCKED() returns the type of lock held.
The code needs to look something like:

locktype = VOP_ISLOCKED(vp);
/* As well as returning 0 for unlocked, it can return LK_EXCLOTHER for

another thread holding a lock on it. */

if (locktype != LK_EXCLUSIVE && locktype != LK_SHARED)

  • lock it

then if you locked it

VOP_UNLOCK(vp);

Then, if you do the vn_lock() without LK_NOWAIT as you have done now,
you must be careful to never call this when another vnode lock is held,
since ordering of vnode locking if you are to lock multiple vnodes must
be carefully designed to avoid deadlocks. (There are very few people in
the world, and I am not one of them, who actually understand how to safely
to lookup and rename of paths the intersect, as an example.)

The safe way to this, which I would be comfortable with, would be a

vn_lock(vp, LK_SHARE | LK_NOWAIT);
then if it fails you can't get all the audit information.

--> I'm guessing you would still want to generate an audit record, but it

won't be able to hold vnode details.
shivank marked 5 inline comments as done and 2 inline comments as done.Thu, Jul 30, 7:08 PM

Thanks for all suggestions. I have incorporated them into my code. There is just a directory vnode unlocking/relocking issue not done yet.

sys/fs/nfsserver/nfs_nfsdserv.c
1123

There is a problem with calling AUDIT_NFSARG_UPATH1_VP at the beginning (as you suggested to do it in nfsd_fhtovp in the mail) of the RPC operation: I don't have the pathname and rootdir info at that point, since the RPC extracts those data in nfsrvd_*.
I think I should drop the unlock/relock idea and define two separate path for the locked and unlocked vnode case.
I earlier added if (VOP_ISLOCKED(vp)) checks in vn_fullpath_any in vfs_cache.c, but that also added complications and bugs in code. Splitting it into the separate would simplify it.

sys/security/audit/audit.c
493

Can you please suggest something here, rmacklem@?

sys/security/audit/audit_arg.c
1064

I did the #ifdef INET... #endif part. But something is wrong somewhere as it skips the block and jumps to the default case. (pointing the INET/INET6 is not defined)
Am I missing some headers file?

1172

if I don't use vref/vrele the kernel crashes - panic: condition vp->v_holdcnt > 0 not met at /usr/home/shivank/freebsd/sys/kern/vfs_vnops.c:1700 (_vn_lock)

I did the suggested changes for locking.

sys/security/audit/audit_bsm.c
1786

Can vattr.va_fileid from the vnode attr token be used to reconstruct the file name later? In that case, filehandle info would not be needed.
I checked manually for some events and I think it should work. What do you think?

shivank updated this revision to Diff 75175.Thu, Jul 30, 7:17 PM
shivank marked an inline comment as done.
shivank updated this revision to Diff 75383.Tue, Aug 4, 6:09 PM
shivank marked an inline comment as done and 5 inline comments as not done.

removing unlocking/relocking implementation for vnode for auditing path, instead, define separate functions in vfs_cache.c for locked vnode as argument.