| Message ID | 20221205112621.3530557-1-mic@digikod.net (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | [v1] landlock: Explain file descriptor access rights | expand |
Hi! Thanks for sending this patch! I have overlooked to update this in the original patch set. On Mon, Dec 05, 2022 at 12:26:21PM +0100, Mickaël Salaün wrote: > Starting with LANDLOCK_ACCESS_FS_TRUNCATE, it is worth explaining why we > choose to restrict access checks at open time. This new "File > descriptor access rights" section is complementary to the existing > "Inode access rights" section. > > Cc: Günther Noack <gnoack3000@gmail.com> > Signed-off-by: Mickaël Salaün <mic@digikod.net> > Link: https://lore.kernel.org/r/20221205112621.3530557-1-mic@digikod.net > --- > Documentation/security/landlock.rst | 25 ++++++++++++++++++++++--- > 1 file changed, 22 insertions(+), 3 deletions(-) > > diff --git a/Documentation/security/landlock.rst b/Documentation/security/landlock.rst > index c0029d5d02eb..bd0af6031ebb 100644 > --- a/Documentation/security/landlock.rst > +++ b/Documentation/security/landlock.rst > @@ -7,7 +7,7 @@ Landlock LSM: kernel documentation > ================================== > > :Author: Mickaël Salaün > -:Date: September 2022 > +:Date: November 2022 > > Landlock's goal is to create scoped access-control (i.e. sandboxing). To > harden a whole system, this feature should be available to any process, > @@ -45,8 +45,8 @@ Guiding principles for safe access controls > Design choices > ============== > > -Filesystem access rights > ------------------------- > +Inode access rights > +------------------- > > All access rights are tied to an inode and what can be accessed through it. > Reading the content of a directory does not imply to be allowed to read the > @@ -57,6 +57,25 @@ directory, not the unlinked inode. This is the reason why > ``LANDLOCK_ACCESS_FS_REMOVE_FILE`` or ``LANDLOCK_ACCESS_FS_REFER`` are not > allowed to be tied to files but only to directories. > > +File descriptor access rights > +----------------------------- > + > +Access rights are checked and tied to file descriptors at open time. As a > +consequence, it may be allowed to create a file without being allowed to > +truncate it if the file hierarchy doesn't grant such access right. The > +rationale is that this approach is simple and consistent with all access > +rights, however file requests are based on path or based on file descriptor > +(obtained with the same path, by a thread restricted with the same Landlock > +domain). For instance, updating an application from using :manpage:`mknod` and > +:manpage:`truncate` to initialize a file (i.e. path-based), to using > +:manpage:`open` and :manpage:`ftruncate` to initialize the same file (i.e. file > +descriptor-based) should work the same way with the same Landlock restrictions. Nit: The paragraph seems a bit long and is mixing more general considerations with examples. Maybe they could be split into separate paragraphs? Regarding the "As a consequence..." example: I would word it as "...it may be allowed to open a file for writing without being allowed to ftruncate the resulting file descriptor...". The example you are giving with creating files is also accurate, but it is potentially confusing, because creat() and open(..., O_TRUNC) are also implicitly truncating files when opening the file. Regarding "The rationale is that this approach is simple and consistent...": The word "simple" is often a sign that we could be more concrete, and there is also a risk that a reader might not perceive it as simple ;) How about this: ``` The rationale is that equivalent sequences of operations should lead to the same results, when they are executed under the same Landlock domain. For example, for the LANDLOCK_ACCESS_FS_TRUNCATE right, the following sequences of operations are roughly equivalent and should have the same result: * truncate(path) * fd = open(path, O_WRONLY); ftruncate(fd); close(fd) ``` (I think it's a bit more readable with bullet points, and the truncate/ftruncate example might be a bit more familiar than the somewhat unusual mknod?) > + > +Processes not sandboxed by Landlock may still be restricted for operations on > +file descriptors coming from a sandboxed process. Indeed, this is required to > +keep a consistent access control over the whole system, and avoid unattended > +bypasses through file descriptor passing (i.e. confused deputy attack). "May still be restricted" is leaving the reader a bit in the dark about the exact circumstances where this might happen? I think we could be more bold and give a guarantee here, like: ``` Access rights attached to file descriptors are retained even if the file descriptor is passed between Unix processes (e.g. through a Unix Domain Socket). The access rights will be enforced even if the receiving process is not sandboxed by Landlock. ``` --Günther > + > Tests > ===== > > > base-commit: 0b4ab8cd635e8b21e42c14b9e4810ca701babd11 > -- > 2.38.1 > --
Thanks Günther, I'll send a v2. On 07/12/2022 18:49, Günther Noack wrote: > Hi! > > Thanks for sending this patch! I have overlooked to update this in the > original patch set. > > On Mon, Dec 05, 2022 at 12:26:21PM +0100, Mickaël Salaün wrote: >> Starting with LANDLOCK_ACCESS_FS_TRUNCATE, it is worth explaining why we >> choose to restrict access checks at open time. This new "File >> descriptor access rights" section is complementary to the existing >> "Inode access rights" section. >> >> Cc: Günther Noack <gnoack3000@gmail.com> >> Signed-off-by: Mickaël Salaün <mic@digikod.net> >> Link: https://lore.kernel.org/r/20221205112621.3530557-1-mic@digikod.net >> --- >> Documentation/security/landlock.rst | 25 ++++++++++++++++++++++--- >> 1 file changed, 22 insertions(+), 3 deletions(-) >> >> diff --git a/Documentation/security/landlock.rst b/Documentation/security/landlock.rst >> index c0029d5d02eb..bd0af6031ebb 100644 >> --- a/Documentation/security/landlock.rst >> +++ b/Documentation/security/landlock.rst >> @@ -7,7 +7,7 @@ Landlock LSM: kernel documentation >> ================================== >> >> :Author: Mickaël Salaün >> -:Date: September 2022 >> +:Date: November 2022 >> >> Landlock's goal is to create scoped access-control (i.e. sandboxing). To >> harden a whole system, this feature should be available to any process, >> @@ -45,8 +45,8 @@ Guiding principles for safe access controls >> Design choices >> ============== >> >> -Filesystem access rights >> ------------------------- >> +Inode access rights >> +------------------- >> >> All access rights are tied to an inode and what can be accessed through it. >> Reading the content of a directory does not imply to be allowed to read the >> @@ -57,6 +57,25 @@ directory, not the unlinked inode. This is the reason why >> ``LANDLOCK_ACCESS_FS_REMOVE_FILE`` or ``LANDLOCK_ACCESS_FS_REFER`` are not >> allowed to be tied to files but only to directories. >> >> +File descriptor access rights >> +----------------------------- >> + >> +Access rights are checked and tied to file descriptors at open time. As a >> +consequence, it may be allowed to create a file without being allowed to >> +truncate it if the file hierarchy doesn't grant such access right. The >> +rationale is that this approach is simple and consistent with all access >> +rights, however file requests are based on path or based on file descriptor >> +(obtained with the same path, by a thread restricted with the same Landlock >> +domain). For instance, updating an application from using :manpage:`mknod` and >> +:manpage:`truncate` to initialize a file (i.e. path-based), to using >> +:manpage:`open` and :manpage:`ftruncate` to initialize the same file (i.e. file >> +descriptor-based) should work the same way with the same Landlock restrictions. > > Nit: The paragraph seems a bit long and is mixing more general > considerations with examples. Maybe they could be split into separate > paragraphs? > > Regarding the "As a consequence..." example: I would word it as "...it > may be allowed to open a file for writing without being allowed to > ftruncate the resulting file descriptor...". > > The example you are giving with creating files is also accurate, but > it is potentially confusing, because creat() and open(..., O_TRUNC) > are also implicitly truncating files when opening the file. > > Regarding "The rationale is that this approach is simple and > consistent...": The word "simple" is often a sign that we could be > more concrete, and there is also a risk that a reader might not > perceive it as simple ;) How about this: > > ``` > > The rationale is that equivalent sequences of operations should lead > to the same results, when they are executed under the same Landlock > domain. > > For example, for the LANDLOCK_ACCESS_FS_TRUNCATE right, the following > sequences of operations are roughly equivalent and should have the > same result: > > * truncate(path) > * fd = open(path, O_WRONLY); ftruncate(fd); close(fd) > > ``` > > (I think it's a bit more readable with bullet points, and the > truncate/ftruncate example might be a bit more familiar than the > somewhat unusual mknod?) > >> + >> +Processes not sandboxed by Landlock may still be restricted for operations on >> +file descriptors coming from a sandboxed process. Indeed, this is required to >> +keep a consistent access control over the whole system, and avoid unattended >> +bypasses through file descriptor passing (i.e. confused deputy attack). > > "May still be restricted" is leaving the reader a bit in the dark > about the exact circumstances where this might happen? I think we > could be more bold and give a guarantee here, like: > > ``` > > Access rights attached to file descriptors are retained even if the > file descriptor is passed between Unix processes (e.g. through a Unix > Domain Socket). The access rights will be enforced even if the > receiving process is not sandboxed by Landlock. > > ``` > > --Günther > >> + >> Tests >> ===== >> >> >> base-commit: 0b4ab8cd635e8b21e42c14b9e4810ca701babd11 >> -- >> 2.38.1 >> >
diff --git a/Documentation/security/landlock.rst b/Documentation/security/landlock.rst index c0029d5d02eb..bd0af6031ebb 100644 --- a/Documentation/security/landlock.rst +++ b/Documentation/security/landlock.rst @@ -7,7 +7,7 @@ Landlock LSM: kernel documentation ================================== :Author: Mickaël Salaün -:Date: September 2022 +:Date: November 2022 Landlock's goal is to create scoped access-control (i.e. sandboxing). To harden a whole system, this feature should be available to any process, @@ -45,8 +45,8 @@ Guiding principles for safe access controls Design choices ============== -Filesystem access rights ------------------------- +Inode access rights +------------------- All access rights are tied to an inode and what can be accessed through it. Reading the content of a directory does not imply to be allowed to read the @@ -57,6 +57,25 @@ directory, not the unlinked inode. This is the reason why ``LANDLOCK_ACCESS_FS_REMOVE_FILE`` or ``LANDLOCK_ACCESS_FS_REFER`` are not allowed to be tied to files but only to directories. +File descriptor access rights +----------------------------- + +Access rights are checked and tied to file descriptors at open time. As a +consequence, it may be allowed to create a file without being allowed to +truncate it if the file hierarchy doesn't grant such access right. The +rationale is that this approach is simple and consistent with all access +rights, however file requests are based on path or based on file descriptor +(obtained with the same path, by a thread restricted with the same Landlock +domain). For instance, updating an application from using :manpage:`mknod` and +:manpage:`truncate` to initialize a file (i.e. path-based), to using +:manpage:`open` and :manpage:`ftruncate` to initialize the same file (i.e. file +descriptor-based) should work the same way with the same Landlock restrictions. + +Processes not sandboxed by Landlock may still be restricted for operations on +file descriptors coming from a sandboxed process. Indeed, this is required to +keep a consistent access control over the whole system, and avoid unattended +bypasses through file descriptor passing (i.e. confused deputy attack). + Tests =====
Starting with LANDLOCK_ACCESS_FS_TRUNCATE, it is worth explaining why we choose to restrict access checks at open time. This new "File descriptor access rights" section is complementary to the existing "Inode access rights" section. Cc: Günther Noack <gnoack3000@gmail.com> Signed-off-by: Mickaël Salaün <mic@digikod.net> Link: https://lore.kernel.org/r/20221205112621.3530557-1-mic@digikod.net --- Documentation/security/landlock.rst | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) base-commit: 0b4ab8cd635e8b21e42c14b9e4810ca701babd11