On 2017/12/04 17:36, Sebastien Marie wrote:
> On Mon, Dec 04, 2017 at 11:41:10AM +0100, Sebastien Marie wrote:
> > Hi,
> >
> > ISSUE: "Regression: rrdtool creates folders/files with arbitrary
> > permissions"
> > https://github.com/oetiker/rrdtool-1.x/issues/794
> >
> > FIX: "Remove all occurances of umask ... this is NOT thread safe! Fix for
> > #794"
> >
> > https://github.com/oetiker/rrdtool-1.x/commit/f1edd121add94fe69ac22d991748d289b5eb76ae
> >
> > The following diff is the fix applied on 1.7.0.
> >
>
> updated diff. It looks like my local repository wasn't up-to-date.
>
> Sorry about that.
Thanks - there are some slight problems with the upstream commit,
> --
> Sebastien Marie
>
> Index: Makefile
> ===================================================================
> RCS file: /cvs/ports/net/rrdtool/Makefile,v
> retrieving revision 1.107
> diff -u -p -r1.107 Makefile
> --- Makefile 1 Dec 2017 10:36:51 -0000 1.107
> +++ Makefile 4 Dec 2017 16:34:45 -0000
> @@ -12,7 +12,7 @@ PKGNAME-main= ${DISTNAME}
> PKGNAME-update= rrdupdate-${VERSION}
> PKGNAME-python= py-rrd-${VERSION}
> PKGNAME-ruby= ruby${MODRUBY_BINREV}-rrd-${VERSION}
> -REVISION-main= 2
> +REVISION-main= 3
> REVISION-update= 0
-update uses this code too. Not sure if it's built directly into the bindings
as well but I would just bump everything, it's cheap.
>
> SHARED_LIBS += rrd 5.2 # 9.0
> Index: patches/patch-doc_rrdcreate_pod
> ===================================================================
> RCS file: patches/patch-doc_rrdcreate_pod
> diff -N patches/patch-doc_rrdcreate_pod
> --- /dev/null 1 Jan 1970 00:00:00 -0000
> +++ patches/patch-doc_rrdcreate_pod 4 Dec 2017 16:34:45 -0000
> @@ -0,0 +1,19 @@
> +$OpenBSD$
> +Fix for https://github.com/oetiker/rrdtool-1.x/issues/794
> +https://github.com/oetiker/rrdtool-1.x/commit/f1edd121add94fe69ac22d991748d289b5eb76ae
> +Index: doc/rrdcreate.pod
> +--- doc/rrdcreate.pod.orig
> ++++ doc/rrdcreate.pod
> +@@ -743,6 +743,12 @@ divides each PDP of the AccumDuration by the correspon
> + TotalRequests and stores the average request duration. The remainder of the
> + RPN expression handles the divide by zero case.
> +
> ++=head1 SECURITY
> ++
> ++Note that new rrd files will have the permission 0622 regarless of your
> ++umask setting. If a file with the same name previously exists, its
> ++permission settings will be copied to the new file.
644 not 622. And if they're touching that, they can fix spelling of
"regardless" as well :)
> ++
> + =head1 AUTHORS
> +
> + Tobias Oetiker E<lt>[email protected]<gt>, Peter Stamfest
> E<lt>[email protected]<gt>
> Index: patches/patch-src_rrd_create_c
> ===================================================================
> RCS file: patches/patch-src_rrd_create_c
> diff -N patches/patch-src_rrd_create_c
> --- /dev/null 1 Jan 1970 00:00:00 -0000
> +++ patches/patch-src_rrd_create_c 4 Dec 2017 16:34:45 -0000
> @@ -0,0 +1,56 @@
> +$OpenBSD$
> +Fix for https://github.com/oetiker/rrdtool-1.x/issues/794
> +https://github.com/oetiker/rrdtool-1.x/commit/f1edd121add94fe69ac22d991748d289b5eb76ae
> +Index: src/rrd_create.c
> +--- src/rrd_create.c.orig
> ++++ src/rrd_create.c
> +@@ -4,6 +4,7 @@
> + * rrd_create.c creates new rrds
> +
> *****************************************************************************/
> +
> ++#include "mutex.h"
Remnant of an earlier attempt at fixing?
> + #include <stdlib.h>
> + #include <time.h>
> + #include <locale.h>
> +@@ -1313,10 +1314,10 @@ done:
> + return rc;
> + }
> +
> ++
> + int write_rrd(const char *outfilename, rrd_t *out) {
> + int rc = -1;
> + char *tmpfilename = NULL;
> +- mode_t saved_umask;
> +
> + /* write out the new file */
> + #ifdef HAVE_LIBRADOS
> +@@ -1341,10 +1342,10 @@ int write_rrd(const char *outfilename, rrd_t *out) {
> + strcpy(tmpfilename, outfilename);
> + strcat(tmpfilename, "XXXXXX");
> +
> +- /* fix CWE-377 */
> +- saved_umask = umask(S_IRUSR|S_IWUSR);
> ++ /* this is 0600 according to the manual page */
> + int tmpfd = mkstemp(tmpfilename);
> +- umask(saved_umask);
> ++
> ++
> + if (tmpfd < 0) {
> + rrd_set_error("Cannot create temporary file");
> + goto done;
> +@@ -1377,13 +1378,8 @@ int write_rrd(const char *outfilename, rrd_t *out) {
> + stat_buf.st_mode = _S_IREAD | _S_IWRITE; // have to test
> it is
> + #else
> + /* an error occurred (file not found, maybe?). Anyway:
> +- set the mode to 0666 using current umask */
> +- stat_buf.st_mode = S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP |
> S_IROTH | S_IWOTH;
> +-
> +- mode_t mask = umask(0);
> +- umask(mask);
> +-
> +- stat_buf.st_mode &= ~mask;
> ++ set the mode to 0644 using current umask */
> ++ stat_buf.st_mode = S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH;
The "using current umask" comment here is wrong.
> + #endif
> + }
> + if (chmod(tmpfilename, stat_buf.st_mode) != 0) {
I propose just doing this for now. What do you think?
Index: Makefile
===================================================================
RCS file: /cvs/ports/net/rrdtool/Makefile,v
retrieving revision 1.107
diff -u -p -r1.107 Makefile
--- Makefile 1 Dec 2017 10:36:51 -0000 1.107
+++ Makefile 5 Dec 2017 10:50:58 -0000
@@ -12,8 +12,7 @@ PKGNAME-main= ${DISTNAME}
PKGNAME-update= rrdupdate-${VERSION}
PKGNAME-python= py-rrd-${VERSION}
PKGNAME-ruby= ruby${MODRUBY_BINREV}-rrd-${VERSION}
-REVISION-main= 2
-REVISION-update= 0
+REVISION= 4
SHARED_LIBS += rrd 5.2 # 9.0
Index: patches/patch-src_rrd_create_c
===================================================================
RCS file: patches/patch-src_rrd_create_c
diff -N patches/patch-src_rrd_create_c
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ patches/patch-src_rrd_create_c 5 Dec 2017 10:50:58 -0000
@@ -0,0 +1,50 @@
+$OpenBSD$
+
+Based on:
+
+From f1edd121add94fe69ac22d991748d289b5eb76ae Mon Sep 17 00:00:00 2001
+From: Tobias Oetiker <[email protected]>
+Date: Sun, 11 Jun 2017 17:19:05 +0200
+Subject: [PATCH] Remove all occurances of umask ... this is NOT thread safe!
+ Fix for #794.
+
+Index: src/rrd_create.c
+--- src/rrd_create.c.orig
++++ src/rrd_create.c
+@@ -1316,7 +1316,6 @@ done:
+ int write_rrd(const char *outfilename, rrd_t *out) {
+ int rc = -1;
+ char *tmpfilename = NULL;
+- mode_t saved_umask;
+
+ /* write out the new file */
+ #ifdef HAVE_LIBRADOS
+@@ -1341,10 +1340,9 @@ int write_rrd(const char *outfilename, rrd_t *out) {
+ strcpy(tmpfilename, outfilename);
+ strcat(tmpfilename, "XXXXXX");
+
+- /* fix CWE-377 */
+- saved_umask = umask(S_IRUSR|S_IWUSR);
++ /* this is 0600 according to the manual page */
+ int tmpfd = mkstemp(tmpfilename);
+- umask(saved_umask);
++
+ if (tmpfd < 0) {
+ rrd_set_error("Cannot create temporary file");
+ goto done;
+@@ -1377,13 +1375,8 @@ int write_rrd(const char *outfilename, rrd_t *out) {
+ stat_buf.st_mode = _S_IREAD | _S_IWRITE; // have to test it
is
+ #else
+ /* an error occurred (file not found, maybe?). Anyway:
+- set the mode to 0666 using current umask */
+- stat_buf.st_mode = S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP |
S_IROTH | S_IWOTH;
+-
+- mode_t mask = umask(0);
+- umask(mask);
+-
+- stat_buf.st_mode &= ~mask;
++ set the mode to 0644 */
++ stat_buf.st_mode = S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH;
+ #endif
+ }
+ if (chmod(tmpfilename, stat_buf.st_mode) != 0) {
