Jail and capability mode for shm_rename; add audit support for shm_rename

[matthew.bryan_isilon.com]

Summary

Co-mingling two things here:

1. Addressing some feedback from Konstantin and Kyle re: jail, capability mode, and a few other things
2. Adding audit support as promised.

The audit support change includes a partial refresh of OpenBSM from upstream, where my change to add shm_rename has already been accepted. I won't be working on refreshing anything else to support audit for those new event types.

Test Plan

Running the posixshm kyua tests, checking audit results manually. Example audit event:
header,96,11,shm_rename(2),0,Tue Oct 8 07:31:50 2019, + 580 msec
path,/t1
path,/t2
argument,2,0x1,flags
subject,root,root,0,root,0,778,755,0,0.0.0.0
return,success,0
trailer,96

[kib]

Please do not add generated files into the review. Also I think it is worth splitting the patch into fixes, and separate review for adding audit.

sys/kern/uipc_shm.c

433 ↗

(On Diff #63462)

No point in the assignment.

951 ↗

(On Diff #63462)

error != 0

1005 ↗

(On Diff #63462)

I do not see why this chunk is needed.

[matthew.bryan_isilon.com]

sys/kern/uipc_shm.c

1005 ↗

(On Diff #63462)

As I understand it: capability mode generally means that we would disallow messing with any global namespaces. Renaming an shm other than SHM_ANON would be messing with the SHM namespace. So: we quit early with ECAPMODE.

I'm sure you know better than I though. So: what is your though more specifically?

[kib]

sys/kern/uipc_shm.c

1005 ↗

(On Diff #63462)

I already answered this in my previous reply. The shm_rename(2) syscall is not listed in capabilities.conf, which means that in capability mode it is rejected right away by the infra code without ever entering into the syscall implementation routine. So it is simply impossible to have the test at line 1005 to ever return true.

[kevans]

sys/kern/uipc_shm.c

1005 ↗

(On Diff #63462)

shm_rename is also, unfortunately, not suitable for renames involving SHM_ANON mapping as either source or destination anyways, since SHM_ANON is a value that should not be suitable for copyinstr(9).

sys/sys/mman.h

328 ↗

(On Diff #63462)

The SHM_RENAME_* flags further up (outside of given context) should also be moved into the __BSD_VISIBLE namespace.

[matthew.bryan_isilon.com]

sys/kern/uipc_shm.c

1005 ↗

(On Diff #63462)

Yep; SHM_ANON is not allowed at all. There are even tests to make sure of it!

1005 ↗

(On Diff #63462)

Well...I was trying to plan for the future when somebody did add it but neglected to change this file :-p Engineering for quality. But I don't mind removing it.

sys/sys/mman.h

328 ↗

(On Diff #63462)

Good call; done.

[matthew.bryan_isilon.com]

CR fixes. Removed generated files. Leaving audit and other changes together (sorry).

[kib]

sys/kern/uipc_shm.c

450 ↗

(On Diff #63522)

It is too late to check for '/' at the start of name for jailed case. I understand that previous code did the same, but this is not a reason to keep it.

454 ↗

(On Diff #63522)

Extra blank line.

460 ↗

(On Diff #63522)

Extra blank line.

810 ↗

(On Diff #63522)

error != 0

1009 ↗

(On Diff #63522)

I see no point in use of strncmp.

1093 ↗

(On Diff #63522)

No point in the assingment.

1097 ↗

(On Diff #63522)

Extra blank line.

1099 ↗

(On Diff #63522)

No need to check for NULL (I think I already noted that).

1101 ↗

(On Diff #63522)

Same.

[matthew.bryan_isilon.com]

sys/kern/uipc_shm.c

450 ↗

(On Diff #63522)

Could you tell me what you are suggesting? In the jail case this is pointing to the char after the jail name to make sure there is a '/' there. Do we also need to check the first character? I don't know if pr_path always has a leading '/'. So are you suggesting this?

if (path[pr_pathlen] != '/' || path[0] != '/')

Or do you mean we shouldn't do this check at all in the jail case? Or only check the first character?

454 ↗

(On Diff #63522)

Here and elsewhere: I'm leaving the blank lines for readability.

810 ↗

(On Diff #63522)

Good catch. Fixed.

1009 ↗

(On Diff #63522)

I think you mean vs strcmp. I just learned that copyinstr makes sure these are null terminated. Thanks. :)

1093 ↗

(On Diff #63522)

It is needed for the ENOENT case above.

[matthew.bryan_isilon.com]

CR fixes

[kib]

Only stylistic or minor code organization issues are left in the patch, I do not see the need for you to wait for somebody to re-read this after fixing, so I am approving the change.

sys/kern/uipc_shm.c

437 ↗

(On Diff #63674)

We usually put the trailing binary (or ternary in this case) operator on the previous line. IOW, style prefers to keep ':' on the line 437.

450 ↗

(On Diff #63522)

I do not remember the previous version of your patch, but the current code looks fine. I mean that the user-supplied path must start with '/'.

Interesting but unrelated question is should we collapse multiple slashes into one.

454 ↗

(On Diff #63522)

Unneeded blank lines eat vertical space in editor.

Style only suggests empty lines in very specific cases, like split between local vars and code block, or before/after multiline-commented block.

1093 ↗

(On Diff #63522)

You mean, around line 1067. I suggest to reset error at that location, then.

1005 ↗

(On Diff #63462)

Hmm, it is not completely true. If mmap at zero is allowed (it could be), then (char *)1 is a valid pointer. I think we should check for SHM_ANON explicitly before calling copyinstr(9).

[matthew.bryan_isilon.com]

sys/kern/uipc_shm.c

437 ↗

(On Diff #63674)

Good to know. I'll put the ternary line on a single line, breaking after '=='.

1093 ↗

(On Diff #63522)

Great idea. Done.

1005 ↗

(On Diff #63462)

Great point! I never thought of that case. Explicit is better too. I'll check for path in or path out being SHM_ANON.