lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Tue, 02 Jun 2009 14:40:01 +0200 From: Patrick McHardy <kaber@...sh.net> To: Evgeniy Polyakov <zbr@...emap.net> CC: netdev@...r.kernel.org, David Miller <davem@...emloft.net>, "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>, Netfilter Development Mailinglist <netfilter-devel@...r.kernel.org>, Jan Engelhardt <jengelh@...ozas.de> Subject: Re: [resend] Passive OS fingerprint xtables match. Evgeniy Polyakov wrote: > Hi Patrick. > > Thanks for your comments. > > On Wed, May 27, 2009 at 06:28:16PM +0200, Patrick McHardy (kaber@...sh.net) wrote: >>> +#define XT_OSF_TTL_TRUE 0 /* True ip and fingerprint >>> TTL comparison */ >>> +#define XT_OSF_TTL_LESS 1 /* Check if ip TTL is less >>> than fingerprint one */ >>> +#define XT_OSF_TTL_NOCHECK 2 /* Do not compare ip and fingerprint >>> TTL at all */ >> These seem redundant - having neither of TRUE or LESS seems >> equivalent to NOCHECK. Perhaps thats the reason why its not >> used at all :) Looking at the code, "TRUE" would be better >> named as "EQUAL". > > There are only three types of TTL check - equal (for true), less than > fingerprint one and when no check is performed at all. NOCHECK is > actually used, but LESS one does not have a special check, but a default > clause when neither TRUE or NOCHECK is specified. OK, thanks for the explanation. >>> +struct xt_osf_user_finger { >>> + struct xt_osf_wc wss; >>> + >>> + __u8 ttl, df; >>> + __u16 ss, mss; >>> + __u16 opt_num; >>> + >>> + char genre[MAXGENRELEN]; >>> + char version[MAXGENRELEN]; >>> + char subtype[MAXGENRELEN]; >>> + >>> + /* MAX_IPOPTLEN is maximum if all options are NOPs or EOLs */ >>> + struct xt_osf_opt opt[MAX_IPOPTLEN]; >> This really looks like you should use nested attributes. > > This will be an unneded complication - we should provide an option > sequence, and maximum number of options is strickly determined by > the protocol specification. How does having separate attributes for each > option simplify the process? It doesn't, but it provides more flexibility which might make things easier in case someone decides to add IPv6 support. >>> +/* Defines for IANA option kinds */ >>> + >>> +enum iana_options { >>> + OSFOPT_EOL = 0, /* End of options */ >>> + OSFOPT_NOP, /* NOP */ >>> + OSFOPT_MSS, /* Maximum segment size */ >>> + OSFOPT_WSO, /* Window scale option */ >>> + OSFOPT_SACKP, /* SACK permitted */ >>> + OSFOPT_SACK, /* SACK */ >>> + OSFOPT_ECHO, >>> + OSFOPT_ECHOREPLY, >>> + OSFOPT_TS, /* Timestamp option */ >>> + OSFOPT_POCP, /* Partial Order Connection Permitted */ >>> + OSFOPT_POSP, /* Partial Order Service Profile */ >>> + >>> + /* Others are not used in the current OSF */ >>> + OSFOPT_EMPTY = 255, >>> +}; >> Why do we need to duplicate these? > > Why duplicate? It is the only place of the constants describing used > option numbers. include/net/tcp.h does not have 'partial order' options > in particular. Indeed, I noticed this after sending my mail. >>> +struct xt_osf_finger_storage >>> +{ >> Please place the opening bracket consistently with the other >> structure definitions. > > I.e. always on the new line? :) Just keep it consistent within your file, though I personally prefer to keep it inconsistent with some of the other netfilter files and not use a new line :) >>> +static int xt_osf_setup_callback(struct sock *ctnl, struct sk_buff *skb, >>> + struct nlmsghdr *nlh, struct nlattr *osf_attrs[]) >>> +{ >>> + struct xt_osf_user_finger *f; >>> + struct nfgenmsg *nfmsg = NLMSG_DATA(nlh); >>> + u16 delete = ntohs(nfmsg->res_id); >> This looks like abuse, we use message types to distinguish between >> additions and deletions, alternative NLM_F_REPLACE. > > Why to introduce the whole new callback function and attribute when the > only difference is to add or remove a processed entry? Sticking to the defined semantics avoids the need for special-casing in generic code like libnl. A new function also doesn't seem like a loss at all, the only common part between addition and removal appears to be the iteration. >>> + if (totlen == f->ss && xt_osf_ttl(skb, info, f->ttl)) { >>> + int foptsize, optnum; >>> + >>> + check_WSS = 0; >>> + >>> + switch (f->wss.wc) { >>> + case 0: >>> + check_WSS = 0; >>> + break; >>> + case 'S': >>> + check_WSS = 1; >>> + break; >>> + case 'T': >>> + check_WSS = 2; >>> + break; >>> + case '%': >>> + check_WSS = 3; >>> + break; >>> + default: >>> + check_WSS = 4; >>> + break; >>> + } >> This is really pushing my taste-buds. Whatever this does, please at >> use symbolic constants so the reader at least has a chance to understand >> it. > > That's a bit unlcear window size processing state machine. > It links together knowledge about window-size, mss, mtu dependancy on > plain size numbers and modulo operations (like window size is multiple > of x bytes). It very much reminds me of iptables userspace option parsing :| Please at least use symbolic names for the different cases. Why does it have to be mapped in the kernel at all? The raw value of f->wss.wc doesn't seem to be used anywhere else. >>> +#define SMART_MSS_1 1460 >>> +#define SMART_MSS_2 1448 >> Sigh. This entire function is completely unreadable and full of >> unexplained magic. I'll stop here, please clean this before >> resubmitting. > > This is a special hack for special modems, which can decrease mss, and > since there is no common knowledge on how to differentiate them, there > is a check against different types. Please explain what exactly it does in a comment. Would it make sense to have the exact values supplied by userspace? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists