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.