Re: ospfd - always check config file permissions
On Thu, Jan 13, 2011 at 12:13:14AM +0100, Claudio Jeker wrote: On Wed, Jan 12, 2011 at 11:57:59PM +0100, Martin Pelikan wrote: Hello, this patch makes ospfd(8) and ospf6d(8) check its config file permissions even if run with a -n to test it. bgpd already behaves this way (changed 6 years ago by henning@) and it's quite handy to fix the permissions while doing tests, rather than at the first production boot time :-) Any comments? Appart from my desire to kill the permission checking? I don't see why bgpd and ospfd needs this non-unix like behaviour, other tools like pfctl do not care. We install the file with the correct permissions so if somebody changes them it is his fault. But this is just my opinion. For what it's worth this is already checked via mtree in /etc/security
Re: ospfd - always check config file permissions
On 2011/01/13 00:13, Claudio Jeker wrote: On Wed, Jan 12, 2011 at 11:57:59PM +0100, Martin Pelikan wrote: Hello, this patch makes ospfd(8) and ospf6d(8) check its config file permissions even if run with a -n to test it. bgpd already behaves this way (changed 6 years ago by henning@) and it's quite handy to fix the permissions while doing tests, rather than at the first production boot time :-) Any comments? Appart from my desire to kill the permission checking? I don't see why bgpd and ospfd needs this non-unix like behaviour, other tools like pfctl do not care. We install the file with the correct permissions so if somebody changes them it is his fault. But this is just my opinion. I don't like this check much. I usually work on a checked-out copy of my config files when I'm editing them so I often have to chmod before I bgpd -nvf bgpd.conf to check I haven't made a stupid typo before I commit and copy them out. It's inconsistent too: the control socket is group-writable for wheel, why should that be forbidden for the configuration file?
Re: ospfd - always check config file permissions
On Thu, Jan 13, 2011 at 02:48:04PM +, Stuart Henderson wrote: On 2011/01/13 00:13, Claudio Jeker wrote: On Wed, Jan 12, 2011 at 11:57:59PM +0100, Martin Pelikan wrote: Hello, this patch makes ospfd(8) and ospf6d(8) check its config file permissions even if run with a -n to test it. bgpd already behaves this way (changed 6 years ago by henning@) and it's quite handy to fix the permissions while doing tests, rather than at the first production boot time :-) Any comments? Appart from my desire to kill the permission checking? I don't see why bgpd and ospfd needs this non-unix like behaviour, other tools like pfctl do not care. We install the file with the correct permissions so if somebody changes them it is his fault. But this is just my opinion. I don't like this check much. I usually work on a checked-out copy of my config files when I'm editing them so I often have to chmod before I bgpd -nvf bgpd.conf to check I haven't made a stupid typo before I commit and copy them out. It's inconsistent too: the control socket is group-writable for wheel, why should that be forbidden for the configuration file? ospf6d.conf doesn't even contain secrets. I don't think it needs to be protected by file permissions. (ospfd.conf does contain secrets)
Re: ospfd - always check config file permissions
On Thu, Jan 13, 2011 at 04:02:47PM +0100, Henning Brauer wrote:
the check is dirt cheap, so that is not the point.
the aforementioned discussion is just being revived ;)
no problem then, here's the new one
--
Martin Pelikan
Index: parse.y
===
RCS file: /cvs/src/usr.sbin/bgpd/parse.y,v
retrieving revision 1.258
diff -u -p -r1.258 parse.y
--- parse.y 2 Sep 2010 14:03:21 - 1.258
+++ parse.y 13 Jan 2011 15:11:04 -
@@ -50,9 +50,8 @@ static struct file {
int lineno;
int errors;
} *file, *topfile;
-struct file*pushfile(const char *, int);
+struct file*pushfile(const char *);
int popfile(void);
-int check_file_secrecy(int, const char *);
int yyparse(void);
int yylex(void);
int yyerror(const char *, ...);
@@ -312,7 +311,7 @@ varset : STRING '=' string {
include: INCLUDE STRING{
struct file *nfile;
- if ((nfile = pushfile($2, 1)) == NULL) {
+ if ((nfile = pushfile($2)) == NULL) {
yyerror(failed to include file %s, $2);
free($2);
YYERROR;
@@ -2471,28 +2470,8 @@ nodigits:
return (c);
}
-int
-check_file_secrecy(int fd, const char *fname)
-{
- struct stat st;
-
- if (fstat(fd, st)) {
- log_warn(cannot stat %s, fname);
- return (-1);
- }
- if (st.st_uid != 0 st.st_uid != getuid()) {
- log_warnx(%s: owner not root or current user, fname);
- return (-1);
- }
- if (st.st_mode (S_IRWXG | S_IRWXO)) {
- log_warnx(%s: group/world readable/writeable, fname);
- return (-1);
- }
- return (0);
-}
-
struct file *
-pushfile(const char *name, int secret)
+pushfile(const char *name)
{
struct file *nfile;
@@ -2511,13 +2490,6 @@ pushfile(const char *name, int secret)
free(nfile);
return (NULL);
}
- if (secret
- check_file_secrecy(fileno(nfile-stream), nfile-name)) {
- fclose(nfile-stream);
- free(nfile-name);
- free(nfile);
- return (NULL);
- }
nfile-lineno = 1;
TAILQ_INSERT_TAIL(files, nfile, entry);
return (nfile);
@@ -2558,7 +2530,7 @@ parse_config(char *filename, struct bgpd
conf-opts = xconf-opts;
conf-csock = strdup(SOCKET_NAME);
- if ((file = pushfile(filename, 1)) == NULL) {
+ if ((file = pushfile(filename)) == NULL) {
free(conf);
return (-1);
}
Index: parse.y
===
RCS file: /cvs/src/usr.sbin/ospfd/parse.y,v
retrieving revision 1.73
diff -u -p -r1.73 parse.y
--- parse.y 13 Dec 2010 13:43:37 - 1.73
+++ parse.y 13 Jan 2011 15:12:02 -
@@ -50,9 +50,8 @@ static struct file {
int lineno;
int errors;
} *file, *topfile;
-struct file*pushfile(const char *, int);
+struct file*pushfile(const char *);
int popfile(void);
-int check_file_secrecy(int, const char *);
int yyparse(void);
int yylex(void);
int yyerror(const char *, ...);
@@ -149,7 +148,7 @@ grammar : /* empty */
include: INCLUDE STRING{
struct file *nfile;
- if ((nfile = pushfile($2, 1)) == NULL) {
+ if ((nfile = pushfile($2)) == NULL) {
yyerror(failed to include file %s, $2);
free($2);
YYERROR;
@@ -999,28 +998,8 @@ nodigits:
return (c);
}
-int
-check_file_secrecy(int fd, const char *fname)
-{
- struct stat st;
-
- if (fstat(fd, st)) {
- log_warn(cannot stat %s, fname);
- return (-1);
- }
- if (st.st_uid != 0 st.st_uid != getuid()) {
- log_warnx(%s: owner not root or current user, fname);
- return (-1);
- }
- if (st.st_mode (S_IRWXG | S_IRWXO)) {
- log_warnx(%s: group/world readable/writeable, fname);
- return (-1);
- }
- return (0);
-}
-
struct file *
-pushfile(const char *name, int secret)
+pushfile(const char *name)
{
struct file *nfile;
@@ -1038,12 +1017,6 @@ pushfile(const char *name, int secret)
free(nfile-name);
free(nfile);
return (NULL);
- } else if (secret
- check_file_secrecy(fileno(nfile-stream),
ospfd - always check config file permissions
Hello,
this patch makes ospfd(8) and ospf6d(8) check its config file permissions
even if run with a -n to test it. bgpd already behaves this way (changed
6 years ago by henning@) and it's quite handy to fix the permissions while
doing tests, rather than at the first production boot time :-)
Any comments?
--
Martin Pelikan
Index: parse.y
===
RCS file: /cvs/src/usr.sbin/ospf6d/parse.y,v
retrieving revision 1.20
diff -u -p -r1.20 parse.y
--- parse.y 13 Dec 2010 13:43:37 - 1.20
+++ parse.y 12 Jan 2011 22:23:36 -
@@ -887,7 +887,7 @@ parse_config(char *filename, int opts)
conf-spf_hold_time = DEFAULT_SPF_HOLDTIME;
conf-spf_state = SPF_IDLE;
- if ((file = pushfile(filename, !(conf-opts OSPFD_OPT_NOACTION))) ==
NULL) {
+ if ((file = pushfile(filename, 1)) == NULL) {
free(conf);
return (NULL);
}
Index: parse.y
===
RCS file: /cvs/src/usr.sbin/ospfd/parse.y,v
retrieving revision 1.73
diff -u -p -r1.73 parse.y
--- parse.y 13 Dec 2010 13:43:37 - 1.73
+++ parse.y 12 Jan 2011 22:23:51 -
@@ -1092,7 +1092,7 @@ parse_config(char *filename, int opts)
conf-spf_hold_time = DEFAULT_SPF_HOLDTIME;
conf-spf_state = SPF_IDLE;
- if ((file = pushfile(filename, !(conf-opts OSPFD_OPT_NOACTION))) ==
NULL) {
+ if ((file = pushfile(filename, 1)) == NULL) {
free(conf);
return (NULL);
}
Re: ospfd - always check config file permissions
On Wed, Jan 12, 2011 at 11:57:59PM +0100, Martin Pelikan wrote:
Hello,
this patch makes ospfd(8) and ospf6d(8) check its config file permissions
even if run with a -n to test it. bgpd already behaves this way (changed
6 years ago by henning@) and it's quite handy to fix the permissions while
doing tests, rather than at the first production boot time :-)
Any comments?
Appart from my desire to kill the permission checking?
I don't see why bgpd and ospfd needs this non-unix like behaviour,
other tools like pfctl do not care. We install the file with the correct
permissions so if somebody changes them it is his fault. But this is just
my opinion.
--
Martin Pelikan
Index: parse.y
===
RCS file: /cvs/src/usr.sbin/ospf6d/parse.y,v
retrieving revision 1.20
diff -u -p -r1.20 parse.y
--- parse.y 13 Dec 2010 13:43:37 - 1.20
+++ parse.y 12 Jan 2011 22:23:36 -
@@ -887,7 +887,7 @@ parse_config(char *filename, int opts)
conf-spf_hold_time = DEFAULT_SPF_HOLDTIME;
conf-spf_state = SPF_IDLE;
- if ((file = pushfile(filename, !(conf-opts OSPFD_OPT_NOACTION))) ==
NULL) {
+ if ((file = pushfile(filename, 1)) == NULL) {
free(conf);
return (NULL);
}
Index: parse.y
===
RCS file: /cvs/src/usr.sbin/ospfd/parse.y,v
retrieving revision 1.73
diff -u -p -r1.73 parse.y
--- parse.y 13 Dec 2010 13:43:37 - 1.73
+++ parse.y 12 Jan 2011 22:23:51 -
@@ -1092,7 +1092,7 @@ parse_config(char *filename, int opts)
conf-spf_hold_time = DEFAULT_SPF_HOLDTIME;
conf-spf_state = SPF_IDLE;
- if ((file = pushfile(filename, !(conf-opts OSPFD_OPT_NOACTION))) ==
NULL) {
+ if ((file = pushfile(filename, 1)) == NULL) {
free(conf);
return (NULL);
}
--
:wq Claudio
Re: ospfd - always check config file permissions
On Thu, Jan 13, 2011 at 12:13:14AM +0100, Claudio Jeker wrote: Appart from my desire to kill the permission checking? I don't see why bgpd and ospfd needs this non-unix like behaviour, other tools like pfctl do not care. We install the file with the correct permissions so if somebody changes them it is his fault. But this is just my opinion. Personally, I don't really care about the check. I saw bgpd notify me, so I fixed that and then both ospfd's told me they were OK. Obviously the errors followed next boot. I'd just want the behavior to be consistent and the information configuration OK to keep its meaning up. Because I found in CVS log that Henning and Theo were discussing that earlier, I did it this way. After all, it doesn't look like an expensive check, or is it? -- Martin Pelikan
