does this patch make sense?
Just for extra paranoia's sake? Against 5.4 sources. -peter diff -u -p -u -r1.82 traceroute.c --- traceroute.c10 Feb 2012 23:05:54 - 1.82 +++ traceroute.c7 Nov 2013 14:36:44 - @@ -310,6 +310,7 @@ main(int argc, char *argv[]) const char *errstr; long l; uid_t uid; + gid_t gid; u_int rtableid; if ((s = socket(AF_INET, SOCK_RAW, IPPROTO_ICMP)) 0) @@ -319,6 +320,14 @@ main(int argc, char *argv[]) /* revoke privs */ uid = getuid(); + gid = getgid(); + + if (setgroups(1, gid) == -1) + err(1, setgroups); + + if (setresgid(gid, gid, gid) == -1) + err(1, setresgid); + if (setresuid(uid, uid, uid) == -1) err(1, setresuid);
Re: does this patch make sense?
On 11/07/13 15:41, Peter J. Philipp wrote:
Just for extra paranoia's sake? Against 5.4 sources.
-peter
diff -u -p -u -r1.82 traceroute.c
--- traceroute.c10 Feb 2012 23:05:54 - 1.82
+++ traceroute.c7 Nov 2013 14:36:44 -
@@ -310,6 +310,7 @@ main(int argc, char *argv[])
const char *errstr;
long l;
uid_t uid;
+ gid_t gid;
u_int rtableid;
if ((s = socket(AF_INET, SOCK_RAW, IPPROTO_ICMP)) 0)
@@ -319,6 +320,14 @@ main(int argc, char *argv[])
/* revoke privs */
uid = getuid();
+ gid = getgid();
+
+ if (setgroups(1, gid) == -1)
+ err(1, setgroups);
+
+ if (setresgid(gid, gid, gid) == -1)
+ err(1, setresgid);
+
if (setresuid(uid, uid, uid) == -1)
err(1, setresuid);
I thought about it and thought my patch didn't really do anything. So
then I added another piece that doesn't really do anything but saves CPU
cycles. I tested this with tcpdump and it seems to update the TOS
accordingly.
-peter
===
RCS file: /cvs/src/usr.sbin/traceroute/traceroute.c,v
retrieving revision 1.82
diff -u -p -u -r1.82 traceroute.c
--- traceroute.c10 Feb 2012 23:05:54 - 1.82
+++ traceroute.c7 Nov 2013 16:13:54 -
@@ -310,6 +310,7 @@ main(int argc, char *argv[])
const char *errstr;
long l;
uid_t uid;
+ gid_t gid;
u_int rtableid;
if ((s = socket(AF_INET, SOCK_RAW, IPPROTO_ICMP)) 0)
@@ -319,6 +320,14 @@ main(int argc, char *argv[])
/* revoke privs */
uid = getuid();
+ gid = getgid();
+
+ if (setgroups(1, gid) == -1)
+ err(1, setgroups);
+
+ if (setresgid(gid, gid, gid) == -1)
+ err(1, setresgid);
+
if (setresuid(uid, uid, uid) == -1)
err(1, setresuid);
@@ -1224,6 +1233,7 @@ int
map_tos(char *s, int *val)
{
/* DiffServ Codepoints and other TOS mappings */
+ /* KEEP SORTED */
const struct toskeywords {
const char *keyword;
int val;
@@ -1258,14 +1268,13 @@ map_tos(char *s, int *val)
{ NULL, -1 },
};
- for (t = toskeywords; t-keyword != NULL; t++) {
- if (strcmp(s, t-keyword) == 0) {
- *val = t-val;
- return (1);
- }
- }
+ t = bsearch(s, toskeywords, nitems(toskeywords), sizeof(struct
toskeywords), (int (*)(const void *, const void *))strcmp);
+ if (t == NULL)
+ return (0);
- return (0);
+ *val = t-val;
+
+ return (1);
}
void
Re: does this patch make sense?
On Thu, Nov 07, 2013 at 17:19, Peter J. Philipp wrote:
+ gid = getgid();
+
+ if (setgroups(1, gid) == -1)
+ err(1, setgroups);
+
+ if (setresgid(gid, gid, gid) == -1)
+ err(1, setresgid);
+
if (setresuid(uid, uid, uid) == -1)
err(1, setresuid);
I thought about it and thought my patch didn't really do anything. So
Right. This doesn't do anything. traceroute isn't setgid, it has no
group privileges to revoke.
/* DiffServ Codepoints and other TOS mappings */
+ /* KEEP SORTED */
const struct toskeywords {
const char *keyword;
int val;
@@ -1258,14 +1268,13 @@ map_tos(char *s, int *val)
{ NULL, -1 },
};
- for (t = toskeywords; t-keyword != NULL; t++) {
- if (strcmp(s, t-keyword) == 0) {
- *val = t-val;
- return (1);
- }
- }
+ t = bsearch(s, toskeywords, nitems(toskeywords), sizeof(struct
toskeywords), (int (*)(const void *, const void *))strcmp);
I don't like the way this is abusing types. In fact, I don't think this
even works. Did you test it? A pointer to a struct toskeyword will not
have the same value as the keyword member.
Re: does this patch make sense?
On Thu, Nov 07, 2013 at 11:32:48AM -0500, Ted Unangst wrote:
On Thu, Nov 07, 2013 at 17:19, Peter J. Philipp wrote:
+ gid = getgid();
+
+ if (setgroups(1, gid) == -1)
+ err(1, setgroups);
+
+ if (setresgid(gid, gid, gid) == -1)
+ err(1, setresgid);
+
if (setresuid(uid, uid, uid) == -1)
err(1, setresuid);
I thought about it and thought my patch didn't really do anything. So
Right. This doesn't do anything. traceroute isn't setgid, it has no
group privileges to revoke.
/* DiffServ Codepoints and other TOS mappings */
+ /* KEEP SORTED */
const struct toskeywords {
const char *keyword;
int val;
@@ -1258,14 +1268,13 @@ map_tos(char *s, int *val)
{ NULL, -1 },
};
- for (t = toskeywords; t-keyword != NULL; t++) {
- if (strcmp(s, t-keyword) == 0) {
- *val = t-val;
- return (1);
- }
- }
+ t = bsearch(s, toskeywords, nitems(toskeywords), sizeof(struct
toskeywords), (int (*)(const void *, const void *))strcmp);
I don't like the way this is abusing types. In fact, I don't think this
even works. Did you test it? A pointer to a struct toskeyword will not
have the same value as the keyword member.
The first field of a struct has the same address as the the struct
itself. Still I consider this bad form and overkill.
-Otto
Re: does this patch make sense?
On 11/07/13 17:32, Ted Unangst wrote:
On Thu, Nov 07, 2013 at 17:19, Peter J. Philipp wrote:
+ gid = getgid();
+
+ if (setgroups(1, gid) == -1)
+ err(1, setgroups);
+
+ if (setresgid(gid, gid, gid) == -1)
+ err(1, setresgid);
+
if (setresuid(uid, uid, uid) == -1)
err(1, setresuid);
I thought about it and thought my patch didn't really do anything. So
Right. This doesn't do anything. traceroute isn't setgid, it has no
group privileges to revoke.
/* DiffServ Codepoints and other TOS mappings */
+ /* KEEP SORTED */
const struct toskeywords {
const char *keyword;
int val;
@@ -1258,14 +1268,13 @@ map_tos(char *s, int *val)
{ NULL, -1 },
};
- for (t = toskeywords; t-keyword != NULL; t++) {
- if (strcmp(s, t-keyword) == 0) {
- *val = t-val;
- return (1);
- }
- }
+ t = bsearch(s, toskeywords, nitems(toskeywords), sizeof(struct
toskeywords), (int (*)(const void *, const void *))strcmp);
I don't like the way this is abusing types. In fact, I don't think this
even works. Did you test it? A pointer to a struct toskeyword will not
have the same value as the keyword member.
OK I'll stop abusing. Here is my reasoning for the setgid change.
Pretend there is a way to break into the binary by means of the socket,
then I thought it'd be neat if it was disallowed to write into groups
that a user was in at the moment this binary was executed. I think this
is paranoid enough.
And yes I tested it. I used reliability keyword and throughput keyword
and a notused keyword, they matched in tcpdump with the #defined values
and bailed on the third keyword.
# traceroute -t throughput venus
traceroute to venus.centroid.eu (192.168.60.1), 64 hops max, 40 byte packets
1 uranus (192.168.34.1) 0.211 ms 0.188 ms 0.248 ms
# 17:10:48.701844 192.168.34.4.52757 192.168.60.1.33435: [no cksum]
udp 12 [tos 0x8] [ttl 1] (id 52758, len 40)
To be honest I'm not at a high level as you so I don't understand what
the last sentence means. I had the bsearch manpage to guide me and it
was surprising to me this even worked so well.
I'm gonna leave this the way it is now.
-peter
Re: does this patch make sense?
On 11/07/13 17:48, Otto Moerbeek wrote:
On Thu, Nov 07, 2013 at 11:32:48AM -0500, Ted Unangst wrote:
On Thu, Nov 07, 2013 at 17:19, Peter J. Philipp wrote:
+ gid = getgid();
+
+ if (setgroups(1, gid) == -1)
+ err(1, setgroups);
+
+ if (setresgid(gid, gid, gid) == -1)
+ err(1, setresgid);
+
if (setresuid(uid, uid, uid) == -1)
err(1, setresuid);
I thought about it and thought my patch didn't really do anything. So
Right. This doesn't do anything. traceroute isn't setgid, it has no
group privileges to revoke.
/* DiffServ Codepoints and other TOS mappings */
+ /* KEEP SORTED */
const struct toskeywords {
const char *keyword;
int val;
@@ -1258,14 +1268,13 @@ map_tos(char *s, int *val)
{ NULL, -1 },
};
- for (t = toskeywords; t-keyword != NULL; t++) {
- if (strcmp(s, t-keyword) == 0) {
- *val = t-val;
- return (1);
- }
- }
+ t = bsearch(s, toskeywords, nitems(toskeywords), sizeof(struct
toskeywords), (int (*)(const void *, const void *))strcmp);
I don't like the way this is abusing types. In fact, I don't think this
even works. Did you test it? A pointer to a struct toskeyword will not
have the same value as the keyword member.
The first field of a struct has the same address as the the struct
itself. Still I consider this bad form and overkill.
-Otto
Hi,
while I don't want to persue this patch further, I'd like to say that I
finished it on my own, thanks to your input I understand what base in
bsearch() is supposed to be now. I had something in mind from qsort()
which also has a variable called base in the manpages and that had
confused me. I have taken a look how bsearch() in other programs and
I have noticed that some are doing it like me but wrap strcmp inside
another *cmp where there is a bit of casting being done. I'm wondering
if that is the right way? Or if it can be cleaned up?
Thanks!
-peter
Re: does this patch make sense?
On 11/07/13 20:33, Peter J. Philipp wrote:
On 11/07/13 17:48, Otto Moerbeek wrote:
On Thu, Nov 07, 2013 at 11:32:48AM -0500, Ted Unangst wrote:
On Thu, Nov 07, 2013 at 17:19, Peter J. Philipp wrote:
+ gid = getgid();
+
+ if (setgroups(1, gid) == -1)
+ err(1, setgroups);
+
+ if (setresgid(gid, gid, gid) == -1)
+ err(1, setresgid);
+
if (setresuid(uid, uid, uid) == -1)
err(1, setresuid);
I thought about it and thought my patch didn't really do anything. So
Right. This doesn't do anything. traceroute isn't setgid, it has no
group privileges to revoke.
/* DiffServ Codepoints and other TOS mappings */
+ /* KEEP SORTED */
const struct toskeywords {
const char *keyword;
int val;
@@ -1258,14 +1268,13 @@ map_tos(char *s, int *val)
{ NULL, -1 },
};
- for (t = toskeywords; t-keyword != NULL; t++) {
- if (strcmp(s, t-keyword) == 0) {
- *val = t-val;
- return (1);
- }
- }
+ t = bsearch(s, toskeywords, nitems(toskeywords), sizeof(struct
toskeywords), (int (*)(const void *, const void *))strcmp);
I don't like the way this is abusing types. In fact, I don't think this
even works. Did you test it? A pointer to a struct toskeyword will not
have the same value as the keyword member.
The first field of a struct has the same address as the the struct
itself. Still I consider this bad form and overkill.
-Otto
Hi,
while I don't want to persue this patch further, I'd like to say that I
finished it on my own, thanks to your input I understand what base in
bsearch() is supposed to be now. I had something in mind from qsort()
which also has a variable called base in the manpages and that had
confused me. I have taken a look how bsearch() in other programs and
I have noticed that some are doing it like me but wrap strcmp inside
another *cmp where there is a bit of casting being done. I'm wondering
if that is the right way? Or if it can be cleaned up?
Thanks!
-peter
Ahh never mind, I didn't test it. It did compile very cleanly though.
Sorry, I'll shut up now.
-peter
Re: does this patch make sense?
On Thu, Nov 07, 2013 at 17:48, Otto Moerbeek wrote:
- for (t = toskeywords; t-keyword != NULL; t++) {
- if (strcmp(s, t-keyword) == 0) {
- *val = t-val;
- return (1);
- }
- }
+ t = bsearch(s, toskeywords, nitems(toskeywords), sizeof(struct
toskeywords), (int (*)(const void *, const void *))strcmp);
I don't like the way this is abusing types. In fact, I don't think this
even works. Did you test it? A pointer to a struct toskeyword will not
have the same value as the keyword member.
The first field of a struct has the same address as the the struct
itself. Still I consider this bad form and overkill.
This is true, but strcmp expects the value of t-keyword, not its
address. Have I read the code wrong?
Re: does this patch make sense?
On Thu, Nov 07, 2013 at 04:35:46PM -0500, Ted Unangst wrote:
On Thu, Nov 07, 2013 at 17:48, Otto Moerbeek wrote:
- for (t = toskeywords; t-keyword != NULL; t++) {
- if (strcmp(s, t-keyword) == 0) {
- *val = t-val;
- return (1);
- }
- }
+ t = bsearch(s, toskeywords, nitems(toskeywords), sizeof(struct
toskeywords), (int (*)(const void *, const void *))strcmp);
I don't like the way this is abusing types. In fact, I don't think this
even works. Did you test it? A pointer to a struct toskeyword will not
have the same value as the keyword member.
The first field of a struct has the same address as the the struct
itself. Still I consider this bad form and overkill.
This is true, but strcmp expects the value of t-keyword, not its
address. Have I read the code wrong?
Right, it is wrong indeed. It could have been ok if keyward was an array.
-Otto
Re: does this patch make sense?
On Thu, Nov 07, 2013 at 17:54, Peter J. Philipp wrote: OK I'll stop abusing. Here is my reasoning for the setgid change. Pretend there is a way to break into the binary by means of the socket, then I thought it'd be neat if it was disallowed to write into groups that a user was in at the moment this binary was executed. I think this is paranoid enough. If this were a concern, we'd need similar patches for ftp, nc, firefox, and every other socket using program in the system. And then similar patches for every image viewer. And text editor. And so on. In short, we are not out to protect users from themselves (at least, not in this way). If you don't want a program to have group privileges, that's your responsibility, not the responsiblity of every program.
