Add audit(4) support to NFS(v3)

[shivank]

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.

[asomers]

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

474

As below, I suggest moving this line into audit_record_ctor

491

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.

496

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

506

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.

837

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?

[shivank]

follow up on changes suggested by asomers@

[shivank]

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

491

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

837

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]

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

491

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.

[rmacklem]

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

246

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)

1134

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.

1538

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

1644

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]

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

1134

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

491

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]

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

[asomers]

This is a much better locking strategy. However, there's a lot of duplicated code. Could you maybe combine the _locked with the original functions, so there wouldn't be so much duplication?

sys/kern/vfs_cache.c

460 ↗

(On Diff #75383)

The naming convention for functions like this is to put the _locked at the end, like vn_fullpath_any_locked

2402 ↗

(On Diff #75383)

What would you do differently if LK_EXCLOTHER were set?

2661 ↗

(On Diff #75383)

There's a lot of duplicated code here. Could you combine it with vn_fullpath_dir into a single function that takes an optionally-locked vnode?

2860 ↗

(On Diff #75383)

Is it really possible to reach this line? Shouldn't lktype always be non-zero in this function?

sys/sys/vnode.h

656 ↗

(On Diff #75383)

Since vn_locked_vptocnp is only used in a single location, you can make it a static function, and not declare it in the header.

[shivank]

follow-up on suggested changes.

[shivank]

Regarding code duplication in vn_fullpath_dir_locked:
I modified vn_fullpath_dir(and removed vn_fullpath_dir_locked) for optionally locked vnode here in git commit: https://github.com/shivankgarg98/freebsd/commit/418c1c2a6de9989fe7a541f6111ee2c3f2786c7b
It works fine NFSv4=3 case but somehow breaks nfsrvd_open to result in an error.{and hence can't open/create a regular file from client}.
Using two completely separate functions reduces the scope of error. Also prevent any mutation to the current code path for not locked vnodes, while allowing it to work for locked vnodes.

sys/kern/vfs_cache.c

2402 ↗

(On Diff #75383)

I request Rick to please guide on this. In one of his other comment, he wrote "/* 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". I didn't completely understand it, So wrote a comment here.

2860 ↗

(On Diff #75383)

I observed the value of the vnode vp pointer can be changed by the vn_vptocnp function call. That's why I check VOP_ISLOCKED whenever such an assignment can happen.
Please correct me if I have any misunderstanding of this concept.

[shivank]

• updated sys/kern/vfs_cache.c to reduce code duplication with vn_fullpath_dir
• some trivial changes

[asomers]

Using two completely separate functions reduces the scope of error. Also prevent any mutation to the current code path for not locked vnodes, while allowing it to work for locked vnodes.

Your motivations are reasonable, but they're short-term thinking. In the long term, maintaining two separate but nearly identical code paths is very error-prone. Better to take the time up front to merge them correctly.

It looks like your most recent change rebased the base revision. That makes it very hard to see which changes are from you and which aren't. Could you please either un-rebase it or, if that's not possible, open a new review?

sys/fs/nfsserver/nfs_nfsdkrpc.c

386

What's different about NFSv4 here?

[shivank]

It looks like your most recent change rebased the base revision. That makes it very hard to see which changes are from you and which aren't. Could you please either un-rebase it or, if that's not possible, open a new review?

Ohh, Sorry! I didn't thought pulling HEAD changes will create this side-effect in revision
I think I would open a new review as going back will have conflicting changes again. Should I abandon this while creating a new one??

sys/fs/nfsserver/nfs_nfsdkrpc.c

386

NFSv4 audit support is in separate feature branch here. Due to NFSv4's slightly different nature ( 1 compound RPC and numbers of suboperations in it ), I audit each of its sub-operation in nfsrvd_compound() by adding AUDIT_NFSRPC_ENTER/EXIT like this.

[asomers]

In D25869#583083, @shivank wrote:

It looks like your most recent change rebased the base revision. That makes it very hard to see which changes are from you and which aren't. Could you please either un-rebase it or, if that's not possible, open a new review?

Ohh, Sorry! I didn't thought pulling HEAD changes will create this side-effect in revision
I think I would open a new review as going back will have conflicting changes again. Should I abandon this while creating a new one??

Sure. Go ahead and create a new review.

sys/fs/nfsserver/nfs_nfsdkrpc.c

386

I get it. NFSv4 goes through this code path, but it's pointless to audit arguments for the compound RPC. You'll audit the operations instead.

[shivank]

I created a new review - D26243. Sorry for the trouble.