Date: Sun, 5 Oct 1997 22:30:05 +0900 (JST) From: chi@bd.mbn.or.jp To: FreeBSD-gnats-submit@FreeBSD.ORG Subject: bin/4727: ppp(IIJ-PPP) bug of setting and showing IP filter Message-ID: <199710051330.WAA00470@chino.localhost> Resent-Message-ID: <199710081240.FAA14894@hub.freebsd.org>
next in thread | raw e-mail | index | archive | help
>Number: 4727
>Category: bin
>Synopsis: ppp(IIJ-PPP) bug of setting and showing IP filter
>Confidential: no
>Severity: non-critical
>Priority: medium
>Responsible: freebsd-bugs
>State: open
>Class: sw-bug
>Submitter-Id: current-users
>Arrival-Date: Wed Oct 8 05:40:01 PDT 1997
>Last-Modified:
>Originator: Chiharu Shibata
>Organization:
NJK corporation
>Release: FreeBSD 2.2.1-RELEASE i386
>Environment:
>Description:
(1)The "estab" could not be specified without specifying a port number.
Ex.)set ifilter 0 permit tcp estab
The ParseUdpOrTcp() of filter.c is corded that it becomes an error
that the number of the parameters following the protocol is not 0
and greater than or equal to 3.
The FilterCheck() of ip.c, when srcop and dstop are both "OP_NONE",
it is possible to check only whether a packet is "estab" or not
without checking a port number.
So, this specification should be possible.
(2)In case of using the "lt" for the comparison of a port number,
it is displayed "none" instead of "lt".
Ex.)set ifilter 0 deny udp src lt 1024
show ifilter
The "OP_LT" is defined by 4 in filter.h, and is used an index of
the array "opname[]".
But the "opname[]" of filter.c is defined only 4 elements([0] to [3]).
Each time we use "show" command about the filter which used "lt",
it becomes access violation of array.
(3)It becomes an error that when specifying a protocol and a port
number just following a source IP address.
Ex.)set ifilter 0 deny 172.16.1.1/24 tcp src eq 23
In the Parse() of filter.c, it should be increased the "argv" when
specifying a protocol following a source IP address.
But it is not doing so.
(4)It has been possible to specify the "estab" for UDP.
Ex.)set ifilter 0 permit udp estab
The ParseUdpOrTcp() of filter.c is corded to check the existence of
the "estab" irrespective of TCP or UDP.
But the "estab" is peculiar to TCP.
So, it should only allow TCP.
Even if modifying the cord like this, there is no influence about IP
filtering specification.
Because the FilterCheck() of ip.c is coded that a UDP packet seems to
the "estab" unconditionally.
(5)Mistaking of the define value for srcop and dstop in ParseUdpOrTcp()
of filter.c.
The other place to refer to srcop and dstop, "OP_XXX" is used for those,
and "A_XXX" is used for action.
Luckily "OP_NONE" and "A_NONE" are 0 together, there is no actual harm.
(6)It does not become an error even if specifying the one which is not
a source IP address and a protocol.
Ex.)set ifilter 0 deny XXXX
The "inet_addr()" is used in the ParseAddr() of filter.c, and it is not
checking a return value.
Manual page of inet_addr(3) says that
> The value INADDR_NONE (0xffffffff) is a valid broadcast address,
> but inet_addr() cannot return that value without indicating failure.
I think that this is a reason of not(or unable to) checking a return value
at the present source code.
Manual page also says that this problem does not happen to inet_aton(3).
So, the ParseAddr() should be rewritten to use inet_aton().
>How-To-Repeat:
Please refer to "Description".
>Fix:
--- filter.c.orig Thu Jan 11 06:27:43 1996
+++ filter.c Thu Sep 25 23:53:00 1997
@@ -56,7 +56,6 @@
struct in_addr *pmask;
int *pwidth;
{
- u_long addr;
int bits;
char *cp, *wp;
@@ -70,8 +69,9 @@
pmask->s_addr = -1; /* Assume 255.255.255.255 as default */
cp = index(*argv, '/');
if (cp) *cp++ = '\0';
- addr = inet_addr(*argv);
- paddr->s_addr = addr;
+ if (inet_aton(*argv, paddr) == 0) {
+ return(0);
+ }
if (cp && *cp) {
bits = strtol(cp, &wp, 0);
if (cp == wp || bits < 0 || bits > 32) {
@@ -201,17 +201,20 @@
int proto;
{
+ filterdata.opt.srcop = filterdata.opt.dstop = OP_NONE;
+ filterdata.opt.estab = 0;
if (argc == 0) {
/* permit/deny all tcp traffic */
- filterdata.opt.srcop = filterdata.opt.dstop = A_NONE;
return(1);
}
+#if 0 /* XXX: allow estab only */
if (argc < 3) {
#ifdef notdef
printf("bad udp syntax.\n");
#endif
return(0);
}
+#endif /* 0 */
if (argc >= 3 && STREQ(*argv, "src")) {
filterdata.opt.srcop = ParseOp(argv[1]);
if (filterdata.opt.srcop == OP_NONE) {
@@ -238,7 +241,7 @@
if (argc == 0)
return(1);
}
- if (argc == 1) {
+ if (argc == 1 && proto == P_TCP) {
if (STREQ(*argv, "estab")) {
filterdata.opt.estab = 1;
return(1);
@@ -251,7 +254,9 @@
return(0);
}
-char *opname[] = { "none", "eq", "gt", "lt" };
+/* XXX: OP_LT defines 4 in filter.h */
+/* 0 1 2 3 4 */
+char *opname[] = { "none", "eq", "gt", NULL, "lt" };
static int
Parse(argc, argv, ofp)
@@ -323,9 +328,9 @@
argc--; argv++;
}
proto = ParseProto(argc, argv);
- if (proto) {
- argc--; argv++;
- }
+ }
+ if (proto != P_NONE) {
+ argc--; argv++;
}
} else {
printf("Address/protocol expected.\n");
>Audit-Trail:
>Unformatted:
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?199710051330.WAA00470>