Landlock v1.1 #7688

regit · 2022-08-03T12:25:30Z

Initial implementation of landlock support for Suricata.

I have read the contributing guide lines at https://redmine.openinfosecfoundation.org/projects/suricata/wiki/Contributing
I have signed the Open Information Security Foundation contribution agreement at https://suricata.io/about/contribution-agreement/
I have updated the user guide (in doc/userguide/) to reflect the changes made (if applicable)

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

Describe changes:

Add landlock support
Document it

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

Feature: OISF#5479

codecov · 2022-08-03T12:54:44Z

Codecov Report

Merging #7688 (3cbe7f0) into master (f3d3274) will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #7688      +/-   ##
==========================================
+ Coverage   75.88%   75.94%   +0.06%     
==========================================
  Files         659      660       +1     
  Lines      185668   185673       +5     
==========================================
+ Hits       140893   141013     +120     
+ Misses      44775    44660     -115

Flag	Coverage Δ
fuzzcorpus	`60.74% <0.00%> (+0.14%)`	⬆️
suricata-verify	`52.57% <100.00%> (-0.02%)`	⬇️
unittests	`60.71% <0.00%> (+<0.01%)`	⬆️

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

suricata-qa · 2022-08-03T15:20:03Z

Information: QA ran without warnings.

Pipeline 8474

l0kod

Nice!

I guess you would like to squash non-doc related changes from the second commit to the first. ;)

src/util-landlock.c

l0kod · 2022-08-03T15:55:02Z

src/util-landlock.c

+        }
+    }
+
+    ConfNode *read_dirs = ConfGetNode("landlock.directories.read");


I don't know Suricata enough, but couldn't these directories be inferred by other configurations (implicit rather than explicit)?

It is possible to specify full path in signature for example for dataset, lua script. As a user can't really rewrite the signatures, this feature is needed.

I guess we can talk about the thread model. If we trust the config (+rules etc) at startup, we might be able collect all the paths we open and then lock things afterwards? Then we'd protect against runtime exploitation of some kind.

If we want to protect the config/rule loading stage as well we couldn't use this.

Right, and we could do both: a first layer of generic rules to forbid everything that a lua script should never be allowed to do (e.g., create block/char devices, read sensitive files, write new rules…), and a second (standalone) layer taking into account the explicit scripts/rules requirements. Being able to not ask users to explicitly configure sandboxing should be preferred to get a wide adoption.

l0kod · 2022-08-03T15:57:42Z

suricata.yaml.in

+      - @e_logdir@
+      - @e_rundir@
+    read:
+      - /usr/


Couldn't /usr and /etc be inferred from the installation paths?

I did put it there as a feature like filemagic is reading its config file in /etc and load things from /usr/. And this is independent from my build.

