From 87a0117afdfe64473a6c802501bc15aee145ebb8 Mon Sep 17 00:00:00 2001 From: Patrick McHardy Date: Wed, 20 Sep 2006 12:10:21 -0700 Subject: [PATCH] [NETFILTER]: PPTP conntrack: clean up debugging cruft Also make sure not to hand packets received in an invalid state to the NAT helper since it will mangle the packet with invalid data. Signed-off-by: Patrick McHardy Signed-off-by: David S. Miller --- net/ipv4/netfilter/ip_conntrack_helper_pptp.c | 128 +++++++----------- 1 file changed, 51 insertions(+), 77 deletions(-) diff --git a/net/ipv4/netfilter/ip_conntrack_helper_pptp.c b/net/ipv4/netfilter/ip_conntrack_helper_pptp.c index 9a98a6ce19..7b6d5aaca4 100644 --- a/net/ipv4/netfilter/ip_conntrack_helper_pptp.c +++ b/net/ipv4/netfilter/ip_conntrack_helper_pptp.c @@ -301,7 +301,7 @@ pptp_inbound_pkt(struct sk_buff **pskb, { struct ip_ct_pptp_master *info = &ct->help.ct_pptp_info; u_int16_t msg; - __be16 cid, pcid; + __be16 cid = 0, pcid = 0; msg = ntohs(ctlh->messageType); DEBUGP("inbound control message %s\n", pptp_msg_name[msg]); @@ -309,11 +309,8 @@ pptp_inbound_pkt(struct sk_buff **pskb, switch (msg) { case PPTP_START_SESSION_REPLY: /* server confirms new control session */ - if (info->sstate < PPTP_SESSION_REQUESTED) { - DEBUGP("%s without START_SESS_REQUEST\n", - pptp_msg_name[msg]); - break; - } + if (info->sstate < PPTP_SESSION_REQUESTED) + goto invalid; if (pptpReq->srep.resultCode == PPTP_START_OK) info->sstate = PPTP_SESSION_CONFIRMED; else @@ -322,11 +319,8 @@ pptp_inbound_pkt(struct sk_buff **pskb, case PPTP_STOP_SESSION_REPLY: /* server confirms end of control session */ - if (info->sstate > PPTP_SESSION_STOPREQ) { - DEBUGP("%s without STOP_SESS_REQUEST\n", - pptp_msg_name[msg]); - break; - } + if (info->sstate > PPTP_SESSION_STOPREQ) + goto invalid; if (pptpReq->strep.resultCode == PPTP_STOP_OK) info->sstate = PPTP_SESSION_NONE; else @@ -335,15 +329,12 @@ pptp_inbound_pkt(struct sk_buff **pskb, case PPTP_OUT_CALL_REPLY: /* server accepted call, we now expect GRE frames */ - if (info->sstate != PPTP_SESSION_CONFIRMED) { - DEBUGP("%s but no session\n", pptp_msg_name[msg]); - break; - } + if (info->sstate != PPTP_SESSION_CONFIRMED) + goto invalid; if (info->cstate != PPTP_CALL_OUT_REQ && - info->cstate != PPTP_CALL_OUT_CONF) { - DEBUGP("%s without OUTCALL_REQ\n", pptp_msg_name[msg]); - break; - } + info->cstate != PPTP_CALL_OUT_CONF) + goto invalid; + if (pptpReq->ocack.resultCode != PPTP_OUTCALL_CONNECT) { info->cstate = PPTP_CALL_NONE; break; @@ -354,11 +345,8 @@ pptp_inbound_pkt(struct sk_buff **pskb, info->pac_call_id = cid; - if (info->pns_call_id != pcid) { - DEBUGP("%s for unknown callid %u\n", - pptp_msg_name[msg], ntohs(pcid)); - break; - } + if (info->pns_call_id != pcid) + goto invalid; DEBUGP("%s, CID=%X, PCID=%X\n", pptp_msg_name[msg], ntohs(cid), ntohs(pcid)); @@ -370,10 +358,9 @@ pptp_inbound_pkt(struct sk_buff **pskb, case PPTP_IN_CALL_REQUEST: /* server tells us about incoming call request */ - if (info->sstate != PPTP_SESSION_CONFIRMED) { - DEBUGP("%s but no session\n", pptp_msg_name[msg]); - break; - } + if (info->sstate != PPTP_SESSION_CONFIRMED) + goto invalid; + pcid = pptpReq->icack.peersCallID; DEBUGP("%s, PCID=%X\n", pptp_msg_name[msg], ntohs(pcid)); info->cstate = PPTP_CALL_IN_REQ; @@ -382,25 +369,17 @@ pptp_inbound_pkt(struct sk_buff **pskb, case PPTP_IN_CALL_CONNECT: /* server tells us about incoming call established */ - if (info->sstate != PPTP_SESSION_CONFIRMED) { - DEBUGP("%s but no session\n", pptp_msg_name[msg]); - break; - } - if (info->cstate != PPTP_CALL_IN_REP - && info->cstate != PPTP_CALL_IN_CONF) { - DEBUGP("%s but never sent IN_CALL_REPLY\n", - pptp_msg_name[msg]); - break; - } + if (info->sstate != PPTP_SESSION_CONFIRMED) + goto invalid; + if (info->cstate != PPTP_CALL_IN_REP && + info->cstate != PPTP_CALL_IN_CONF) + goto invalid; pcid = pptpReq->iccon.peersCallID; cid = info->pac_call_id; - if (info->pns_call_id != pcid) { - DEBUGP("%s for unknown CallID %u\n", - pptp_msg_name[msg], ntohs(pcid)); - break; - } + if (info->pns_call_id != pcid) + goto invalid; DEBUGP("%s, PCID=%X\n", pptp_msg_name[msg], ntohs(pcid)); info->cstate = PPTP_CALL_IN_CONF; @@ -425,18 +404,21 @@ pptp_inbound_pkt(struct sk_buff **pskb, /* I don't have to explain these ;) */ break; default: - DEBUGP("invalid %s (TY=%d)\n", (msg <= PPTP_MSG_MAX) - ? pptp_msg_name[msg]:pptp_msg_name[0], msg); - break; + goto invalid; } - if (ip_nat_pptp_hook_inbound) return ip_nat_pptp_hook_inbound(pskb, ct, ctinfo, ctlh, pptpReq); - return NF_ACCEPT; +invalid: + DEBUGP("invalid %s: type=%d cid=%u pcid=%u " + "cstate=%d sstate=%d pns_cid=%u pac_cid=%u\n", + msg <= PPTP_MSG_MAX ? pptp_msg_name[msg] : pptp_msg_name[0], + msg, ntohs(cid), ntohs(pcid), info->cstate, info->sstate, + ntohs(info->pns_call_id), ntohs(info->pac_call_id)); + return NF_ACCEPT; } static inline int @@ -449,7 +431,7 @@ pptp_outbound_pkt(struct sk_buff **pskb, { struct ip_ct_pptp_master *info = &ct->help.ct_pptp_info; u_int16_t msg; - __be16 cid, pcid; + __be16 cid = 0, pcid = 0; msg = ntohs(ctlh->messageType); DEBUGP("outbound control message %s\n", pptp_msg_name[msg]); @@ -457,10 +439,8 @@ pptp_outbound_pkt(struct sk_buff **pskb, switch (msg) { case PPTP_START_SESSION_REQUEST: /* client requests for new control session */ - if (info->sstate != PPTP_SESSION_NONE) { - DEBUGP("%s but we already have one", - pptp_msg_name[msg]); - } + if (info->sstate != PPTP_SESSION_NONE) + goto invalid; info->sstate = PPTP_SESSION_REQUESTED; break; case PPTP_STOP_SESSION_REQUEST: @@ -470,11 +450,8 @@ pptp_outbound_pkt(struct sk_buff **pskb, case PPTP_OUT_CALL_REQUEST: /* client initiating connection to server */ - if (info->sstate != PPTP_SESSION_CONFIRMED) { - DEBUGP("%s but no session\n", - pptp_msg_name[msg]); - break; - } + if (info->sstate != PPTP_SESSION_CONFIRMED) + goto invalid; info->cstate = PPTP_CALL_OUT_REQ; /* track PNS call id */ cid = pptpReq->ocreq.callID; @@ -483,22 +460,17 @@ pptp_outbound_pkt(struct sk_buff **pskb, break; case PPTP_IN_CALL_REPLY: /* client answers incoming call */ - if (info->cstate != PPTP_CALL_IN_REQ - && info->cstate != PPTP_CALL_IN_REP) { - DEBUGP("%s without incall_req\n", - pptp_msg_name[msg]); - break; - } + if (info->cstate != PPTP_CALL_IN_REQ && + info->cstate != PPTP_CALL_IN_REP) + goto invalid; + if (pptpReq->icack.resultCode != PPTP_INCALL_ACCEPT) { info->cstate = PPTP_CALL_NONE; break; } pcid = pptpReq->icack.peersCallID; - if (info->pac_call_id != pcid) { - DEBUGP("%s for unknown call %u\n", - pptp_msg_name[msg], ntohs(pcid)); - break; - } + if (info->pac_call_id != pcid) + goto invalid; DEBUGP("%s, CID=%X\n", pptp_msg_name[msg], ntohs(pcid)); /* part two of the three-way handshake */ info->cstate = PPTP_CALL_IN_REP; @@ -507,10 +479,8 @@ pptp_outbound_pkt(struct sk_buff **pskb, case PPTP_CALL_CLEAR_REQUEST: /* client requests hangup of call */ - if (info->sstate != PPTP_SESSION_CONFIRMED) { - DEBUGP("CLEAR_CALL but no session\n"); - break; - } + if (info->sstate != PPTP_SESSION_CONFIRMED) + goto invalid; /* FUTURE: iterate over all calls and check if * call ID is valid. We don't do this without newnat, * because we only know about last call */ @@ -522,16 +492,20 @@ pptp_outbound_pkt(struct sk_buff **pskb, /* I don't have to explain these ;) */ break; default: - DEBUGP("invalid %s (TY=%d)\n", (msg <= PPTP_MSG_MAX)? - pptp_msg_name[msg]:pptp_msg_name[0], msg); - /* unknown: no need to create GRE masq table entry */ - break; + goto invalid; } if (ip_nat_pptp_hook_outbound) return ip_nat_pptp_hook_outbound(pskb, ct, ctinfo, ctlh, pptpReq); + return NF_ACCEPT; +invalid: + DEBUGP("invalid %s: type=%d cid=%u pcid=%u " + "cstate=%d sstate=%d pns_cid=%u pac_cid=%u\n", + msg <= PPTP_MSG_MAX ? pptp_msg_name[msg] : pptp_msg_name[0], + msg, ntohs(cid), ntohs(pcid), info->cstate, info->sstate, + ntohs(info->pns_call_id), ntohs(info->pac_call_id)); return NF_ACCEPT; } -- 2.39.5