Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mac_veriexec : extend veriexec to control deletion (unlink) and move (rename_from and rename_to) on protected files #613

Closed

Conversation

darklem
Copy link

@darklem darklem commented Sep 5, 2022

This patch intend to elevate the level of protection provided by the veriexec module on files by including new syscall hooks. This patch is inspired by NetBSD veriexec.

Functions implemented :

  • mac_veriexec_vnode_check_unlink: Unlink on a file has been requested and requires validation. This function prohibits the deleting a protected file (or deleting one of these hard links, if any).
  • mac_veriexec_vnode_check_rename_from: Rename the file has been requested and must be validated. This function controls the renaming of protected file
  • mac_veriexec_vnode_check_rename_to: File overwrite rename has been requested and must be validated. This function prevent overwriting of a file protected (overwriting by mv command).

The 3 fonctions together aim to control the 'removal' (via unlink) and the 'mv' on files protected by veriexec. The intention is to reach the functional level of NetBSD veriexec.

…veriexec module on files by including new syscall hooks. This patch is inspired by NetBSD veriexec.

Functions implemented :

- mac\_veriexec\_vnode\_check\_unlink: Unlink on a file has been requested and requires validation. This function prohibits the deleting a protected file (or deleting one of these hard links, if any).
- mac\_veriexec\_vnode\_check\_rename\_from: Rename the file has been requested and must be validated. This function controls the renaming of protected file
- mac\_veriexec\_vnode\_check\_rename\_to: File overwrite rename has been requested and must be validated. This function prevent overwriting of a file protected (overwriting by mv command).

The 3 fonctions together aim to control the 'removal' (via unlink) and the 'mv' on files protected by veriexec. The intention is to reach the functional level of NetBSD veriexec.
@darklem darklem changed the title This patch intend to elevate the level of protection provided by the … mac_veriexec : extend veriexec to control deletion (unlink) and move (rename_from and rename_to) on protected files Sep 5, 2022
@darklem
Copy link
Author

darklem commented Feb 26, 2023

Anything I should change/adapt to make it useful for FreeBSD ?

@bsdimp
Copy link
Member

bsdimp commented Feb 26, 2023

On the surface, this looks good to my eye.

I don't think the normal veriexec folks are on github that often, so I've sent an email off to them to get their attention. I don't know their github handles, or I'd @ them here. I should have done this when it came in, please accept my apologies for missing it at the time.

@sgerraty
Copy link
Contributor

Blocking rename looks fine, but blocking unlink would break our package system.
Perhaps a sysctl to control that?

@bsdimp
Copy link
Member

bsdimp commented Mar 4, 2023

Anything I should change/adapt to make it useful for FreeBSD ?

Have you seen @sgerraty's suggestions?

@darklem
Copy link
Author

darklem commented Mar 4, 2023

On the surface, this looks good to my eye.

I don't think the normal veriexec folks are on github that often, so I've sent an email off to them to get their attention. I don't know their github handles, or I'd @ them here. I should have done this when it came in, please accept my apologies for missing it at the time.

Sorry for the delay ! I just read this answer. Thank you!

@darklem
Copy link
Author

darklem commented Mar 4, 2023

Blocking rename looks fine, but blocking unlink would break our package system.

Perhaps a sysctl to control that?

Thank you Simon. I will make a change and propose it asap.

@bsdimp
Copy link
Member

bsdimp commented Mar 7, 2023

@sgerraty in your court: I think the changes look OK, but I'm no expert.

@sgerraty
Copy link
Contributor

sgerraty commented Mar 7, 2023 via email

@darklem
Copy link
Author

darklem commented Mar 9, 2023

@sgerraty, I add a sysctl to toggle the unlink optionally. Tell me if you think I should adapt or change something.
Thank you for the review

@sgerraty
Copy link
Contributor

sgerraty commented Mar 9, 2023 via email

…gle unlink protection. Add the corresponding read-only sysctl
@darklem
Copy link
Author

darklem commented Mar 11, 2023

Hi @sgerraty, I made the modifications based on your last suggestion. I hope it will be fine.
Thank you again for the review.

Copy link
Contributor

@sgerraty sgerraty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks promising.
Thanks very much!

@darklem
Copy link
Author

