Skip to content

Commit

Permalink
netfilter: nf_conntrack_pptp: prevent buffer overflows in debug code
Browse files Browse the repository at this point in the history 
Dan Carpenter says: "Smatch complains that the value for "cmd" comes
from the network and can't be trusted."

Add pptp_msg_name() helper function that checks for the array boundary.

Fixes: f09943f ("[NETFILTER]: nf_conntrack/nf_nat: add PPTP helper port")
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
  • Loading branch information
@ummakynes
ummakynes committed May 25, 2020
1 parent a164b95 commit 4c559f1
Show file tree
Hide file tree
Showing 3 changed files with 38 additions and 33 deletions.
2 changes: 1 addition & 1 deletion include/linux/netfilter/nf_conntrack_pptp.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
#include <net/netfilter/nf_conntrack_expect.h>
#include <uapi/linux/netfilter/nf_conntrack_tuple_common.h>

extern const char *const pptp_msg_name[];
extern const char *const pptp_msg_name(u_int16_t msg);

/* state of the control session */
enum pptp_ctrlsess_state {
Expand Down
7 changes: 2 additions & 5 deletions net/ipv4/netfilter/nf_nat_pptp.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -166,8 +166,7 @@ pptp_outbound_pkt(struct sk_buff *skb,
break;
default:
pr_debug("unknown outbound packet 0x%04x:%s\n", msg,
msg <= PPTP_MSG_MAX ? pptp_msg_name[msg] :
pptp_msg_name[0]);
pptp_msg_name(msg));
fallthrough;
case PPTP_SET_LINK_INFO:
/* only need to NAT in case PAC is behind NAT box */
Expand Down Expand Up @@ -268,9 +267,7 @@ pptp_inbound_pkt(struct sk_buff *skb,
pcid_off = offsetof(union pptp_ctrl_union, setlink.peersCallID);
break;
default:
pr_debug("unknown inbound packet %s\n",
msg <= PPTP_MSG_MAX ? pptp_msg_name[msg] :
pptp_msg_name[0]);
pr_debug("unknown inbound packet %s\n", pptp_msg_name(msg));
fallthrough;
case PPTP_START_SESSION_REQUEST:
case PPTP_START_SESSION_REPLY:
Expand Down
62 changes: 35 additions & 27 deletions net/netfilter/nf_conntrack_pptp.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,24 +72,32 @@ EXPORT_SYMBOL_GPL(nf_nat_pptp_hook_expectfn);

#if defined(DEBUG) || defined(CONFIG_DYNAMIC_DEBUG)
/* PptpControlMessageType names */
const char *const pptp_msg_name[] = {
"UNKNOWN_MESSAGE",
"START_SESSION_REQUEST",
"START_SESSION_REPLY",
"STOP_SESSION_REQUEST",
"STOP_SESSION_REPLY",
"ECHO_REQUEST",
"ECHO_REPLY",
"OUT_CALL_REQUEST",
"OUT_CALL_REPLY",
"IN_CALL_REQUEST",
"IN_CALL_REPLY",
"IN_CALL_CONNECT",
"CALL_CLEAR_REQUEST",
"CALL_DISCONNECT_NOTIFY",
"WAN_ERROR_NOTIFY",
"SET_LINK_INFO"
static const char *const pptp_msg_name_array[PPTP_MSG_MAX + 1] = {
[0] = "UNKNOWN_MESSAGE",
[PPTP_START_SESSION_REQUEST] = "START_SESSION_REQUEST",
[PPTP_START_SESSION_REPLY] = "START_SESSION_REPLY",
[PPTP_STOP_SESSION_REQUEST] = "STOP_SESSION_REQUEST",
[PPTP_STOP_SESSION_REPLY] = "STOP_SESSION_REPLY",
[PPTP_ECHO_REQUEST] = "ECHO_REQUEST",
[PPTP_ECHO_REPLY] = "ECHO_REPLY",
[PPTP_OUT_CALL_REQUEST] = "OUT_CALL_REQUEST",
[PPTP_OUT_CALL_REPLY] = "OUT_CALL_REPLY",
[PPTP_IN_CALL_REQUEST] = "IN_CALL_REQUEST",
[PPTP_IN_CALL_REPLY] = "IN_CALL_REPLY",
[PPTP_IN_CALL_CONNECT] = "IN_CALL_CONNECT",
[PPTP_CALL_CLEAR_REQUEST] = "CALL_CLEAR_REQUEST",
[PPTP_CALL_DISCONNECT_NOTIFY] = "CALL_DISCONNECT_NOTIFY",
[PPTP_WAN_ERROR_NOTIFY] = "WAN_ERROR_NOTIFY",
[PPTP_SET_LINK_INFO] = "SET_LINK_INFO"
};

const char *const pptp_msg_name(u_int16_t msg)
{
if (msg > PPTP_MSG_MAX)
return pptp_msg_name_array[0];

return pptp_msg_name_array[msg];
}
EXPORT_SYMBOL(pptp_msg_name);
#endif

Expand Down Expand Up @@ -276,7 +284,7 @@ pptp_inbound_pkt(struct sk_buff *skb, unsigned int protoff,
typeof(nf_nat_pptp_hook_inbound) nf_nat_pptp_inbound;

msg = ntohs(ctlh->messageType);
pr_debug("inbound control message %s\n", pptp_msg_name[msg]);
pr_debug("inbound control message %s\n", pptp_msg_name(msg));

switch (msg) {
case PPTP_START_SESSION_REPLY:
Expand Down Expand Up @@ -311,7 +319,7 @@ pptp_inbound_pkt(struct sk_buff *skb, unsigned int protoff,
pcid = pptpReq->ocack.peersCallID;
if (info->pns_call_id != pcid)
goto invalid;
pr_debug("%s, CID=%X, PCID=%X\n", pptp_msg_name[msg],
pr_debug("%s, CID=%X, PCID=%X\n", pptp_msg_name(msg),
ntohs(cid), ntohs(pcid));

if (pptpReq->ocack.resultCode == PPTP_OUTCALL_CONNECT) {
Expand All @@ -328,7 +336,7 @@ pptp_inbound_pkt(struct sk_buff *skb, unsigned int protoff,
goto invalid;

cid = pptpReq->icreq.callID;
pr_debug("%s, CID=%X\n", pptp_msg_name[msg], ntohs(cid));
pr_debug("%s, CID=%X\n", pptp_msg_name(msg), ntohs(cid));
info->cstate = PPTP_CALL_IN_REQ;
info->pac_call_id = cid;
break;
Expand All @@ -347,7 +355,7 @@ pptp_inbound_pkt(struct sk_buff *skb, unsigned int protoff,
if (info->pns_call_id != pcid)
goto invalid;

pr_debug("%s, PCID=%X\n", pptp_msg_name[msg], ntohs(pcid));
pr_debug("%s, PCID=%X\n", pptp_msg_name(msg), ntohs(pcid));
info->cstate = PPTP_CALL_IN_CONF;

/* we expect a GRE connection from PAC to PNS */
Expand All @@ -357,7 +365,7 @@ pptp_inbound_pkt(struct sk_buff *skb, unsigned int protoff,
case PPTP_CALL_DISCONNECT_NOTIFY:
/* server confirms disconnect */
cid = pptpReq->disc.callID;
pr_debug("%s, CID=%X\n", pptp_msg_name[msg], ntohs(cid));
pr_debug("%s, CID=%X\n", pptp_msg_name(msg), ntohs(cid));
info->cstate = PPTP_CALL_NONE;

/* untrack this call id, unexpect GRE packets */
Expand All @@ -384,7 +392,7 @@ pptp_inbound_pkt(struct sk_buff *skb, unsigned int protoff,
invalid:
pr_debug("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],
pptp_msg_name(msg),
msg, ntohs(cid), ntohs(pcid), info->cstate, info->sstate,
ntohs(info->pns_call_id), ntohs(info->pac_call_id));
return NF_ACCEPT;
Expand All @@ -404,7 +412,7 @@ pptp_outbound_pkt(struct sk_buff *skb, unsigned int protoff,
typeof(nf_nat_pptp_hook_outbound) nf_nat_pptp_outbound;

msg = ntohs(ctlh->messageType);
pr_debug("outbound control message %s\n", pptp_msg_name[msg]);
pr_debug("outbound control message %s\n", pptp_msg_name(msg));

switch (msg) {
case PPTP_START_SESSION_REQUEST:
Expand All @@ -426,7 +434,7 @@ pptp_outbound_pkt(struct sk_buff *skb, unsigned int protoff,
info->cstate = PPTP_CALL_OUT_REQ;
/* track PNS call id */
cid = pptpReq->ocreq.callID;
pr_debug("%s, CID=%X\n", pptp_msg_name[msg], ntohs(cid));
pr_debug("%s, CID=%X\n", pptp_msg_name(msg), ntohs(cid));
info->pns_call_id = cid;
break;

Expand All @@ -440,7 +448,7 @@ pptp_outbound_pkt(struct sk_buff *skb, unsigned int protoff,
pcid = pptpReq->icack.peersCallID;
if (info->pac_call_id != pcid)
goto invalid;
pr_debug("%s, CID=%X PCID=%X\n", pptp_msg_name[msg],
pr_debug("%s, CID=%X PCID=%X\n", pptp_msg_name(msg),
ntohs(cid), ntohs(pcid));

if (pptpReq->icack.resultCode == PPTP_INCALL_ACCEPT) {
Expand Down Expand Up @@ -480,7 +488,7 @@ pptp_outbound_pkt(struct sk_buff *skb, unsigned int protoff,
invalid:
pr_debug("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],
pptp_msg_name(msg),
msg, ntohs(cid), ntohs(pcid), info->cstate, info->sstate,
ntohs(info->pns_call_id), ntohs(info->pac_call_id));
return NF_ACCEPT;
Expand Down

0 comments on commit 4c559f1

Please sign in to comment.