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_range", 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 672 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 int | |||||
nat_port_alias_parse(char *str, u_short *lpout, u_short *hpout) { | |||||
donner: I'd prefer a function witch return the valid port number or 0 in the case of failure. Writable… | |||||
long lp, hp; | |||||
char *ptr; | |||||
Done Inline ActionsDid you try "12345six" as a port number? donner: Did you try "12345six" as a port number? | |||||
/* Lower port parsing */ | |||||
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. | |||||
lp = (long) strtol(str, &ptr, 10); | |||||
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. | |||||
if (lp < 1024 || lp > 65535) | |||||
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… | |||||
return 0; | |||||
Done Inline Actionsu_short comparsion with MAX_USHORT is not recommended. donner: u_short comparsion with MAX_USHORT is not recommended. | |||||
if (!ptr || *ptr != '-') | |||||
return 0; | |||||
/* Upper port parsing */ | |||||
hp = (long) strtol(ptr, &ptr, 10); | |||||
Done Inline Actionsand here. kp: and here. | |||||
if (hp < 1024 || hp > 65535) | |||||
return 0; | |||||
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? | |||||
if (ptr) | |||||
return 0; | |||||
*lpout = (u_short) lp; | |||||
*hpout = (u_short) hp; | |||||
return 1; | |||||
} | |||||
void | void | ||||
ipfw_config_nat(int ac, char **av) | ipfw_config_nat(int ac, char **av) | ||||
{ | { | ||||
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 All 11 Lines | ipfw_config_nat(int ac, char **av) | ||||
av1 = av; | av1 = av; | ||||
while (ac1 > 0) { | while (ac1 > 0) { | ||||
tok = match_token(nat_params, *av1); | tok = match_token(nat_params, *av1); | ||||
ac1--; | ac1--; | ||||
av1++; | av1++; | ||||
switch (tok) { | switch (tok) { | ||||
case TOK_IP: | case TOK_IP: | ||||
case TOK_IF: | case TOK_IF: | ||||
case TOK_PORT_ALIAS: | |||||
ac1--; | ac1--; | ||||
av1++; | av1++; | ||||
break; | break; | ||||
case TOK_ALOG: | case TOK_ALOG: | ||||
case TOK_DENY_INC: | case TOK_DENY_INC: | ||||
case TOK_SAME_PORTS: | case TOK_SAME_PORTS: | ||||
case TOK_SKIP_GLOBAL: | case TOK_SKIP_GLOBAL: | ||||
case TOK_UNREG_ONLY: | case TOK_UNREG_ONLY: | ||||
▲ Show 20 Lines • Show All 123 Lines • ▼ Show 20 Lines | case TOK_REDIR_PROTO: | ||||
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; | break; | ||||
case TOK_PORT_ALIAS: | |||||
if (ac == 0) | |||||
errx(EX_DATAERR, "missing option"); | |||||
if (!nat_port_alias_parse(av[0], &lp, &hp)) | |||||
errx(EX_DATAERR, | |||||
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. | |||||
Done Inline Actions'You need' (missing 'e'), and line length. kp: 'You need' (missing 'e'), and line length. | |||||
"You need a range of port(s) from 1024 <= x < 65536"); | |||||
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… | |||||
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])))
```… | |||||
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… | |||||
errx(EX_DATAERR, | |||||
"Upper port has to be greater than lower port"); | |||||
n->alias_port_lo = lp; | |||||
n->alias_port_hi = hp; | |||||
ac--; | |||||
av++; | |||||
break; | |||||
} | } | ||||
} | } | ||||
if (n->mode & PKT_ALIAS_SAME_PORTS && n->alias_port_lo) | |||||
errx(EX_DATAERR, "same_ports and port_range cannot both be selected"); | |||||
Done Inline ActionsLine length. kp: Line length. | |||||
i = do_set3(IP_FW_NAT44_XCONFIG, &oh->opheader, len); | i = do_set3(IP_FW_NAT44_XCONFIG, &oh->opheader, len); | ||||
if (i != 0) | if (i != 0) | ||||
err(1, "setsockopt(%s)", "IP_FW_NAT44_XCONFIG"); | err(1, "setsockopt(%s)", "IP_FW_NAT44_XCONFIG"); | ||||
if (!g_co.do_quiet) { | if (!g_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; | ||||
▲ Show 20 Lines • Show All 213 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.