Changeset View
Standalone View
sbin/ipfw/nat.c
Show First 20 Lines • Show All 59 Lines • ▼ Show 20 Lines | static struct _s_x nat_params[] = { | ||||
{ "deny_in", TOK_DENY_INC }, | { "deny_in", TOK_DENY_INC }, | ||||
{ "same_ports", TOK_SAME_PORTS }, | { "same_ports", TOK_SAME_PORTS }, | ||||
{ "unreg_only", TOK_UNREG_ONLY }, | { "unreg_only", TOK_UNREG_ONLY }, | ||||
{ "unreg_cgn", TOK_UNREG_CGN }, | { "unreg_cgn", TOK_UNREG_CGN }, | ||||
{ "skip_global", TOK_SKIP_GLOBAL }, | { "skip_global", TOK_SKIP_GLOBAL }, | ||||
{ "reset", TOK_RESET_ADDR }, | { "reset", TOK_RESET_ADDR }, | ||||
{ "reverse", TOK_ALIAS_REV }, | { "reverse", TOK_ALIAS_REV }, | ||||
{ "proxy_only", TOK_PROXY_ONLY }, | { "proxy_only", TOK_PROXY_ONLY }, | ||||
{ "port_alias", TOK_PORT_ALIAS }, | |||||
{ "redirect_addr", TOK_REDIR_ADDR }, | { "redirect_addr", TOK_REDIR_ADDR }, | ||||
{ "redirect_port", TOK_REDIR_PORT }, | { "redirect_port", TOK_REDIR_PORT }, | ||||
{ "redirect_proto", TOK_REDIR_PROTO }, | { "redirect_proto", TOK_REDIR_PROTO }, | ||||
{ NULL, 0 } /* terminator */ | { NULL, 0 } /* terminator */ | ||||
}; | }; | ||||
/* | /* | ||||
▲ Show 20 Lines • Show All 671 Lines • ▼ Show 20 Lines | for (cnt = 0; cnt < n->redir_cnt; cnt++) { | ||||
default: | default: | ||||
errx(EX_DATAERR, "unknown redir mode"); | errx(EX_DATAERR, "unknown redir mode"); | ||||
break; | break; | ||||
} | } | ||||
} | } | ||||
printf("\n"); | printf("\n"); | ||||
} | } | ||||
static u_short | |||||
nat_port_alias_parse(char *ptr) | |||||
donner: I'd prefer a function witch return the valid port number or 0 in the case of failure. Writable… | |||||
{ | |||||
u_short port; | |||||
Done Inline ActionsDid you try "12345six" as a port number? donner: Did you try "12345six" as a port number? | |||||
for (char *ptr2 = ptr; *ptr2 != '\0'; ptr2++) | |||||
Done Inline ActionsDid try "7654321" as a port number? donner: Did try "7654321" as a port number? | |||||
Done Inline ActionsIt looks like that will still get through, since the cast will convert it to 52145 (the C standard defines this conversion, although it is not what is wanted here). I suggest storing the result from strtol() in a variable of type long and doing range checks on that. jilles: It looks like that will still get through, since the cast will convert it to 52145 (the C… | |||||
Done Inline Actionslong is indeed better than any unsigned type, because it would catch negative numbers easily. donner: long is indeed better than any unsigned type, because it would catch negative numbers easily. | |||||
Done Inline ActionsThere's some extraneous whitespace here. kp: There's some extraneous whitespace here. | |||||
if (!isdigit(*ptr2)) return 0; | |||||
Done Inline ActionsThe pointer end will always be != NULL, so you may even allow port numbers below 1024. donner: The pointer end will always be != NULL, so you may even allow port numbers below 1024. | |||||
port = (u_short) strtol(ptr, NULL, 10); | |||||
Done Inline ActionsThe common idiom is to provide a pointer as the second argument ti strtol and inspect the (first unparsable) char it points to afterwards. port = strtol(ptr, ptr2, 10); if ( *ptr2 != '\0' || port < 1024 ) error donner: The common idiom is to provide a pointer as the second argument ti strtol and inspect the… | |||||
if (port < 1024 || port > 65535) | |||||
Done Inline Actionsu_short comparsion with MAX_USHORT is not recommended. donner: u_short comparsion with MAX_USHORT is not recommended. | |||||
return 0; | |||||
return port; | |||||
} | |||||
Done Inline Actionsand here. kp: and here. | |||||
void | void | ||||
ipfw_config_nat(int ac, char **av) | ipfw_config_nat(int ac, char **av) | ||||
Done Inline ActionsWhy parsing from again from the very beginning of the string? donner: Why parsing from again from the very beginning of the string? | |||||
{ | { | ||||
ipfw_obj_header *oh; | ipfw_obj_header *oh; | ||||
struct nat44_cfg_nat *n; /* Nat instance configuration. */ | struct nat44_cfg_nat *n; /* Nat instance configuration. */ | ||||
int i, off, tok, ac1; | int i, off, tok, ac1; | ||||
u_short lp, hp; | |||||
char *id, *buf, **av1, *end; | char *id, *buf, **av1, *end; | ||||
size_t len; | size_t len; | ||||
av++; | av++; | ||||
ac--; | ac--; | ||||
/* Nat id. */ | /* Nat id. */ | ||||
if (ac == 0) | if (ac == 0) | ||||
errx(EX_DATAERR, "missing nat id"); | errx(EX_DATAERR, "missing nat id"); | ||||
▲ Show 20 Lines • Show All 63 Lines • ▼ Show 20 Lines | case TOK_REDIR_PROTO: | ||||
av1++; | av1++; | ||||
ac1--; | ac1--; | ||||
} | } | ||||
if (ac1 != 0 && isdigit(**av1)) { | if (ac1 != 0 && isdigit(**av1)) { | ||||
av1++; | av1++; | ||||
ac1--; | ac1--; | ||||
} | } | ||||
break; | break; | ||||
case TOK_PORT_ALIAS: | |||||
if (ac1 < 2) | |||||
errx(EX_DATAERR, "port_alias: " | |||||
"not enough arguments"); | |||||
av1 += 2; | |||||
ac1 -= 2; | |||||
break; | |||||
default: | default: | ||||
errx(EX_DATAERR, "unrecognised option ``%s''", av1[-1]); | errx(EX_DATAERR, "unrecognised option ``%s''", av1[-1]); | ||||
} | } | ||||
} | } | ||||
if ((buf = malloc(len)) == NULL) | if ((buf = malloc(len)) == NULL) | ||||
errx(EX_OSERR, "malloc failed"); | errx(EX_OSERR, "malloc failed"); | ||||
▲ Show 20 Lines • Show All 66 Lines • ▼ Show 20 Lines | case TOK_REDIR_PROTO: | ||||
i = setup_redir_port(&buf[off], &ac, &av); | i = setup_redir_port(&buf[off], &ac, &av); | ||||
break; | break; | ||||
case TOK_REDIR_PROTO: | case TOK_REDIR_PROTO: | ||||
i = setup_redir_proto(&buf[off], &ac, &av); | i = setup_redir_proto(&buf[off], &ac, &av); | ||||
break; | break; | ||||
} | } | ||||
n->redir_cnt++; | n->redir_cnt++; | ||||
off += i; | off += i; | ||||
break; | |||||
case TOK_PORT_ALIAS: | |||||
if (ac == 0) | |||||
errx(EX_DATAERR, "missing option"); | |||||
if (!(lp = nat_port_alias_parse(av[0])) || | |||||
!(hp = nat_port_alias_parse(av[1]))) | |||||
Done Inline ActionsIs this case already covered by the validation functions? If yes, remove those check. donner: Is this case already covered by the validation functions? If yes, remove those check. | |||||
errx(EX_DATAERR, "port has to be an integer from 1024 <= x < 65536"); | |||||
if (lp >= hp) | |||||
Done Inline ActionsUsing a other type of function call this would be (0 < (hp = nat_port_...(av[1]))) which is easier to audit. donner: Using a other type of function call this would be
```
(0 < (hp = nat_port_...(av[1])))
```… | |||||
errx(EX_DATAERR, "upper port has to be greater than lower port"); | |||||
n->alias_port_range.lower = lp; | |||||
n->alias_port_range.upper = hp; | |||||
ac -= 2; | |||||
av += 2; | |||||
break; | break; | ||||
} | } | ||||
} | } | ||||
i = do_set3(IP_FW_NAT44_XCONFIG, &oh->opheader, len); | i = do_set3(IP_FW_NAT44_XCONFIG, &oh->opheader, len); | ||||
Done Inline ActionsLine length. kp: Line length. | |||||
if (i != 0) | if (i != 0) | ||||
Done Inline Actions'You need' (missing 'e'), and line length. kp: 'You need' (missing 'e'), and line length. | |||||
err(1, "setsockopt(%s)", "IP_FW_NAT44_XCONFIG"); | err(1, "setsockopt(%s)", "IP_FW_NAT44_XCONFIG"); | ||||
Done Inline ActionsGiven the fact, that the upper limit is one above the usable range, this can't be specified, because the port number to configure is out of range. So it might be advisable to redefine the upper limit to be part of the interval (2000 2999 instead of 2000 3000). donner: Given the fact, that the upper limit is one above the usable range, this can't be specified… | |||||
Done Inline ActionsLine length, and small inconsistency in that some errors have their first word capitalised and some don't. kp: Line length, and small inconsistency in that some errors have their first word capitalised and… | |||||
if (!co.do_quiet) { | if (!co.do_quiet) { | ||||
/* After every modification, we show the resultant rule. */ | /* After every modification, we show the resultant rule. */ | ||||
int _ac = 3; | int _ac = 3; | ||||
const char *_av[] = {"show", "config", id}; | const char *_av[] = {"show", "config", id}; | ||||
ipfw_show_nat(_ac, (char **)(void *)_av); | ipfw_show_nat(_ac, (char **)(void *)_av); | ||||
} | } | ||||
} | } | ||||
▲ Show 20 Lines • Show All 179 Lines • Show Last 20 Lines |
I'd prefer a function witch return the valid port number or 0 in the case of failure. Writable pointers in arguments open a special class of worms in debugging.