Skip to content

Landlock v1.2 #7696

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

Closed
wants to merge 4 commits into from
Closed

Landlock v1.2 #7696

wants to merge 4 commits into from

Conversation

regit
Copy link
Contributor

@regit regit commented Aug 4, 2022

Update of #7688 addressing the remarks.

Link to redmine ticket: https://redmine.openinfosecfoundation.org/issues/5479

Describe changes:

  • Handle file store case
  • Fix capabilities and ABI detection
  • No more directories in write list
  • Less directories in read list

regit added 3 commits August 4, 2022 22:51
This patch is adding support for Landlock, a Linux
Security Module available since Linux 5.13.

The concept is to prevent any file operation on directories where
Suricata is not supposed to access.

Landlock support is built by default if the header is present. The
feature is disabled by default and need to be activated in the YAML
to be active.

Landlock documentation: https://docs.kernel.org/userspace-api/landlock.html

Feature: OISF#5479
Add functions to state that file store is enable and one other
one to check it.
If landlock ABI is inferior to 2 (before Linux 5.19) then the
renaming of files is impossible if the protection is enabled. This
patch disables landlock if ABI < 2 and file-store is enabled.

As file store is initialized in output the call to landlock had to
done after the output initialization.
@regit regit mentioned this pull request Aug 4, 2022
3 tasks
@codecov
Copy link

codecov bot commented Aug 4, 2022

Codecov Report

Merging #7696 (fcfc42a) into master (f3d3274) will increase coverage by 0.04%.
The diff coverage is 77.77%.

@@            Coverage Diff             @@
##           master    #7696      +/-   ##
==========================================
+ Coverage   75.88%   75.93%   +0.04%     
==========================================
  Files         659      660       +1     
  Lines      185668   185678      +10     
==========================================
+ Hits       140893   140990      +97     
+ Misses      44775    44688      -87     
Flag Coverage Δ
fuzzcorpus 60.74% <44.44%> (+0.14%) ⬆️
suricata-verify 52.53% <77.77%> (-0.05%) ⬇️
unittests 60.71% <0.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 8502

}
}

void LandlockSandboxing(SCInstance *suri)
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to hear some thoughts about whether we should try to set the complete policy here, which means we encode logic like runmode, -S commandline option presence, etc here, or that we would scatter the policy logic over the actual code handling those things like runmodes and commandline option handling.

To me it feels a bit out of place here, but OTOH it is nice to be able to see it in a single place. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree on the feeling as I got the same. I thought at first: let's add a registration mechanism for modules to add their read and write directories. But I stop myself than doing this because I wanted the protection to be done early in the run. I was also afraid of loosing track of what was really enabled. With current status, the code handling this part is not that big. But I think that if we want to go more granular (v2 of landlock support) with things like per thread permissions then we will need to have a framework in place.

Copy link
Member

Choose a reason for hiding this comment

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

I can live with that for this early phase. I guess going forward we might add some file & dir open wrappers that track the paths and annotate them, e.g.
SCFileOpenOnce <- would not get in policy if the file access already happened.
SCFileOpenOften <- would get into policy
Based on the file open flags we could put them into read or write, and maybe we'd have some additional logic for special operations like the rename case discussed before.

In our support for libcap-ng we have done a per module logic that is applied centrally, I think this ultimately should be done in a similar way.

@regit regit mentioned this pull request Aug 5, 2022
3 tasks
Copy link
Contributor

@jufajardini jufajardini left a comment

Choose a reason for hiding this comment

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

Documentation looks good and informative to me! :)

Just some nit comments from my end

==================

Landlock is a Linux Security Module that has been introduced in Linux 5.13.
It allows an application to sandbox itself by selecting access right to
Copy link
Contributor

Choose a reason for hiding this comment

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

s/right/rights

@regit regit closed this Aug 5, 2022
@jufajardini
Copy link
Contributor

Documentation looks good and informative to me! :)

Just some nit comments from my end

I guess they came in too late :x sorry about that.

@regit
Copy link
Contributor Author

regit commented Aug 5, 2022

Documentation looks good and informative to me! :)
Just some nit comments from my end

I guess they came in too late :x sorry about that.

No pb, I'm updating the new MR :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants