Permalink
Browse files

core: be more paranoid when mixing umask and fopen()

Let's be extra careful with the umask when we use simple fopen(), as this
creates files with 0777 by default.
  • Loading branch information...
poettering committed Apr 7, 2016
1 parent 4f4afc8 commit 8612da973d30c5a9530fa1b6b3d449147b5a3324
Showing with 7 additions and 4 deletions.
  1. +3 −1 src/basic/util.c
  2. +1 −2 src/core/machine-id-setup.c
  3. +3 −1 src/core/main.c
View
@@ -55,6 +55,7 @@
#include "string-util.h"
#include "strv.h"
#include "time-util.h"
#include "umask-util.h"
#include "user-util.h"
#include "util.h"
@@ -781,7 +782,8 @@ int update_reboot_param_file(const char *param) {
int r = 0;
if (param) {
r = write_string_file(REBOOT_PARAM_FILE, param, WRITE_STRING_FILE_CREATE);
RUN_WITH_UMASK(0022)
r = write_string_file(REBOOT_PARAM_FILE, param, WRITE_STRING_FILE_CREATE);
if (r < 0)
return log_error_errno(r, "Failed to write reboot param to "REBOOT_PARAM_FILE": %m");
} else
@@ -259,9 +259,8 @@ int machine_id_setup(const char *root, sd_id128_t machine_id) {
/* Hmm, we couldn't write it? So let's write it to
* /run/machine-id as a replacement */
RUN_WITH_UMASK(0022) {
RUN_WITH_UMASK(0022)
r = write_string_file(run_machine_id, id, WRITE_STRING_FILE_CREATE);
}
if (r < 0) {
(void) unlink(run_machine_id);
return log_error_errno(r, "Cannot write %s: %m", run_machine_id);
View
@@ -81,6 +81,7 @@
#include "strv.h"
#include "switch-root.h"
#include "terminal-util.h"
#include "umask-util.h"
#include "user-util.h"
#include "virt.h"
#include "watchdog.h"
@@ -1237,7 +1238,8 @@ static int write_container_id(void) {
if (isempty(c))
return 0;
r = write_string_file("/run/systemd/container", c, WRITE_STRING_FILE_CREATE);
RUN_WITH_UMASK(0022)
r = write_string_file("/run/systemd/container", c, WRITE_STRING_FILE_CREATE);
if (r < 0)
return log_warning_errno(r, "Failed to write /run/systemd/container, ignoring: %m");

5 comments on commit 8612da9

@calimeroteknik

This comment has been minimized.

Show comment
Hide comment
@calimeroteknik

calimeroteknik Sep 29, 2016

Creating files with 0777 mode by default is practically begging for problems and should be changed by removing this:

/* Disable the umask logic */

If that's not changed, you will, again in the future, end up with unnoticeable and critical vulnerabilities like this one when you forget to use RUN_WITH_UMASK().

It would be so much safer to get something that doesn't work, and that you would notice and fix immediately as you test.

calimeroteknik replied Sep 29, 2016

Creating files with 0777 mode by default is practically begging for problems and should be changed by removing this:

/* Disable the umask logic */

If that's not changed, you will, again in the future, end up with unnoticeable and critical vulnerabilities like this one when you forget to use RUN_WITH_UMASK().

It would be so much safer to get something that doesn't work, and that you would notice and fix immediately as you test.

@poettering

This comment has been minimized.

Show comment
Hide comment
@poettering
Member

poettering replied Oct 7, 2016

For an explanation why umask() is broken for system services see this mail:

https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/message/FZZAHFGZCSYEVUVWQ6LZM43U5TWQWYDZ/

@calimeroteknik

This comment has been minimized.

Show comment
Hide comment
@calimeroteknik

calimeroteknik Oct 7, 2016

If disabling the umask logic is required, why not consider other ways to get a safe-by-default architecture?

Given your reasons:

a number of library calls and syscalls don't allow passing the file mode for nodes in the file system that shall be created

…then why wouldn't you integrate the umask shenanigans that cope with this into the helpers then? (e.g. it would be part of the work done by write_string_file procedure for our current case)

Which would also lead to a safe-by-default code architecture too, as I see it.
I really have no other motivation here than preventing future human error.
File modes are worth it.

calimeroteknik replied Oct 7, 2016

If disabling the umask logic is required, why not consider other ways to get a safe-by-default architecture?

Given your reasons:

a number of library calls and syscalls don't allow passing the file mode for nodes in the file system that shall be created

…then why wouldn't you integrate the umask shenanigans that cope with this into the helpers then? (e.g. it would be part of the work done by write_string_file procedure for our current case)

Which would also lead to a safe-by-default code architecture too, as I see it.
I really have no other motivation here than preventing future human error.
File modes are worth it.

@poettering

This comment has been minimized.

Show comment
Hide comment
@poettering

poettering Oct 7, 2016

Member

@calimeroteknik i'd be happy to take a patch that adds a few helpers: bind_with_mode(), fopen_with_mode() and so on, as calls that take an additional mode_t for the files to create. I'd also be happy to take a patch that extends write_string_file() to take an additional mode_t

Member

poettering replied Oct 7, 2016

@calimeroteknik i'd be happy to take a patch that adds a few helpers: bind_with_mode(), fopen_with_mode() and so on, as calls that take an additional mode_t for the files to create. I'd also be happy to take a patch that extends write_string_file() to take an additional mode_t

@evverx

This comment has been minimized.

Show comment
Hide comment
@evverx

evverx Oct 7, 2016

Member

I really have no other motivation here than preventing future human error

that take an additional mode_t for the files to create

I think this doesn't prevent human errors

and so on

Member

evverx replied Oct 7, 2016

I really have no other motivation here than preventing future human error

that take an additional mode_t for the files to create

I think this doesn't prevent human errors

and so on

Please sign in to comment.