* Re: [9front] Snoopy(8) patch
@ 2019-06-07 7:13 Alex Musolino
0 siblings, 0 replies; 3+ messages in thread
From: Alex Musolino @ 2019-06-07 7:13 UTC (permalink / raw)
To: ori, 9front, alex
> And because addr is only 4 bytes, we overwrite random memory.
> I think a check on nocts here is neeeded.
Indeed. Good catch. I've also adjusted the pclroutes function to
only print the leading space after the potential bail-out point.
Here's the new patch:
diff -r f0621f169310 sys/src/cmd/ip/snoopy/dhcp.c
--- a/sys/src/cmd/ip/snoopy/dhcp.c Wed Jun 05 16:04:50 2019 +0930
+++ b/sys/src/cmd/ip/snoopy/dhcp.c Fri Jun 07 12:57:36 2019 +0930
@@ -3,105 +3,7 @@
#include <ip.h>
#include "dat.h"
#include "protos.h"
-
-enum
-{
- Maxoptlen= 312-4,
-
- /* dhcp types */
- Discover= 1,
- Offer= 2,
- Request= 3,
- Decline= 4,
- Ack= 5,
- Nak= 6,
- Release= 7,
- Inform= 8,
-
- /* bootp option types */
- OBend= 255,
- OBpad= 0,
- OBmask= 1,
- OBtimeoff= 2,
- OBrouter= 3,
- OBtimeserver= 4,
- OBnameserver= 5,
- OBdnserver= 6,
- OBlogserver= 7,
- OBcookieserver= 8,
- OBlprserver= 9,
- OBimpressserver= 10,
- OBrlserver= 11,
- OBhostname= 12, /* 0xc0 */
- OBbflen= 13,
- OBdumpfile= 14,
- OBdomainname= 15,
- OBswapserver= 16, /* 0x10 */
- OBrootpath= 17,
- OBextpath= 18,
- OBipforward= 19,
- OBnonlocal= 20,
- OBpolicyfilter= 21,
- OBmaxdatagram= 22,
- OBttl= 23,
- OBpathtimeout= 24,
- OBpathplateau= 25,
- OBmtu= 26,
- OBsubnetslocal= 27,
- OBbaddr= 28,
- OBdiscovermask= 29,
- OBsupplymask= 30,
- OBdiscoverrouter= 31,
- OBrsserver= 32, /* 0x20 */
- OBstaticroutes= 33,
- OBtrailerencap= 34,
- OBarptimeout= 35,
- OBetherencap= 36,
- OBtcpttl= 37,
- OBtcpka= 38,
- OBtcpkag= 39,
- OBnisdomain= 40,
- OBniserver= 41,
- OBntpserver= 42,
- OBvendorinfo= 43, /* 0x2b */
- OBnetbiosns= 44,
- OBnetbiosdds= 45,
- OBnetbiostype= 46,
- OBnetbiosscope= 47,
- OBxfontserver= 48, /* 0x30 */
- OBxdispmanager= 49,
- OBnisplusdomain= 64, /* 0x40 */
- OBnisplusserver= 65,
- OBhomeagent= 68,
- OBsmtpserver= 69,
- OBpop3server= 70,
- OBnntpserver= 71,
- OBwwwserver= 72,
- OBfingerserver= 73,
- OBircserver= 74,
- OBstserver= 75,
- OBstdaserver= 76,
-
- /* dhcp options */
- ODipaddr= 50, /* 0x32 */
- ODlease= 51,
- ODoverload= 52,
- ODtype= 53, /* 0x35 */
- ODserverid= 54, /* 0x36 */
- ODparams= 55, /* 0x37 */
- ODmessage= 56,
- ODmaxmsg= 57,
- ODrenewaltime= 58,
- ODrebindingtime= 59,
- ODvendorclass= 60,
- ODclientid= 61, /* 0x3d */
- ODtftpserver= 66,
- ODbootfile= 67,
-
- /* plan9 vendor info options */
- OP9fsv4= 128, /* plan9 file servers */
- OP9authv4= 129, /* plan9 auth servers */
-};
+#include "../dhcp.h"
/*
* convert a byte array to hex
@@ -163,9 +65,14 @@
static char*
pserver(char *p, char *e, char *tag, uchar *o, int n)
{
+ int i;
+
p = seprint(p, e, "%s=(", tag);
+ i = 0;
while(n >= 4){
- p = seprint(p, e, " %V", o);
+ if(i++ > 0)
+ p = seprint(p, e, " ");
+ p = seprint(p, e, "%V", o);
n -= 4;
o += 4;
}
@@ -173,6 +80,52 @@
return p;
}
+static char*
+pcfroutes(char *p, char *e, char *tag, uchar *o, int n)
+{
+ int i;
+
+ p = seprint(p, e, "%s=(", tag);
+ i = 0;
+ while(n >= 8){
+ if(i++ > 0)
+ p = seprint(p, e, " ");
+ p = seprint(p, e, "%V→%V", o, o+4);
+ o += 8;
+ n -= 8;
+ }
+ p = seprint(p, e, ")");
+ return p;
+}
+
+static char*
+pclroutes(char *p, char *e, char *tag, uchar *o, int n)
+{
+ uchar addr[4];
+ int i, nbits, nocts;
+
+ p = seprint(p, e, "%s=(", tag);
+ i = 0;
+ while(n >= 5){
+ nbits = *o++;
+ n--;
+ nocts = nbits <= 32 ? (nbits+7)/8 : 4;
+ if(n < nocts+4)
+ break;
+ memset(addr, 0, 4);
+ memmove(addr, o, nocts);
+ o += nocts;
+ n -= nocts;
+ if(i++ > 0)
+ p = seprint(p, e, " ");
+ p = seprint(p, e, "%V/%d→%V", addr, nbits, o);
+ o += 4;
+ n -= 4;
+ }
+ p = seprint(p, e, ")");
+ return p;
+}
+
static char *dhcptype[256] =
{
[Discover] "Discover",
@@ -305,8 +258,8 @@
case OBdomainname:
p = pstring(p, e, "domname", o, n);
break;
- case OBswapserver:
- p = pserver(p, e, "swapsrv", o, n);
+ case OBrootserver:
+ p = pserver(p, e, "rootsrv", o, n);
break;
case OBrootpath:
p = pstring(p, e, "rootpath", o, n);
@@ -357,7 +310,10 @@
p = pserver(p, e, "rsrouter", o, n);
break;
case OBstaticroutes:
- p = phex(p, e, "staticroutes", o, n);
+ p = pcfroutes(p, e, "cf-routes", o, n);
+ break;
+ case ODclasslessroutes:
+ p = pclroutes(p, e, "cl-routes", o, n);
break;
case OBtrailerencap:
p = phex(p, e, "trailerencap", o, n);
@@ -443,7 +399,7 @@
case OBend:
goto out;
default:
- snprint(msg, sizeof msg, " T%ud", code);
+ snprint(msg, sizeof msg, "T%ud", code);
p = phex(p, e, msg, o, n);
break;
}
--
Cheers,
Alex Musolino
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [9front] Snoopy(8) patch
@ 2019-06-09 3:11 ori
0 siblings, 0 replies; 3+ messages in thread
From: ori @ 2019-06-09 3:11 UTC (permalink / raw)
To: alex, ori, 9front
>> And because addr is only 4 bytes, we overwrite random memory.
>> I think a check on nocts here is neeeded.
>
> Indeed. Good catch. I've also adjusted the pclroutes function to
> only print the leading space after the potential bail-out point.
Looks sane to me, although this diff is missing the change to dhcp.h.
Guessing you just forgot that in the diff.
Quickly comparing against rfc3442, it looks correct to me.
> Here's the new patch:
>
> diff -r f0621f169310 sys/src/cmd/ip/snoopy/dhcp.c
> --- a/sys/src/cmd/ip/snoopy/dhcp.c Wed Jun 05 16:04:50 2019 +0930
> +++ b/sys/src/cmd/ip/snoopy/dhcp.c Fri Jun 07 12:57:36 2019 +0930
> @@ -3,105 +3,7 @@
> #include <ip.h>
> #include "dat.h"
> #include "protos.h"
> -
> -enum
> -{
> - Maxoptlen= 312-4,
> -
> - /* dhcp types */
> - Discover= 1,
> - Offer= 2,
> - Request= 3,
> - Decline= 4,
> - Ack= 5,
> - Nak= 6,
> - Release= 7,
> - Inform= 8,
> -
> - /* bootp option types */
> - OBend= 255,
> - OBpad= 0,
> - OBmask= 1,
> - OBtimeoff= 2,
> - OBrouter= 3,
> - OBtimeserver= 4,
> - OBnameserver= 5,
> - OBdnserver= 6,
> - OBlogserver= 7,
> - OBcookieserver= 8,
> - OBlprserver= 9,
> - OBimpressserver= 10,
> - OBrlserver= 11,
> - OBhostname= 12, /* 0xc0 */
> - OBbflen= 13,
> - OBdumpfile= 14,
> - OBdomainname= 15,
> - OBswapserver= 16, /* 0x10 */
> - OBrootpath= 17,
> - OBextpath= 18,
> - OBipforward= 19,
> - OBnonlocal= 20,
> - OBpolicyfilter= 21,
> - OBmaxdatagram= 22,
> - OBttl= 23,
> - OBpathtimeout= 24,
> - OBpathplateau= 25,
> - OBmtu= 26,
> - OBsubnetslocal= 27,
> - OBbaddr= 28,
> - OBdiscovermask= 29,
> - OBsupplymask= 30,
> - OBdiscoverrouter= 31,
> - OBrsserver= 32, /* 0x20 */
> - OBstaticroutes= 33,
> - OBtrailerencap= 34,
> - OBarptimeout= 35,
> - OBetherencap= 36,
> - OBtcpttl= 37,
> - OBtcpka= 38,
> - OBtcpkag= 39,
> - OBnisdomain= 40,
> - OBniserver= 41,
> - OBntpserver= 42,
> - OBvendorinfo= 43, /* 0x2b */
> - OBnetbiosns= 44,
> - OBnetbiosdds= 45,
> - OBnetbiostype= 46,
> - OBnetbiosscope= 47,
> - OBxfontserver= 48, /* 0x30 */
> - OBxdispmanager= 49,
> - OBnisplusdomain= 64, /* 0x40 */
> - OBnisplusserver= 65,
> - OBhomeagent= 68,
> - OBsmtpserver= 69,
> - OBpop3server= 70,
> - OBnntpserver= 71,
> - OBwwwserver= 72,
> - OBfingerserver= 73,
> - OBircserver= 74,
> - OBstserver= 75,
> - OBstdaserver= 76,
> -
> - /* dhcp options */
> - ODipaddr= 50, /* 0x32 */
> - ODlease= 51,
> - ODoverload= 52,
> - ODtype= 53, /* 0x35 */
> - ODserverid= 54, /* 0x36 */
> - ODparams= 55, /* 0x37 */
> - ODmessage= 56,
> - ODmaxmsg= 57,
> - ODrenewaltime= 58,
> - ODrebindingtime= 59,
> - ODvendorclass= 60,
> - ODclientid= 61, /* 0x3d */
> - ODtftpserver= 66,
> - ODbootfile= 67,
> -
> - /* plan9 vendor info options */
> - OP9fsv4= 128, /* plan9 file servers */
> - OP9authv4= 129, /* plan9 auth servers */
> -};
> +#include "../dhcp.h"
>
> /*
> * convert a byte array to hex
> @@ -163,9 +65,14 @@
> static char*
> pserver(char *p, char *e, char *tag, uchar *o, int n)
> {
> + int i;
> +
> p = seprint(p, e, "%s=(", tag);
> + i = 0;
> while(n >= 4){
> - p = seprint(p, e, " %V", o);
> + if(i++ > 0)
> + p = seprint(p, e, " ");
> + p = seprint(p, e, "%V", o);
> n -= 4;
> o += 4;
> }
> @@ -173,6 +80,52 @@
> return p;
> }
>
> +static char*
> +pcfroutes(char *p, char *e, char *tag, uchar *o, int n)
> +{
> + int i;
> +
> + p = seprint(p, e, "%s=(", tag);
> + i = 0;
> + while(n >= 8){
> + if(i++ > 0)
> + p = seprint(p, e, " ");
> + p = seprint(p, e, "%V→%V", o, o+4);
> + o += 8;
> + n -= 8;
> + }
> + p = seprint(p, e, ")");
> + return p;
> +}
> +
> +static char*
> +pclroutes(char *p, char *e, char *tag, uchar *o, int n)
> +{
> + uchar addr[4];
> + int i, nbits, nocts;
> +
> + p = seprint(p, e, "%s=(", tag);
> + i = 0;
> + while(n >= 5){
> + nbits = *o++;
> + n--;
> + nocts = nbits <= 32 ? (nbits+7)/8 : 4;
> + if(n < nocts+4)
> + break;
> + memset(addr, 0, 4);
> + memmove(addr, o, nocts);
> + o += nocts;
> + n -= nocts;
> + if(i++ > 0)
> + p = seprint(p, e, " ");
> + p = seprint(p, e, "%V/%d→%V", addr, nbits, o);
> + o += 4;
> + n -= 4;
> + }
> + p = seprint(p, e, ")");
> + return p;
> +}
> +
> static char *dhcptype[256] =
> {
> [Discover] "Discover",
> @@ -305,8 +258,8 @@
> case OBdomainname:
> p = pstring(p, e, "domname", o, n);
> break;
> - case OBswapserver:
> - p = pserver(p, e, "swapsrv", o, n);
> + case OBrootserver:
> + p = pserver(p, e, "rootsrv", o, n);
> break;
> case OBrootpath:
> p = pstring(p, e, "rootpath", o, n);
> @@ -357,7 +310,10 @@
> p = pserver(p, e, "rsrouter", o, n);
> break;
> case OBstaticroutes:
> - p = phex(p, e, "staticroutes", o, n);
> + p = pcfroutes(p, e, "cf-routes", o, n);
> + break;
> + case ODclasslessroutes:
> + p = pclroutes(p, e, "cl-routes", o, n);
> break;
> case OBtrailerencap:
> p = phex(p, e, "trailerencap", o, n);
> @@ -443,7 +399,7 @@
> case OBend:
> goto out;
> default:
> - snprint(msg, sizeof msg, " T%ud", code);
> + snprint(msg, sizeof msg, "T%ud", code);
> p = phex(p, e, msg, o, n);
> break;
> }
>
> --
> Cheers,
> Alex Musolino
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [9front] Snoopy(8) patch
2019-06-06 15:21 Alex Musolino
@ 2019-06-06 18:08 ` Ori Bernstein
0 siblings, 0 replies; 3+ messages in thread
From: Ori Bernstein @ 2019-06-06 18:08 UTC (permalink / raw)
To: 9front; +Cc: Alex Musolino
On Fri, 7 Jun 2019 00:51:52 +0930
Alex Musolino <alex@musolino.id.au> wrote:
> Hello,
>
> I've made some changes [1] to snoopy(8) in order to help with some
> work I'm doing on dhcpd(8) to support the static route options.
>
> Any objections to committing/pushing these?
>
> [1] http://musolino.id.au/up/2019/06/05/snoopy.dhcp.patch
>
> --
> Cheers,
> Alex Musolino
>
If you inline the patch in the mail, it's easier for me to comment on it.
It mostly looks good to me, just noticed one thing:
> + uchar addr[4];
> ...
> + nbits = *o++;
Nbits is a uchar, so if we get a junk message, the maximum value here is 255.
> + n--;
> + nocts = (nbits+7)/8;
(255 + 7)/8 == 32
> + memset(addr, 0, 4);
> + if(n < nocts+4)
> + break;
> + memmove(addr, o, nocts);
And because addr is only 4 bytes, we overwrite random memory.
I think a check on nocts here is neeeded.
--
Ori Bernstein <ori@eigenstate.org>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-06-09 3:11 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-07 7:13 [9front] Snoopy(8) patch Alex Musolino
-- strict thread matches above, loose matches on Subject: below --
2019-06-09 3:11 ori
2019-06-06 15:21 Alex Musolino
2019-06-06 18:08 ` [9front] " Ori Bernstein
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).