Re: [PATCH 6/6] tools/pygrub: Hook libfsimage's fdopen() to pygrub

[Andrew Cooper <andcooper@xxxxxxxxx>]

On 06/11/2023 3:05 pm, Alejandro Vallejo wrote:
> This patch increases the security posture by removing the need to grant
> filesystem access to the depriv pygrub. Using libfsimage's fdopen(), the
> parent thread in the depriv procedure can simply ensure all the appropriate
> file descriptors are present before revoking permissions to the filesystem.
>
> A convenient usability side-effect is that it's no longer required for the
> restricted user to have access to the disk, because the depriv thread
> already has the file descriptor open by its parent.
>
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>
> ---
>  docs/man/xl.cfg.5.pod.in         |  6 +-
>  tools/pygrub/src/ExtLinuxConf.py | 20 ++++---
>  tools/pygrub/src/GrubConf.py     | 29 ++++++----
>  tools/pygrub/src/LiloConf.py     | 20 ++++---
>  tools/pygrub/src/pygrub          | 95 ++++++++------------------------
>  5 files changed, 68 insertions(+), 102 deletions(-)
>
> diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
> index 2e234b450e..e3fd2e37eb 100644
> --- a/docs/man/xl.cfg.5.pod.in
> +++ b/docs/man/xl.cfg.5.pod.in
> @@ -1704,8 +1704,7 @@ See docs/features/qemu-deprivilege.pandoc for more 
> information
>  on how to setup the unprivileged users.
>  
>  Note that running the bootloader in restricted mode also implies using
> -non-interactive mode, and the disk image must be readable by the
> -restricted user.
> +non-interactive mode.
>  
>  =item B<bootloader_user=USERNAME>
>  
> @@ -2768,8 +2767,7 @@ See docs/features/qemu-deprivilege.pandoc for more 
> information
>  on how to setup the unprivileged users.
>  
>  Note that running the bootloader in restricted mode also implies using
> -non-interactive mode, and the disk image must be readable by the
> -restricted user.
> +non-interactive mode.
>  
>  =item B<bootloader_user=USERNAME>
>  

I'm leaning towards suggesting that this wants a note in the changelog,
as we're removing a limitation imposed by a security fix.


> diff --git a/tools/pygrub/src/ExtLinuxConf.py 
> b/tools/pygrub/src/ExtLinuxConf.py
> index 4e990a9304..f2e9a81013 100644
> --- a/tools/pygrub/src/ExtLinuxConf.py
> +++ b/tools/pygrub/src/ExtLinuxConf.py
> @@ -123,6 +123,7 @@ class ExtLinuxImage(object):
>  class ExtLinuxConfigFile(object):
>      def __init__(self, fn = None):
>          self.filename = fn
> +        self.file = None
>          self.images = []
>          self.timeout = -1
>          self._default = 0
> @@ -138,16 +139,21 @@ class ExtLinuxConfigFile(object):
>  
>      def parse(self, buf = None):
>          if buf is None:
> -            if self.filename is None:
> +            if not self.filename and not self.file:
>                  raise ValueError("No config file defined to parse!")
>  
> -            f = open(self.filename, 'r')
> -            lines = f.readlines()
> -            f.close()
> -        else:
> -            lines = buf.split("\n")
> +            if self.file:
> +                buf = file.read()
> +                path = self.file.name
> +            else:
> +                f = open(self.filename, 'r')
> +                buf = f.read()
> +                f.close()
> +        lines = buf.split("\n")
> +
> +        if not path:
> +            path = os.path.dirname(self.filename)
>  
> -        path = os.path.dirname(self.filename)
>          img = []
>          for l in lines:
>              l = l.strip()

[List context, Alejandro and I discussed this IRL]

This pattern is horrible and repeated in each object.  I'm going to
experiment and see if there's a (limited) bit of cleanup which can be
done to reduce the invasiveness, and therefore the legibility of this patch.

~Andrew