darklem commented Mar 13, 2023

Thank you @sgerraty and @bsdimp for your time

freebsd-git pushed a commit that referenced this pull request Mar 14, 2023
Functions implemented :

- mac_veriexec_vnode_check_unlink: Unlink on a file has been
  requested and requires validation. This function prohibits the
  deleting a protected file (or deleting one of these hard links, if
  any).
- mac_veriexec_vnode_check_rename_from: Rename the file has been
  requested and must be validated. This function controls the renaming
  of protected file
- mac_veriexec_vnode_check_rename_to: File overwrite rename has been
  requested and must be validated. This function prevent overwriting of
  a file protected (overwriting by mv command).

The 3 fonctions together aim to control the 'removal' (via unlink) and
the 'mv' on files protected by veriexec. The intention is to reach the
functional level of NetBSD veriexec.

Add sysctl node security.mac.veriexec.unlink to toggle control on
syscall unlink.

Add tunable kernel variable security.mac.veriexec.block_unlink to toggle
unlink protection. Add the corresponding read-only sysctl.

[ tidied up commit message, trailing whitespace, long lines, { placement ]

Reviewed by: sjg, imp
Pull Request: #613
@bsdimp
Copy link
Member

bsdimp commented Mar 14, 2023

Landed in the tree. Had to do a number of style cleanups, mostly brace placements and > 80 columns (for future reference). If we'd not let this linger, or had a better style checking script, I'd have done one more round to get them fixed.

@bsdimp bsdimp closed this Mar 14, 2023
@darklem
Copy link
Author

darklem commented Mar 14, 2023

@bsdimp, thank you ! Would it be possible please to link the contribution to my github user ( @darklem ) ? It seems to be linked to my login 'dl'. Thank you.

@bsdimp
Copy link
Member

bsdimp commented Mar 14, 2023

@bsdimp, thank you ! Would it be possible please to link the contribution to my github user ( @darklem ) ? It seems to be linked to my login 'dl'. Thank you.

I'm not sure how to do that after the fact. All we have in our repo is the email address in the commit message submitted here. If there is a way we can do that w/o a forced push, I'm happy to do so.

@darklem
Copy link
Author

darklem commented Mar 14, 2023

@bsdimp, thank you ! Would it be possible please to link the contribution to my github user ( @darklem ) ? It seems to be linked to my login 'dl'. Thank you.

I'm not sure how to do that after the fact. All we have in our repo is the email address in the commit message submitted here. If there is a way we can do that w/o a forced push, I'm happy to do so.

I linked a second email to my account. It works now. Thank you :)

@emaste
Copy link
Member

emaste commented Mar 14, 2023

We can add a .mailmap file mapping if you provide the appropriate entry.

Your Display Name <new_email@example.com> <old_email_in_git@example.com>

@darklem
Copy link
Author

darklem commented Mar 14, 2023

We can add a .mailmap file mapping if you provide the appropriate entry.


Your Display Name <new_email@example.com>

Thanks for the advice
(Btw, could you please remove my email address from your comment? Just in case)

bsdjhb pushed a commit to bsdjhb/cheribsd that referenced this pull request Apr 19, 2023
Functions implemented :

- mac_veriexec_vnode_check_unlink: Unlink on a file has been
  requested and requires validation. This function prohibits the
  deleting a protected file (or deleting one of these hard links, if
  any).
- mac_veriexec_vnode_check_rename_from: Rename the file has been
  requested and must be validated. This function controls the renaming
  of protected file
- mac_veriexec_vnode_check_rename_to: File overwrite rename has been
  requested and must be validated. This function prevent overwriting of
  a file protected (overwriting by mv command).

The 3 fonctions together aim to control the 'removal' (via unlink) and
the 'mv' on files protected by veriexec. The intention is to reach the
functional level of NetBSD veriexec.

Add sysctl node security.mac.veriexec.unlink to toggle control on
syscall unlink.

Add tunable kernel variable security.mac.veriexec.block_unlink to toggle
unlink protection. Add the corresponding read-only sysctl.

[ tidied up commit message, trailing whitespace, long lines, { placement ]

Reviewed by: sjg, imp
Pull Request: freebsd/freebsd-src#613
@emaste emaste added the merged label Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants