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 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 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? | |||||
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. | |||||
kpUnsubmitted Done Inline ActionsThere's some extraneous whitespace here. kp: There's some extraneous whitespace here. | |||||
/* Lower port parsing */ | |||||
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. | |||||
lp = (long) strtol(str, &ptr, 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 (lp < 1024 || lp > 65535) | |||||
Done Inline Actionsu_short comparsion with MAX_USHORT is not recommended. donner: u_short comparsion with MAX_USHORT is not recommended. | |||||
return 0; | |||||
if (!ptr || *ptr != '-') | |||||
return 0; | |||||
kpUnsubmitted Done Inline Actionsand here. kp: and here. | |||||
/* Upper port parsing */ | |||||
hp = (long) strtol(ptr, &ptr, 10); | |||||
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 (hp < 1024 || hp > 65535) | |||||
return 0; | |||||
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, "You ned a range of port(s) from 1024 <= x < 65536"); | |||||
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. | |||||
kpUnsubmitted Done Inline Actions'You need' (missing 'e'), and line length. kp: 'You need' (missing 'e'), and line length. | |||||
if (lp >= hp) | |||||
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… | |||||
errx(EX_DATAERR, "upper port has to be greater than lower port"); | |||||
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])))
```… | |||||
kpUnsubmitted 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… | |||||
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"); | |||||
kpUnsubmitted 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 (!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; | ||||
▲ Show 20 Lines • Show All 184 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.