chmod of generated grub.cfg

chmod of generated grub.cfg

[Felix Zielcke]

Currently grub-mkconfig uses chmod 444 on the newly generated grub.cfg
Wouldn't it be better to use 400 now that we have plaintext password
support?
Or should we add support for a GRUB_CHMOD variable so users can override
this setting as they please?

Else I'd need to add a /etc/grub.d/999_chmod file in grub-installer
which changes the mode of grub.cfg.new if the user wants to have a
password.

-- 
Felix Zielcke
Proud Debian Maintainer

Re: chmod of generated grub.cfg

[Colin Watson]

On Sun, Sep 06, 2009 at 02:29:03PM +0200, Felix Zielcke wrote:
> Currently grub-mkconfig uses chmod 444 on the newly generated grub.cfg
> Wouldn't it be better to use 400 now that we have plaintext password
> support?
> Or should we add support for a GRUB_CHMOD variable so users can override
> this setting as they please?

I'd prefer to see this done only if they set a password. A GRUB_CHMOD
variable seems overkill, though.

> Else I'd need to add a /etc/grub.d/999_chmod file in grub-installer
> which changes the mode of grub.cfg.new if the user wants to have a
> password.

I think it'd be more sensible to do this in grub-mkconfig itself - it
doesn't really fit well into the /etc/grub.d/ hook system, which is
really just for generating output.

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: chmod of generated grub.cfg

[Felix Zielcke]

Am Sonntag, den 06.09.2009, 14:38 +0100 schrieb Colin Watson:
> On Sun, Sep 06, 2009 at 02:29:03PM +0200, Felix Zielcke wrote:
> > Currently grub-mkconfig uses chmod 444 on the newly generated grub.cfg
> > Wouldn't it be better to use 400 now that we have plaintext password
> > support?
> > Or should we add support for a GRUB_CHMOD variable so users can override
> > this setting as they please?
> 
> I'd prefer to see this done only if they set a password. A GRUB_CHMOD
> variable seems overkill, though.

> > Else I'd need to add a /etc/grub.d/999_chmod file in grub-installer
> > which changes the mode of grub.cfg.new if the user wants to have a
> > password.
> 
> I think it'd be more sensible to do this in grub-mkconfig itself - it
> doesn't really fit well into the /etc/grub.d/ hook system, which is
> really just for generating output.

You mean we check with grep if there's a password line in the generated
config and then just use chmod 400 instead of 444?
Sounds good.


-- 
Felix Zielcke
Proud Debian Maintainer

Re: chmod of generated grub.cfg

[Colin Watson]

On Sun, Sep 06, 2009 at 03:43:46PM +0200, Felix Zielcke wrote:
> Am Sonntag, den 06.09.2009, 14:38 +0100 schrieb Colin Watson:
> > I think it'd be more sensible to do this in grub-mkconfig itself - it
> > doesn't really fit well into the /etc/grub.d/ hook system, which is
> > really just for generating output.
> 
> You mean we check with grep if there's a password line in the generated
> config and then just use chmod 400 instead of 444?
> Sounds good.

Yeah, that kind of thing.

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: chmod of generated grub.cfg

[Felix Zielcke]

