On Tuesday 2010-03-16 17:28, Thomas Woerner wrote: >1) Newer glibc versions are checking for overflows in targets of string >functions. Therefore you should use memcpy instead of strcpy. The >target is 29 chars while the source is up to 30 chars. So if it checks for *string* functions (I think it only does that when -D_FORTIFY_SOURCE is set), why should I use a *memory* function over the string function that's already in? >2) If the name is 30 chars +1 for '\0' and +1 for revision to make 32, >why is xt_entry_match.user.name 29 chars in size then? Oh, for match_size/target_size. So it's 29 +1 +1 +2. I shall revise the patch immediately. >revision will contain the last byte then if the memory alignment is 1 >byte, right? For 4 byte alignments this will be not the case. See ia64 >and others. __u8 won't be aligned on a 4-byte boundary (ignoring the weird ARM ABI). parent 89b6c32f88be47e83c3f6e7f8fee812088cb8c22 (v1.4.7-3-g89b6c32) commit 21d1283750d9c4df7ca80165d2b9dc0b9bd214eb Author: Jan Engelhardt <jengelh@xxxxxxxxxx> Date: Tue Mar 16 16:49:21 2010 +0100 iptables: correctly check for too-long chain/target/match names * iptables-restore was not checking for chain name length * iptables was not checking for match name length * target length was checked against 32, not 29. References: http://bugzilla.netfilter.org/show_bug.cgi?id=641 Signed-off-by: Jan Engelhardt <jengelh@xxxxxxxxxx> --- ip6tables-restore.c | 6 ++++++ ip6tables.c | 4 ++-- iptables-restore.c | 6 ++++++ iptables.c | 4 ++-- xtables.c | 5 +++++ 5 files changed, 21 insertions(+), 4 deletions(-) diff --git a/ip6tables-restore.c b/ip6tables-restore.c index d0efbee..f0725d1 100644 --- a/ip6tables-restore.c +++ b/ip6tables-restore.c @@ -253,6 +253,12 @@ int main(int argc, char *argv[]) exit(1); } + if (strlen(chain) > XT_FUNCTION_MAXNAMELEN - 1) + xtables_error(PARAMETER_PROBLEM, + "Invalid chain name `%s' " + "(%u chars max)", + chain, XT_FUNCTION_MAXNAMELEN - 1); + if (ip6tc_builtin(chain, handle) <= 0) { if (noflush && ip6tc_is_chain(chain, handle)) { DEBUGP("Flushing existing user defined chain '%s'\n", chain); diff --git a/ip6tables.c b/ip6tables.c index e2359df..6ee4281 100644 --- a/ip6tables.c +++ b/ip6tables.c @@ -456,10 +456,10 @@ parse_target(const char *targetname) xtables_error(PARAMETER_PROBLEM, "Invalid target name (too short)"); - if (strlen(targetname)+1 > sizeof(ip6t_chainlabel)) + if (strlen(targetname) > XT_FUNCTION_MAXNAMELEN - 1) xtables_error(PARAMETER_PROBLEM, "Invalid target name `%s' (%u chars max)", - targetname, (unsigned int)sizeof(ip6t_chainlabel)-1); + targetname, XT_FUNCTION_MAXNAMELEN - 1); for (ptr = targetname; *ptr; ptr++) if (isspace(*ptr)) diff --git a/iptables-restore.c b/iptables-restore.c index 86d63e2..4a74485 100644 --- a/iptables-restore.c +++ b/iptables-restore.c @@ -259,6 +259,12 @@ main(int argc, char *argv[]) exit(1); } + if (strlen(chain) > XT_FUNCTION_MAXNAMELEN - 1) + xtables_error(PARAMETER_PROBLEM, + "Invalid chain name `%s' " + "(%u chars max)", + chain, XT_FUNCTION_MAXNAMELEN - 1); + if (iptc_builtin(chain, handle) <= 0) { if (noflush && iptc_is_chain(chain, handle)) { DEBUGP("Flushing existing user defined chain '%s'\n", chain); diff --git a/iptables.c b/iptables.c index 08eb134..25bc8cc 100644 --- a/iptables.c +++ b/iptables.c @@ -460,10 +460,10 @@ parse_target(const char *targetname) xtables_error(PARAMETER_PROBLEM, "Invalid target name (too short)"); - if (strlen(targetname)+1 > sizeof(ipt_chainlabel)) + if (strlen(targetname) > XT_FUNCTION_MAXNAMELEN - 1) xtables_error(PARAMETER_PROBLEM, "Invalid target name `%s' (%u chars max)", - targetname, (unsigned int)sizeof(ipt_chainlabel)-1); + targetname, XT_FUNCTION_MAXNAMELEN - 1); for (ptr = targetname; *ptr; ptr++) if (isspace(*ptr)) diff --git a/xtables.c b/xtables.c index f3baf84..7340c87 100644 --- a/xtables.c +++ b/xtables.c @@ -545,6 +545,11 @@ xtables_find_match(const char *name, enum xtables_tryload tryload, struct xtables_match *ptr; const char *icmp6 = "icmp6"; + if (strlen(name) > XT_FUNCTION_MAXNAMELEN - 1) + xtables_error(PARAMETER_PROBLEM, + "Invalid match name \"%s\" (%u chars max)", + name, XT_FUNCTION_MAXNAMELEN - 1); + /* This is ugly as hell. Nonetheless, there is no way of changing * this without hurting backwards compatibility */ if ( (strcmp(name,"icmpv6") == 0) || -- # Created with git-export-patch -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
