[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 6/6] tools/pygrub: Hook libfsimage's fdopen() to pygrub
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |