[PATCH] mkswap: Add warnings for insecure device permissions/owners

[PATCH] mkswap: Add warnings for insecure device permissions/owners

[Wayne R. Roth]

Logic copied from sys-utils/swapon.c

Signed-off-by: Wayne R. Roth <wayneroth42@gmail.com>
---
 disk-utils/mkswap.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/disk-utils/mkswap.c b/disk-utils/mkswap.c
index c559e60..2729596 100644
--- a/disk-utils/mkswap.c
+++ b/disk-utils/mkswap.c
@@ -344,7 +344,7 @@ static void write_header_to_device(struct mkswap_control *ctl)
 int main(int argc, char **argv)
 {
 	struct mkswap_control ctl = { .fd = -1 };
-	int c;
+	int c, permMask;
 	uint64_t sz;
 	int version = SWAP_VERSION;
 	char *block_count = NULL, *strsz = NULL;
@@ -464,6 +464,15 @@ int main(int argc, char **argv)
 			ctl.devname);
 
 	open_device(&ctl);
+        permMask = S_ISBLK(ctl.devstat.st_mode) ? 07007 : 07077;
+        if ((ctl.devstat.st_mode & permMask) != 0)
+                warnx(_("%s: insecure permissions %04o, %04o suggested."),
+                                ctl.devname, ctl.devstat.st_mode & 07777,
+                                ~permMask & 0666);
+        if (S_ISREG(ctl.devstat.st_mode) && ctl.devstat.st_uid != 0)
+                warnx(_("%s: insecure file owner %d, 0 (root) suggested."),
+                                ctl.devname, ctl.devstat.st_uid);
+
 
 	if (ctl.check)
 		check_blocks(&ctl);
-- 
1.8.3.1

Re: [PATCH] mkswap: Add warnings for insecure device permissions/owners

[Mike Frysinger]

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

On 19 Jan 2016 10:37, Wayne R. Roth wrote:
> +        permMask = S_ISBLK(ctl.devstat.st_mode) ? 07007 : 07077;
> +        if ((ctl.devstat.st_mode & permMask) != 0)
> +                warnx(_("%s: insecure permissions %04o, %04o suggested."),
> +                                ctl.devname, ctl.devstat.st_mode & 07777,
> +                                ~permMask & 0666);
> +        if (S_ISREG(ctl.devstat.st_mode) && ctl.devstat.st_uid != 0)
> +                warnx(_("%s: insecure file owner %d, 0 (root) suggested."),
> +                                ctl.devname, ctl.devstat.st_uid);

i haven't read/tested the code, so my assumptions might be off, but this
seems to complain even when creating files as non-root.  mkswap should
not do that.  a perfectly reasonable use case is to create images as a
non-root user for use with something like qemu.

maybe you want to add a getuid() check in there, or scuttle it altogether.
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH] mkswap: Add warnings for insecure device permissions/owners Logic modified from sys-utils/swapon.c

[Wayne R. Roth]

From: "Wayne R. Roth" <wayneroth42@gmail.com>

Signed-off-by: Wayne R. Roth <wayneroth42@gmail.com>
---
 disk-utils/mkswap.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/disk-utils/mkswap.c b/disk-utils/mkswap.c
index c559e60..0a8dc05 100644
--- a/disk-utils/mkswap.c
+++ b/disk-utils/mkswap.c
@@ -344,7 +344,7 @@ static void write_header_to_device(struct mkswap_control *ctl)
 int main(int argc, char **argv)
 {
 	struct mkswap_control ctl = { .fd = -1 };
-	int c;
+	int c, permMask;
 	uint64_t sz;
 	int version = SWAP_VERSION;
 	char *block_count = NULL, *strsz = NULL;
@@ -464,6 +464,15 @@ int main(int argc, char **argv)
 			ctl.devname);
 
 	open_device(&ctl);
+        permMask = S_ISBLK(ctl.devstat.st_mode) ? 07007 : 07077;
+        if ((ctl.devstat.st_mode & permMask) != 0)
+                warnx(_("%s: insecure permissions %04o, %04o suggested."),
+                                ctl.devname, ctl.devstat.st_mode & 07777,
+                                ~permMask & 0666);
+        if (getuid()==0 && S_ISREG(ctl.devstat.st_mode) && ctl.devstat.st_uid != 0)
+                warnx(_("%s: insecure file owner %d, 0 (root) suggested."),
+                                ctl.devname, ctl.devstat.st_uid);
+
 
 	if (ctl.check)
 		check_blocks(&ctl);
-- 
1.8.3.1

Re: [PATCH] mkswap: Add warnings for insecure device permissions/owners Logic modified from sys-utils/swapon.c

[Mike Frysinger]

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

On 19 Jan 2016 20:17, Wayne R. Roth wrote:
> +        permMask = S_ISBLK(ctl.devstat.st_mode) ? 07007 : 07077;

pretty sure you want to indent w/tabs, not spaces

> +        if (getuid()==0 && S_ISREG(ctl.devstat.st_mode) && ctl.devstat.st_uid != 0)

there should be spaces around the ==
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH] mkswap: add warnings for insecure device permissions/owners Logic modified from sys-utils/swapon.c

[Wayne R. Roth]

From: "Wayne R. Roth" <wayneroth42@gmail.com>

Signed-off-by: Wayne R. Roth <wayneroth42@gmail.com>
---
 disk-utils/mkswap.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/disk-utils/mkswap.c b/disk-utils/mkswap.c
index c559e60..22999da 100644
--- a/disk-utils/mkswap.c
+++ b/disk-utils/mkswap.c
@@ -344,7 +344,7 @@ static void write_header_to_device(struct mkswap_control *ctl)
 int main(int argc, char **argv)
 {
 	struct mkswap_control ctl = { .fd = -1 };
-	int c;
+	int c, permMask;
 	uint64_t sz;
 	int version = SWAP_VERSION;
 	char *block_count = NULL, *strsz = NULL;
@@ -464,6 +464,15 @@ int main(int argc, char **argv)
 			ctl.devname);
 
 	open_device(&ctl);
+	permMask = S_ISBLK(ctl.devstat.st_mode) ? 07007 : 07077;
+	if ((ctl.devstat.st_mode & permMask) != 0)
+		warnx(_("%s: insecure permissions %04o, %04o suggested."),
+			ctl.devname, ctl.devstat.st_mode & 07777,
+			~permMask & 0666);
+	if (getuid() == 0 && S_ISREG(ctl.devstat.st_mode) && ctl.devstat.st_uid != 0)
+		warnx(_("%s: insecure file owner %d, 0 (root) suggested."),
+			ctl.devname, ctl.devstat.st_uid);
+
 
 	if (ctl.check)
 		check_blocks(&ctl);
-- 
1.8.3.1

Re: [PATCH] mkswap: Add warnings for insecure device permissions/owners

[Sami Kerola]

On 19 January 2016 at 19:44, Mike Frysinger <vapier@gentoo.org> wrote:
> On 19 Jan 2016 10:37, Wayne R. Roth wrote:
>> +        permMask = S_ISBLK(ctl.devstat.st_mode) ? 07007 : 07077;
>> +        if ((ctl.devstat.st_mode & permMask) != 0)
>> +                warnx(_("%s: insecure permissions %04o, %04o suggested."),
>> +                                ctl.devname, ctl.devstat.st_mode & 07777,
>> +                                ~permMask & 0666);
>> +        if (S_ISREG(ctl.devstat.st_mode) && ctl.devstat.st_uid != 0)
>> +                warnx(_("%s: insecure file owner %d, 0 (root) suggested."),
>> +                                ctl.devname, ctl.devstat.st_uid);
>
> i haven't read/tested the code, so my assumptions might be off, but this
> seems to complain even when creating files as non-root.  mkswap should
> not do that.  a perfectly reasonable use case is to create images as a
> non-root user for use with something like qemu.
>
> maybe you want to add a getuid() check in there, or scuttle it altogether.

I do not think mkswap, or mkfs of any sort, is an operation only root is
allowed or expected to run. Taking device(s) in use is different matter,
and that is already covered in swapon(8) and mount(8).

-- 
Sami Kerola
http://www.iki.fi/kerolasa/

Re: [PATCH] mkswap: Add warnings for insecure device permissions/owners

[Karel Zak]

On Tue, Jan 19, 2016 at 10:37:06AM -0800, Wayne R. Roth wrote:
> Logic copied from sys-utils/swapon.c

Why? I think swapon is the right place for this check.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: Re: [PATCH] mkswap: Add warnings for insecure device permissions/owners

[Sarah Newman]

On 01/20/2016 02:30 AM, Karel Zak wrote:
> On Tue, Jan 19, 2016 at 10:37:06AM -0800, Wayne R. Roth wrote:
>> Logic copied from sys-utils/swapon.c
> 
> Why? I think swapon is the right place for this check.
> 
>     Karel
> 

Hi Karel,

Warnings are probably best put in *both* mkswap and swapon for the following two reasons:

1. The person(s) reviewing the output for swapon may not be the same person(s) reviewing the output for mkswap. For example, this might happen with a
company with a separate development and quality assurance department.

2. To my best knowledge the release of mkswap and swapon do not have to match. An example of when this might happen is the build process for an
embedded device or virtual machine. I am pretty sure busybox does not warn on world readable swap right now.

This patch does not break any existing behavior. The worst case possibility from accepting this patch is it will annoy some people, and best case it
will save millions of devices from being shipped with insecure permissions.

Thanks, Sarah

Re: [PATCH] mkswap: Add warnings for insecure device permissions/owners

[Tilman Schmidt]

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

Am 21.01.2016 um 23:19 schrieb Sarah Newman:
> This patch does not break any existing behavior. The worst case possibility from accepting this patch is it will annoy some people, and best case it
> will save millions of devices from being shipped with insecure permissions.

The worst case is it will train millions of administrators to ignore
warning messages.

-- 
Tilman Schmidt                              E-Mail: tilman@imap.cc
Bonn, Germany
Nous, on a des fleurs et des bougies pour nous protéger.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] mkswap: Add warnings for insecure device permissions/owners

[Sarah Newman]

On 01/22/2016 08:01 AM, Tilman Schmidt wrote:
> Am 21.01.2016 um 23:19 schrieb Sarah Newman:
>> This patch does not break any existing behavior. The worst case possibility from accepting this patch is it will annoy some people, and best case it
>> will save millions of devices from being shipped with insecure permissions.
> 
> The worst case is it will train millions of administrators to ignore
> warning messages.
> 

If the warnings in swapon are legitimate, they are just as legitimate in mkswap if the file owner check is only done when mkswap is run as root.

Regarding the legitimacy of the swapon warnings: do you honestly believe most of the people who will get these warnings will have intended to have
world readable swap or swap owned as a non-root owner?

When I search for "linux swap file" on google this is the second hit for me, the first being an arch linux wiki page:
https://www.linux.com/news/software/applications/8208-all-about-linux-swap-space "centos swap file" top two hits
https://www.centos.org/docs/5/html/5.2/Deployment_Guide/s2-swap-creating-file.html
https://www.centos.org/docs/5/html/Deployment_Guide-en-US/s1-swap-adding.html

I followed the instructions for CentOS on a CentOS 5 machine and it resulted in world readable swap. Those instructions came from Red Hat. If
documentation from Red Hat gets it wrong, it's presumably a very common error. I made this mistake myself and I knew better.

--Sarah

Re: [PATCH] mkswap: Add warnings for insecure device permissions/owners

[Sami Kerola]

On 22 January 2016 at 19:14, Sarah Newman <srn@prgmr.com> wrote:
> On 01/22/2016 08:01 AM, Tilman Schmidt wrote:
>> Am 21.01.2016 um 23:19 schrieb Sarah Newman:
>>> This patch does not break any existing behavior. The worst case possibility from accepting this patch is it will annoy some people, and best case it
>>> will save millions of devices from being shipped with insecure permissions.
>>
>> The worst case is it will train millions of administrators to ignore
>> warning messages.
>>
>
> If the warnings in swapon are legitimate, they are just as legitimate in mkswap if the file owner check is only done when mkswap is run as root.
>
> Regarding the legitimacy of the swapon warnings: do you honestly believe most of the people who will get these warnings will have intended to have
> world readable swap or swap owned as a non-root owner?
>
> When I search for "linux swap file" on google this is the second hit for me, the first being an arch linux wiki page:
> https://www.linux.com/news/software/applications/8208-all-about-linux-swap-space "centos swap file" top two hits
> https://www.centos.org/docs/5/html/5.2/Deployment_Guide/s2-swap-creating-file.html
> https://www.centos.org/docs/5/html/Deployment_Guide-en-US/s1-swap-adding.html
>
> I followed the instructions for CentOS on a CentOS 5 machine and it resulted in world readable swap. Those instructions came from Red Hat. If
> documentation from Red Hat gets it wrong, it's presumably a very common error. I made this mistake myself and I knew better.

Fair points, but should same logic be applied to file holes when
running mkswap? If the test _must_ be the same surely the mkswap &
swapon should share the function(s) performing them, as drifting would
be bad. Should these messages be optional, and if so which way around
default ought to be? This will lead to --quite or --verbose options.

Alternatively one could make swapon to get rid of all permission bits
and set ownership to UID 0 by default when ever it activates a
swapfile. How about that.

-- 
Sami Kerola
http://www.iki.fi/kerolasa/

Re: [PATCH] mkswap: Add warnings for insecure device permissions/owners

[Karel Zak]

On Fri, Jan 22, 2016 at 10:03:47PM +0000, Sami Kerola wrote:
> Alternatively one could make swapon to get rid of all permission bits
> and set ownership to UID 0 by default when ever it activates a
> swapfile. How about that.

Not sure if want to change any permissions on the fly, it would be
better to reject files (by swapon) with insecure permissions and
require something like --force for crazy users who wants to ignore
this problem.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH] mkswap: Add warnings for insecure device permissions/owners

[Sami Kerola]

On 23 January 2016 at 16:22, Karel Zak <kzak@redhat.com> wrote:
> On Fri, Jan 22, 2016 at 10:03:47PM +0000, Sami Kerola wrote:
>> Alternatively one could make swapon to get rid of all permission bits
>> and set ownership to UID 0 by default when ever it activates a
>> swapfile. How about that.
>
> Not sure if want to change any permissions on the fly, it would be
> better to reject files (by swapon) with insecure permissions and
> require something like --force for crazy users who wants to ignore
> this problem.

Why not completely optional?

$ swapon --path-permissions [ignore|complain|stop|fix]

Current default is 'complain', and it feels about right.

-- 
Sami Kerola
http://www.iki.fi/kerolasa/

Re: [PATCH] mkswap: Add warnings for insecure device permissions/owners

[Sami Kerola]

On 24 January 2016 at 11:09, Sami Kerola <kerolasa@iki.fi> wrote:
> On 23 January 2016 at 16:22, Karel Zak <kzak@redhat.com> wrote:
>> On Fri, Jan 22, 2016 at 10:03:47PM +0000, Sami Kerola wrote:
>>> Alternatively one could make swapon to get rid of all permission bits
>>> and set ownership to UID 0 by default when ever it activates a
>>> swapfile. How about that.
>>
>> Not sure if want to change any permissions on the fly, it would be
>> better to reject files (by swapon) with insecure permissions and
>> require something like --force for crazy users who wants to ignore
>> this problem.
>
> Why not completely optional?
>
> $ swapon --path-permissions [ignore|complain|stop|fix]
>
> Current default is 'complain', and it feels about right.

Something like this.

https://github.com/kerolasa/lelux-utiliteetit/commit/d79b3cdbe4e61b7e10d595dec4d4e299c8300c9e

Notice that the change above is wrote on top of another change, that
added swapon control structure.

https://github.com/kerolasa/lelux-utiliteetit/commit/4d510f769de438092558f51430f3b195f328ef0e

-- 
Sami Kerola
http://www.iki.fi/kerolasa/

Re: [PATCH] mkswap: Add warnings for insecure device permissions/owners

[Sarah Newman]

On 01/23/2016 08:22 AM, Karel Zak wrote:
> On Fri, Jan 22, 2016 at 10:03:47PM +0000, Sami Kerola wrote:
>> Alternatively one could make swapon to get rid of all permission bits
>> and set ownership to UID 0 by default when ever it activates a
>> swapfile. How about that.
> 
> Not sure if want to change any permissions on the fly, it would be
> better to reject files (by swapon) with insecure permissions and
> require something like --force for crazy users who wants to ignore
> this problem.

Rejecting insecure permissions in swapon will make warnings in mkswap even more important. Are there any further changes required to Wayne's latest
patch to mkswap.c https://marc.info/?l=util-linux-ng&m=145327019508709&w=2 before it is merged?

Thanks, Sarah

Re: [PATCH] mkswap: add warnings for insecure device permissions/owners Logic modified from sys-utils/swapon.c

[Karel Zak]

On Tue, Jan 19, 2016 at 10:09:37PM -0800, Wayne R. Roth wrote:
>  disk-utils/mkswap.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)

 Applied, although I don't feel 100% happy with it. At least we will
 educate users about the problem :-)

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH] mkswap: Add warnings for insecure device permissions/owners

[Karel Zak]

On Sun, Jan 24, 2016 at 11:09:47AM +0000, Sami Kerola wrote:
> On 23 January 2016 at 16:22, Karel Zak <kzak@redhat.com> wrote:
> > On Fri, Jan 22, 2016 at 10:03:47PM +0000, Sami Kerola wrote:
> >> Alternatively one could make swapon to get rid of all permission bits
> >> and set ownership to UID 0 by default when ever it activates a
> >> swapfile. How about that.
> >
> > Not sure if want to change any permissions on the fly, it would be
> > better to reject files (by swapon) with insecure permissions and
> > require something like --force for crazy users who wants to ignore
> > this problem.
> 
> Why not completely optional?
> 
> $ swapon --path-permissions [ignore|complain|stop|fix]

I don't think we want to merge another functionality to swapon. The
warnings are enough. For the rest we have ch{own,mod}. 

Let's Keep It Simple and Stupid. We all love kisses, right? :-)

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH] mkswap: Add warnings for insecure device permissions/owners

[Sarah Newman]

On 01/26/2016 02:42 AM, Karel Zak wrote:
> On Sun, Jan 24, 2016 at 11:09:47AM +0000, Sami Kerola wrote:
>> On 23 January 2016 at 16:22, Karel Zak <kzak@redhat.com> wrote:
>>> On Fri, Jan 22, 2016 at 10:03:47PM +0000, Sami Kerola wrote:
>>>> Alternatively one could make swapon to get rid of all permission bits
>>>> and set ownership to UID 0 by default when ever it activates a
>>>> swapfile. How about that.
>>>
>>> Not sure if want to change any permissions on the fly, it would be
>>> better to reject files (by swapon) with insecure permissions and
>>> require something like --force for crazy users who wants to ignore
>>> this problem.
>>
>> Why not completely optional?
>>
>> $ swapon --path-permissions [ignore|complain|stop|fix]
> 
> I don't think we want to merge another functionality to swapon. The
> warnings are enough. For the rest we have ch{own,mod}. 
> 
> Let's Keep It Simple and Stupid. We all love kisses, right? :-)

Hi Karel,

Your original suggestion for swapon to require '--force' for insecure permissions seems like the most sane thing to do - it protects the user without
adding a lot of knobs. Presumably there would need to a "force" option for fstab too.

But implementing that without advance notice could lead to broken systems. Maybe it would make sense to add the --force option now and change the
warning to indicate that in future versions of swapon, insecure permissions used without --force will be rejected. Then in a couple of years actually
implement that change.

If you agree this is sane behavior appropriate for upstream, I'll get you a patch for swapon.

--Sarah