Is some thing like /usr/*/magic.mgc possible here?

I've got a nightmare of links on debian to access to this file. I don't think we can do something else than directory with landlock and certainly not a blob.

All glob/regex resolution is the job of user space, and it is then freeze once loaded in the kernel.

Gotcha. Trick with libmagic is that to maximize portability we by default don't specify a path to the magic file. The library will then load from a default value that differs between OS'/distros. I haven't seen a way to retrieve the path that it uses.

l0kod · 2022-08-03T16:45:36Z

suricata.yaml.in

+  enabled: no
+  directories:
+    write:
+      - @e_logdir@


Couldn't the log and run directory be a static/define string?

the are set by --sysconfdir and --localstatedir, e.g. ./configure --prefix=/usr --sysconfdir=/etc --localstatedir=/var

These values it can be overwritten from configuration file so I prefer to have them here. For the log directory @jasonish remark on -l usage makes sense and it could be better to avoid to add it in the configuration.

I got in down to in the MR to come

directories: #write: # - @e_rundir@ read: - /usr/ - /etc/ - @e_sysconfdir@

the are set by --sysconfdir and --localstatedir, e.g. ./configure --prefix=/usr --sysconfdir=/etc --localstatedir=/var

By static/define I meant C static or #define strings that could be populated by the ./configure command at build time.

I got in down to in the MR to come

directories: #write: # - @e_rundir@ read: - /usr/ - /etc/ - @e_sysconfdir@

Great to not have to worry about write permissions!

/usr and /etc could maybe be inferred from a build configuration, maybe a new one with these paths as default values? I think it would be OK if users only had to specify such paths when they have a non-standard installation or distro.

src/util-landlock.c

jasonish

Overall I like this idea. Reminds me a bit of pledge on OpenBSD.

At some point I wonder if it makes sense to just do as much as possible automatically by default? Outside of Lua scripts and plugins, we know what we need to read and write.

Is read of /usr/ required for shared libraries and such?

doc/userguide/configuration/landlock.rst

src/util-landlock.h

src/util-landlock.c

victorjulien · 2022-08-03T18:35:19Z

Thanks for jumping for review @l0kod !

regit · 2022-08-03T19:08:30Z

Nice!

I guess you would like to squash non-doc related changes from the second commit to the first. ;)

Damn, the clang rewrite script did screw me up once more.

victorjulien · 2022-08-03T19:37:38Z

Nice!
I guess you would like to squash non-doc related changes from the second commit to the first. ;)

Damn, the clang rewrite script did screw me up once more.

git rebase master -x 'git clang-format HEAD' works well for me. Any change it makes can be commited as git commit -a --amend.

regit · 2022-08-03T19:41:11Z

Overall I like this idea. Reminds me a bit of pledge on OpenBSD.

At some point I wonder if it makes sense to just do as much as possible automatically by default? Outside of Lua scripts and plugins, we know what we need to read and write.

Is read of /usr/ required for shared libraries and such?

Yes, it is needed for libmagic at least who wants /etc and /usr. Maybe this is the only one.

l0kod · 2022-08-04T09:53:45Z

About LANDLOCK_ACCESS_FS_REFER, you need to make sure it is not required by Suricata to (hard)link or rename files from/to different hierarchies. If it is legitimate for Lua scripts to do so, then we could defer the choice to enable Landlock with ABI 1 to users or infer from the Lua scripts. If it may be legitimate to do so for a default Suricata installation, you should abort sandboxing if the Landlock ABI is 1 (which would deny such actions except if the source/destination are in the same directory).

I guess link and rename actions are not common for most applications, but we should keep this in mind.

LANDLOCK_ACCESS_FS_REFER was a bit complex to implement securely, then the incremental development, which is the reason why renaming/linking was partially restricted at first. This access right (and the lifted limitation) is supported since Linux 5.19.

TL; DR: If Suricata might legitimately rename or link files to different directories, you should consider only using Landlock with ABI >= 2.

See https://www.kernel.org/doc/html/latest/userspace-api/landlock.html#file-renaming-and-linking-abi-1

regit · 2022-08-04T12:22:24Z

About LANDLOCK_ACCESS_FS_REFER, you need to make sure it is not required by Suricata to (hard)link or rename files from/to different hierarchies. If it is legitimate for Lua scripts to do so, then we could defer the choice to enable Landlock with ABI 1 to users or infer from the Lua scripts. If it may be legitimate to do so for a default Suricata installation, you should abort sandboxing if the Landlock ABI is 1 (which would deny such actions except if the source/destination are in the same directory).

I guess link and rename actions are not common for most applications, but we should keep this in mind.

LANDLOCK_ACCESS_FS_REFER was a bit complex to implement securely, then the incremental development, which is the reason why renaming/linking was partially restricted at first. This access right (and the lifted limitation) is supported since Linux 5.19.

TL; DR: If Suricata might legitimately rename or link files to different directories, you should consider only using Landlock with ABI >= 2.

See https://www.kernel.org/doc/html/latest/userspace-api/landlock.html#file-renaming-and-linking-abi-1

OK, this could be really an issue with files extraction, I need to test that.

l0kod · 2022-08-04T14:19:35Z

OK, this could be really an issue with files extraction, I need to test that.

You can test that behavior by just removing LANDLOCK_ACCESS_FS_REFER from handled_access_fs (and then allowed_access).

regit · 2022-08-04T14:45:49Z

OK, this could be really an issue with files extraction, I need to test that.

You can test that behavior by just removing LANDLOCK_ACCESS_FS_REFER from handled_access_fs (and then allowed_access).

OK, I run a 5.18 and I got:

[68938] 4/8/2022 -- 16:43:59 - (output-filestore.c:144) <Warning> (OutputFilestoreFinalizeFiles) -- [ERRCODE: SC_WARN_RENAMING_FILE(303)] - Failed to rename /tmp/ddd//filestore/tmp/file.1 to /tmp/ddd//filestore/01/010f5a2db9b9ce1e8a9e0ae6837f8fb208f825423ab35aac09c26e3d2b3d4aef: Invalid cross-device link
[68942] 4/8/2022 -- 16:43:59 - (output-filestore.c:144) <Warning> (OutputFilestoreFinalizeFiles) -- [ERRCODE: SC_WARN_RENAMING_FILE(303)] - Failed to rename /tmp/ddd//filestore/tmp/file.2 to /tmp/ddd//filestore/ef/ef7239c674fd380fc2d8713d0fd82be91e543b7562ccfdffce3357819be1fd2c: Invalid cross-device link

So it makes sense for now to disable landlock if abi is 1 and filestore is enabled.

l0kod · 2022-08-04T15:22:49Z

So it makes sense for now to disable landlock if abi is 1 and filestore is enabled.

Usually, tools copy files instead of renaming or linking them when these syscalls return EXDEV (e.g. mv), but in the case of filestore I agree with these conditions when not to use Landlock.

victorjulien · 2022-08-04T15:33:28Z

So it makes sense for now to disable landlock if abi is 1 and filestore is enabled.

Usually, tools copy files instead of renaming or linking them when these syscalls return EXDEV (e.g. mv), but in the case of filestore I agree with these conditions when not to use Landlock.

Is this related? https://redmine.openinfosecfoundation.org/issues/3003

Wonder if it could make sense to optionally support this using a copy logic.This would be at the cost of perf, but the trade off might be worth it to some?

l0kod · 2022-08-04T15:45:24Z

Is this related? https://redmine.openinfosecfoundation.org/issues/3003

Wonder if it could make sense to optionally support this using a copy logic.This would be at the cost of perf, but the trade off might be worth it to some?

Indeed, that might be interesting. Renaming and linking will always be faster than a copy, but the cost could be small using in-kernel copy such as sendfile or splice, especially with tmpfs.

regit · 2022-08-04T20:57:41Z

Replaced by #7696

regit added 2 commits August 3, 2022 14:23

doc: add landlock documentation

3cbe7f0

Feature: OISF#5479

regit requested review from jufajardini and victorjulien as code owners August 3, 2022 12:25

l0kod reviewed Aug 3, 2022

View reviewed changes

jasonish requested changes Aug 3, 2022

View reviewed changes

doc/userguide/configuration/landlock.rst Show resolved Hide resolved

src/util-landlock.h Show resolved Hide resolved

src/util-landlock.c Show resolved Hide resolved

src/util-landlock.c Show resolved Hide resolved

regit closed this Aug 4, 2022

regit reopened this Aug 4, 2022

regit mentioned this pull request Aug 4, 2022

Landlock v1.2 #7696

Closed

3 tasks

regit closed this Aug 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Landlock v1.1 #7688

Landlock v1.1 #7688

regit commented Aug 3, 2022

codecov bot commented Aug 3, 2022 •

edited

Loading

suricata-qa commented Aug 3, 2022

l0kod left a comment

l0kod Aug 3, 2022

regit Aug 3, 2022

victorjulien Aug 3, 2022

l0kod Aug 4, 2022

l0kod Aug 3, 2022

regit Aug 3, 2022 •

edited

Loading

victorjulien Aug 3, 2022

regit Aug 3, 2022 •

edited

Loading

l0kod Aug 4, 2022

victorjulien Aug 4, 2022

l0kod Aug 3, 2022

victorjulien Aug 3, 2022

regit Aug 3, 2022

regit Aug 3, 2022 •

edited

Loading

l0kod Aug 4, 2022

l0kod Aug 4, 2022

jasonish left a comment

victorjulien commented Aug 3, 2022

regit commented Aug 3, 2022

victorjulien commented Aug 3, 2022 •

edited

Loading

regit commented Aug 3, 2022

l0kod commented Aug 4, 2022

regit commented Aug 4, 2022

l0kod commented Aug 4, 2022

regit commented Aug 4, 2022

l0kod commented Aug 4, 2022 •

edited

Loading

victorjulien commented Aug 4, 2022

l0kod commented Aug 4, 2022

regit commented Aug 4, 2022

Landlock v1.1 #7688

Landlock v1.1 #7688

Conversation

regit commented Aug 3, 2022

codecov bot commented Aug 3, 2022 • edited Loading

Codecov Report

suricata-qa commented Aug 3, 2022

l0kod left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

regit Aug 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

regit Aug 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

regit Aug 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonish left a comment

Choose a reason for hiding this comment

victorjulien commented Aug 3, 2022

regit commented Aug 3, 2022

victorjulien commented Aug 3, 2022 • edited Loading

regit commented Aug 3, 2022

l0kod commented Aug 4, 2022

regit commented Aug 4, 2022

l0kod commented Aug 4, 2022

regit commented Aug 4, 2022

l0kod commented Aug 4, 2022 • edited Loading

victorjulien commented Aug 4, 2022

l0kod commented Aug 4, 2022

regit commented Aug 4, 2022

codecov bot commented Aug 3, 2022 •

edited

Loading

regit Aug 3, 2022 •

edited

Loading

regit Aug 3, 2022 •

edited

Loading

regit Aug 3, 2022 •

edited

Loading

victorjulien commented Aug 3, 2022 •

edited

Loading

l0kod commented Aug 4, 2022 •

edited

Loading