[-- Attachment #1: Type: text/plain, Size: 681 bytes --]

Am Sonntag, den 06.09.2009, 14:57 +0100 schrieb Colin Watson:
> On Sun, Sep 06, 2009 at 03:43:46PM +0200, Felix Zielcke wrote:
> > Am Sonntag, den 06.09.2009, 14:38 +0100 schrieb Colin Watson:
> > > I think it'd be more sensible to do this in grub-mkconfig itself - it
> > > doesn't really fit well into the /etc/grub.d/ hook system, which is
> > > really just for generating output.
> > 
> > You mean we check with grep if there's a password line in the generated
> > config and then just use chmod 400 instead of 444?
> > Sounds good.
> 
> Yeah, that kind of thing.
> 

Ok here's now a patch.
Robert do you think this can go into 1.97?

-- 
Felix Zielcke
Proud Debian Maintainer

[-- Attachment #2: chmod.diff --]
[-- Type: text/x-patch, Size: 804 bytes --]

2009-09-06  Felix Zielcke  <fzielcke@z-51.de>

	* util/grub-mkconfig.in: Make generated config file mode 400 if
	it contains a password and print a warning if it fails.

Index: util/grub-mkconfig.in
===================================================================
--- util/grub-mkconfig.in	(revision 2574)
+++ util/grub-mkconfig.in	(working copy)
@@ -260,6 +260,11 @@ for i in ${grub_mkconfig_dir}/* ; do
   esac
 done
 
+if [ "x${grub_cfg}" != "x" ] && grep -q "^password " ${grub_cfg}.new ; then
+  chmod 400 ${grub_cfg}.new || grub_warn "Could not make ${grub_cfg}.new readable by only root.\
+  This means your password is readable by everyone"
+fi
+
 if test "x${grub_cfg}" != "x" ; then
   # none of the children aborted with error, install the new grub.cfg
   mv -f ${grub_cfg}.new ${grub_cfg}

Re: chmod of generated grub.cfg

[Robert Millan]

On Sun, Sep 06, 2009 at 02:38:18PM +0100, Colin Watson wrote:
> 
> A GRUB_CHMOD
> variable seems overkill, though.

Yes.  Please not a new config option just for this.  Just pick a reasonable
mode and set to it.

Auto-detection doesn't sound bad, although I'd be fine with just using
0400.

> > Else I'd need to add a /etc/grub.d/999_chmod file in grub-installer
> > which changes the mode of grub.cfg.new if the user wants to have a
> > password.
> 
> I think it'd be more sensible to do this in grub-mkconfig itself - it
> doesn't really fit well into the /etc/grub.d/ hook system, which is
> really just for generating output.

Agreed.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: chmod of generated grub.cfg

[Robert Millan]

On Sun, Sep 06, 2009 at 04:58:40PM +0200, Felix Zielcke wrote:
> 
> Ok here's now a patch.
> Robert do you think this can go into 1.97?

For 1.97 I'd be more comfortable with a simple s/444/400/.  An automated check
smells like it could have corner cases.  In fact I found one:

> @@ -260,6 +260,11 @@ for i in ${grub_mkconfig_dir}/* ; do
>    esac
>  done
>  
> +if [ "x${grub_cfg}" != "x" ] && grep -q "^password " ${grub_cfg}.new ; then
> +  chmod 400 ${grub_cfg}.new || grub_warn "Could not make ${grub_cfg}.new readable by only root.\
> +  This means your password is readable by everyone"
> +fi

There's a short time window in which /boot/grub/grub.cfg.new exists, has been
fully generated, and its mode is 444 rather than 400.  An attacker could poll
this file and with some luck extract a password from it.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: chmod of generated grub.cfg

[Felix Zielcke]

[-- Attachment #1: Type: text/plain, Size: 994 bytes --]

Am Sonntag, den 06.09.2009, 17:09 +0200 schrieb Robert Millan:
> On Sun, Sep 06, 2009 at 04:58:40PM +0200, Felix Zielcke wrote:
> > 
> > Ok here's now a patch.
> > Robert do you think this can go into 1.97?
> 
> For 1.97 I'd be more comfortable with a simple s/444/400/.  An automated check
> smells like it could have corner cases.  In fact I found one:
> 
> > @@ -260,6 +260,11 @@ for i in ${grub_mkconfig_dir}/* ; do
> >    esac
> >  done
> >  
> > +if [ "x${grub_cfg}" != "x" ] && grep -q "^password " ${grub_cfg}.new ; then
> > +  chmod 400 ${grub_cfg}.new || grub_warn "Could not make ${grub_cfg}.new readable by only root.\
> > +  This means your password is readable by everyone"
> > +fi
> 
> There's a short time window in which /boot/grub/grub.cfg.new exists, has been
> fully generated, and its mode is 444 rather than 400.  An attacker could poll
> this file and with some luck extract a password from it.
> 

Oh right. So how about this?

-- 
Felix Zielcke
Proud Debian Maintainer

[-- Attachment #2: chmod.diff.2 --]
[-- Type: text/plain, Size: 1196 bytes --]

2009-09-06  Felix Zielcke  <fzielcke@z-51.de>

	* util/grub-mkconfig.in: Make the temporary created config mode
	400 and print a warning if it fails.  Change mode to 444 if it
	does not contain a plaintext password.

Index: util/grub-mkconfig.in
===================================================================
--- util/grub-mkconfig.in	(revision 2574)
+++ util/grub-mkconfig.in	(working copy)
@@ -232,7 +232,8 @@ if test "x${grub_cfg}" != "x"; then
 
   # Allow this to fail, since /boot/grub/ might need to be fatfs to support some
   # firmware implementations (e.g. OFW or EFI).
-  chmod 444 ${grub_cfg}.new || true
+  chmod 400 ${grub_cfg}.new || grub_warn "Could not make ${grub_cfg}.new readable by only root.\
+  This means that if the generated config contains a password it is readable by everyone"
 fi
 echo "Generating grub.cfg ..." >&2
 
@@ -260,6 +261,10 @@ for i in ${grub_mkconfig_dir}/* ; do
   esac
 done
 
+if [ "x${grub_cfg}" != "x" ] && ! grep -q "^password " ${grub_cfg}.new ; then
+  chmod 444 ${grub_cfg}.new || true
+fi
+
 if test "x${grub_cfg}" != "x" ; then
   # none of the children aborted with error, install the new grub.cfg
   mv -f ${grub_cfg}.new ${grub_cfg}

Re: chmod of generated grub.cfg

[Vladimir 'phcoder' Serbinenko]

On Sun, Sep 6, 2009 at 3:38 PM, Colin Watson<cjwatson@ubuntu.com> wrote:
> On Sun, Sep 06, 2009 at 02:29:03PM +0200, Felix Zielcke wrote:
>> Currently grub-mkconfig uses chmod 444 on the newly generated grub.cfg
>> Wouldn't it be better to use 400 now that we have plaintext password
>> support?
>> Or should we add support for a GRUB_CHMOD variable so users can override
>> this setting as they please?
>
> I'd prefer to see this done only if they set a password. A GRUB_CHMOD
> variable seems overkill, though.
Is there a reason a non-root would like to look at grub.cfg on
production system? Developers can always override chmod. If there is
no real reason for non-root to look into grub.cfg I would follow the
best friend in security considerations called "paranoia" and just use
mode 400
>
>> Else I'd need to add a /etc/grub.d/999_chmod file in grub-installer
>> which changes the mode of grub.cfg.new if the user wants to have a
>> password.
>
> I think it'd be more sensible to do this in grub-mkconfig itself - it
> doesn't really fit well into the /etc/grub.d/ hook system, which is
> really just for generating output.

>
> --
> Colin Watson                                       [cjwatson@ubuntu.com]
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git

Re: chmod of generated grub.cfg

[Robert Millan]

On Sun, Sep 06, 2009 at 05:17:34PM +0200, Felix Zielcke wrote:
> Am Sonntag, den 06.09.2009, 17:09 +0200 schrieb Robert Millan:
> > On Sun, Sep 06, 2009 at 04:58:40PM +0200, Felix Zielcke wrote:
> > > 
> > > Ok here's now a patch.
> > > Robert do you think this can go into 1.97?
> > 
> > For 1.97 I'd be more comfortable with a simple s/444/400/.  An automated check
> > smells like it could have corner cases.  In fact I found one:
> > 
> > > @@ -260,6 +260,11 @@ for i in ${grub_mkconfig_dir}/* ; do
> > >    esac
> > >  done
> > >  
> > > +if [ "x${grub_cfg}" != "x" ] && grep -q "^password " ${grub_cfg}.new ; then
> > > +  chmod 400 ${grub_cfg}.new || grub_warn "Could not make ${grub_cfg}.new readable by only root.\
> > > +  This means your password is readable by everyone"
> > > +fi
> > 
> > There's a short time window in which /boot/grub/grub.cfg.new exists, has been
> > fully generated, and its mode is 444 rather than 400.  An attacker could poll
> > this file and with some luck extract a password from it.
> > 
> 
> Oh right. So how about this?

Ok.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: chmod of generated grub.cfg

[richardvoigt]

On Sun, Sep 6, 2009 at 12:22 PM, Vladimir 'phcoder'
Serbinenko<phcoder@gmail.com> wrote:
> On Sun, Sep 6, 2009 at 3:38 PM, Colin Watson<cjwatson@ubuntu.com> wrote:
>> On Sun, Sep 06, 2009 at 02:29:03PM +0200, Felix Zielcke wrote:
>>> Currently grub-mkconfig uses chmod 444 on the newly generated grub.cfg
>>> Wouldn't it be better to use 400 now that we have plaintext password
>>> support?
>>> Or should we add support for a GRUB_CHMOD variable so users can override
>>> this setting as they please?
>>
>> I'd prefer to see this done only if they set a password. A GRUB_CHMOD
>> variable seems overkill, though.
> Is there a reason a non-root would like to look at grub.cfg on
> production system? Developers can always override chmod. If there is
> no real reason for non-root to look into grub.cfg I would follow the
> best friend in security considerations called "paranoia" and just use
> mode 400

Shouldn't it be u+rw anyway, or 0600 ?

Re: chmod of generated grub.cfg

[Felix Zielcke]

Am Sonntag, den 06.09.2009, 20:21 -0500 schrieb richardvoigt@gmail.com:
> On Sun, Sep 6, 2009 at 12:22 PM, Vladimir 'phcoder'
> Serbinenko<phcoder@gmail.com> wrote:
> > On Sun, Sep 6, 2009 at 3:38 PM, Colin Watson<cjwatson@ubuntu.com> wrote:
> >> On Sun, Sep 06, 2009 at 02:29:03PM +0200, Felix Zielcke wrote:
> >>> Currently grub-mkconfig uses chmod 444 on the newly generated grub.cfg
> >>> Wouldn't it be better to use 400 now that we have plaintext password
> >>> support?
> >>> Or should we add support for a GRUB_CHMOD variable so users can override
> >>> this setting as they please?
> >>
> >> I'd prefer to see this done only if they set a password. A GRUB_CHMOD
> >> variable seems overkill, though.
> > Is there a reason a non-root would like to look at grub.cfg on
> > production system? Developers can always override chmod. If there is
> > no real reason for non-root to look into grub.cfg I would follow the
> > best friend in security considerations called "paranoia" and just use
> > mode 400
> 
> Shouldn't it be u+rw anyway, or 0600 ?

I don't think this would be good.
The next grub-mkconfig run would just overwrite your changes you made
directly in grub.cfg
If any admin wants to do this nevertheless the warning at the top of it,
then he can also just change the mode or use with vim for example `:w!'


-- 
Felix Zielcke
Proud Debian Maintainer

Re: chmod of generated grub.cfg

[Robert Millan]

On Sun, Sep 06, 2009 at 07:22:36PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> On Sun, Sep 6, 2009 at 3:38 PM, Colin Watson<cjwatson@ubuntu.com> wrote:
> > On Sun, Sep 06, 2009 at 02:29:03PM +0200, Felix Zielcke wrote:
> >> Currently grub-mkconfig uses chmod 444 on the newly generated grub.cfg
> >> Wouldn't it be better to use 400 now that we have plaintext password
> >> support?
> >> Or should we add support for a GRUB_CHMOD variable so users can override
> >> this setting as they please?
> >
> > I'd prefer to see this done only if they set a password. A GRUB_CHMOD
> > variable seems overkill, though.
> Is there a reason a non-root would like to look at grub.cfg on
> production system? Developers can always override chmod. If there is
> no real reason for non-root to look into grub.cfg I would follow the
> best friend in security considerations called "paranoia" and just use
> mode 400

I like the idea of using 0400 right away, for simplicity.

OTOH, world-readable grub.cfg is useful, at least in Debian, because
reportbug includes this file in bug reports.

But if it's only useful for Debian, we shouldn't let this change our
agenda (ah, the conflict of wearing two hats...).

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: chmod of generated grub.cfg

[Felix Zielcke]

Am Dienstag, den 08.09.2009, 16:48 +0200 schrieb Robert Millan:
> On Sun, Sep 06, 2009 at 07:22:36PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> > On Sun, Sep 6, 2009 at 3:38 PM, Colin Watson<cjwatson@ubuntu.com> wrote:
> > > On Sun, Sep 06, 2009 at 02:29:03PM +0200, Felix Zielcke wrote:
> > >> Currently grub-mkconfig uses chmod 444 on the newly generated grub.cfg
> > >> Wouldn't it be better to use 400 now that we have plaintext password
> > >> support?
> > >> Or should we add support for a GRUB_CHMOD variable so users can override
> > >> this setting as they please?
> > >
> > > I'd prefer to see this done only if they set a password. A GRUB_CHMOD
> > > variable seems overkill, though.
> > Is there a reason a non-root would like to look at grub.cfg on
> > production system? Developers can always override chmod. If there is
> > no real reason for non-root to look into grub.cfg I would follow the
> > best friend in security considerations called "paranoia" and just use
> > mode 400
> 
> I like the idea of using 0400 right away, for simplicity.
> 
> OTOH, world-readable grub.cfg is useful, at least in Debian, because
> reportbug includes this file in bug reports.
> 
> But if it's only useful for Debian, we shouldn't let this change our
> agenda (ah, the conflict of wearing two hats...).
> 

So in upstream we change it to 400 + warning and for Debian we use my
last patch?

-- 
Felix Zielcke
Proud Debian Maintainer

Re: chmod of generated grub.cfg

[Robert Millan]

On Tue, Sep 08, 2009 at 04:51:41PM +0200, Felix Zielcke wrote:
> Am Dienstag, den 08.09.2009, 16:48 +0200 schrieb Robert Millan:
> > On Sun, Sep 06, 2009 at 07:22:36PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> > > On Sun, Sep 6, 2009 at 3:38 PM, Colin Watson<cjwatson@ubuntu.com> wrote:
> > > > On Sun, Sep 06, 2009 at 02:29:03PM +0200, Felix Zielcke wrote:
> > > >> Currently grub-mkconfig uses chmod 444 on the newly generated grub.cfg
> > > >> Wouldn't it be better to use 400 now that we have plaintext password
> > > >> support?
> > > >> Or should we add support for a GRUB_CHMOD variable so users can override
> > > >> this setting as they please?
> > > >
> > > > I'd prefer to see this done only if they set a password. A GRUB_CHMOD
> > > > variable seems overkill, though.
> > > Is there a reason a non-root would like to look at grub.cfg on
> > > production system? Developers can always override chmod. If there is
> > > no real reason for non-root to look into grub.cfg I would follow the
> > > best friend in security considerations called "paranoia" and just use
> > > mode 400
> > 
> > I like the idea of using 0400 right away, for simplicity.
> > 
> > OTOH, world-readable grub.cfg is useful, at least in Debian, because
> > reportbug includes this file in bug reports.
> > 
> > But if it's only useful for Debian, we shouldn't let this change our
> > agenda (ah, the conflict of wearing two hats...).
> > 
> 
> So in upstream we change it to 400 + warning and for Debian we use my
> last patch?

Ok.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: chmod of generated grub.cfg

[Felix Zielcke]

Am Donnerstag, den 10.09.2009, 20:56 +0200 schrieb Robert Millan:
> On Tue, Sep 08, 2009 at 04:51:41PM +0200, Felix Zielcke wrote:
> > Am Dienstag, den 08.09.2009, 16:48 +0200 schrieb Robert Millan:
> > > On Sun, Sep 06, 2009 at 07:22:36PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> > > > On Sun, Sep 6, 2009 at 3:38 PM, Colin Watson<cjwatson@ubuntu.com> wrote:
> > > > > On Sun, Sep 06, 2009 at 02:29:03PM +0200, Felix Zielcke wrote:
> > > > >> Currently grub-mkconfig uses chmod 444 on the newly generated grub.cfg
> > > > >> Wouldn't it be better to use 400 now that we have plaintext password
> > > > >> support?
> > > > >> Or should we add support for a GRUB_CHMOD variable so users can override
> > > > >> this setting as they please?
> > > > >
> > > > > I'd prefer to see this done only if they set a password. A GRUB_CHMOD
> > > > > variable seems overkill, though.
> > > > Is there a reason a non-root would like to look at grub.cfg on
> > > > production system? Developers can always override chmod. If there is
> > > > no real reason for non-root to look into grub.cfg I would follow the
> > > > best friend in security considerations called "paranoia" and just use
> > > > mode 400
> > > 
> > > I like the idea of using 0400 right away, for simplicity.
> > > 
> > > OTOH, world-readable grub.cfg is useful, at least in Debian, because
> > > reportbug includes this file in bug reports.
> > > 
> > > But if it's only useful for Debian, we shouldn't let this change our
> > > agenda (ah, the conflict of wearing two hats...).
> > > 
> > 
> > So in upstream we change it to 400 + warning and for Debian we use my
> > last patch?
> 
> Ok.
> 

Ok commited.

-- 
Felix Zielcke
Proud Debian Maintainer