Page MenuHomeFreeBSD

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

Authored by matthew.bryan_isilon.com on Oct 18 2019, 9:44 PM.

Details

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

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

kib added a comment.Oct 19 2019, 3:11 AM

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 marked 2 inline comments as done.Oct 20 2019, 4:16 AM
matthew.bryan_isilon.com added inline comments.
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 added inline comments.Oct 20 2019, 1:28 PM
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 added inline comments.Oct 20 2019, 11:15 PM
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 marked 4 inline comments as done.Oct 21 2019, 7:33 PM
matthew.bryan_isilon.com added inline comments.
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 marked 3 inline comments as done.

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

kib added inline comments.Oct 24 2019, 3:43 PM
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 marked 9 inline comments as done.Oct 25 2019, 8:11 PM
matthew.bryan_isilon.com added inline comments.
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 marked 5 inline comments as done.

CR fixes

kib accepted this revision.Oct 29 2019, 6:42 PM

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
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).

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.

This revision is now accepted and ready to land.Oct 29 2019, 6:42 PM
matthew.bryan_isilon.com marked 3 inline comments as done.Oct 30 2019, 12:25 AM
matthew.bryan_isilon.com added inline comments.
sys/kern/uipc_shm.c
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.

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.