Skip to content

Commit 4c559f1

Browse files
ummakynesummakynes
committed
netfilter: nf_conntrack_pptp: prevent buffer overflows in debug code
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>
1 parent a164b95 commit 4c559f1

File tree

3 files changed

+38
-33
lines changed

3 files changed

+38
-33
lines changed

‎include/linux/netfilter/nf_conntrack_pptp.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
#include <net/netfilter/nf_conntrack_expect.h>
1111
#include <uapi/linux/netfilter/nf_conntrack_tuple_common.h>
1212

13-
extern const char *const pptp_msg_name[];
13+
extern const char *const pptp_msg_name(u_int16_t msg);
1414

1515
/* state of the control session */
1616
enum pptp_ctrlsess_state {

‎net/ipv4/netfilter/nf_nat_pptp.c‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -166,8 +166,7 @@ pptp_outbound_pkt(struct sk_buff *skb,
166166
break;
167167
default:
168168
pr_debug("unknown outbound packet 0x%04x:%s\n", msg,
169-
msg <= PPTP_MSG_MAX ? pptp_msg_name[msg] :
170-
pptp_msg_name[0]);
169+
pptp_msg_name(msg));
171170
fallthrough;
172171
case PPTP_SET_LINK_INFO:
173172
/* only need to NAT in case PAC is behind NAT box */
@@ -268,9 +267,7 @@ pptp_inbound_pkt(struct sk_buff *skb,
268267
pcid_off = offsetof(union pptp_ctrl_union, setlink.peersCallID);
269268
break;
270269
default:
271-
pr_debug("unknown inbound packet %s\n",
272-
msg <= PPTP_MSG_MAX ? pptp_msg_name[msg] :
273-
pptp_msg_name[0]);
270+
pr_debug("unknown inbound packet %s\n", pptp_msg_name(msg));
274271
fallthrough;
275272
case PPTP_START_SESSION_REQUEST:
276273
case PPTP_START_SESSION_REPLY:

‎net/netfilter/nf_conntrack_pptp.c‎

Lines changed: 35 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -72,24 +72,32 @@ EXPORT_SYMBOL_GPL(nf_nat_pptp_hook_expectfn);
7272

7373
#if defined(DEBUG) || defined(CONFIG_DYNAMIC_DEBUG)
7474
/* PptpControlMessageType names */
75-
const char *const pptp_msg_name[] = {
76-
"UNKNOWN_MESSAGE",
77-
"START_SESSION_REQUEST",
78-
"START_SESSION_REPLY",
79-
"STOP_SESSION_REQUEST",
80-
"STOP_SESSION_REPLY",
81-
"ECHO_REQUEST",
82-
"ECHO_REPLY",
83-
"OUT_CALL_REQUEST",
84-
"OUT_CALL_REPLY",
85-
"IN_CALL_REQUEST",
86-
"IN_CALL_REPLY",
87-
"IN_CALL_CONNECT",
88-
"CALL_CLEAR_REQUEST",
89-
"CALL_DISCONNECT_NOTIFY",
90-
"WAN_ERROR_NOTIFY",
91-
"SET_LINK_INFO"
75+
static const char *const pptp_msg_name_array[PPTP_MSG_MAX + 1] = {
76+
[0] = "UNKNOWN_MESSAGE",
77+
[PPTP_START_SESSION_REQUEST] = "START_SESSION_REQUEST",
78+
[PPTP_START_SESSION_REPLY] = "START_SESSION_REPLY",
79+
[PPTP_STOP_SESSION_REQUEST] = "STOP_SESSION_REQUEST",
80+
[PPTP_STOP_SESSION_REPLY] = "STOP_SESSION_REPLY",
81+
[PPTP_ECHO_REQUEST] = "ECHO_REQUEST",
82+
[PPTP_ECHO_REPLY] = "ECHO_REPLY",
83+
[PPTP_OUT_CALL_REQUEST] = "OUT_CALL_REQUEST",
84+
[PPTP_OUT_CALL_REPLY] = "OUT_CALL_REPLY",
85+
[PPTP_IN_CALL_REQUEST] = "IN_CALL_REQUEST",
86+
[PPTP_IN_CALL_REPLY] = "IN_CALL_REPLY",
87+
[PPTP_IN_CALL_CONNECT] = "IN_CALL_CONNECT",
88+
[PPTP_CALL_CLEAR_REQUEST] = "CALL_CLEAR_REQUEST",
89+
[PPTP_CALL_DISCONNECT_NOTIFY] = "CALL_DISCONNECT_NOTIFY",
90+
[PPTP_WAN_ERROR_NOTIFY] = "WAN_ERROR_NOTIFY",
91+
[PPTP_SET_LINK_INFO] = "SET_LINK_INFO"
9292
};
93+
94+
const char *const pptp_msg_name(u_int16_t msg)
95+
{
96+
if (msg > PPTP_MSG_MAX)
97+
return pptp_msg_name_array[0];
98+
99+
return pptp_msg_name_array[msg];
100+
}
93101
EXPORT_SYMBOL(pptp_msg_name);
94102
#endif
95103

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

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

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

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

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

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

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

363371
/* untrack this call id, unexpect GRE packets */
@@ -384,7 +392,7 @@ pptp_inbound_pkt(struct sk_buff *skb, unsigned int protoff,
384392
invalid:
385393
pr_debug("invalid %s: type=%d cid=%u pcid=%u "
386394
"cstate=%d sstate=%d pns_cid=%u pac_cid=%u\n",
387-
msg <= PPTP_MSG_MAX ? pptp_msg_name[msg] : pptp_msg_name[0],
395+
pptp_msg_name(msg),
388396
msg, ntohs(cid), ntohs(pcid), info->cstate, info->sstate,
389397
ntohs(info->pns_call_id), ntohs(info->pac_call_id));
390398
return NF_ACCEPT;
@@ -404,7 +412,7 @@ pptp_outbound_pkt(struct sk_buff *skb, unsigned int protoff,
404412
typeof(nf_nat_pptp_hook_outbound) nf_nat_pptp_outbound;
405413

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

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

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

446454
if (pptpReq->icack.resultCode == PPTP_INCALL_ACCEPT) {
@@ -480,7 +488,7 @@ pptp_outbound_pkt(struct sk_buff *skb, unsigned int protoff,
480488
invalid:
481489
pr_debug("invalid %s: type=%d cid=%u pcid=%u "
482490
"cstate=%d sstate=%d pns_cid=%u pac_cid=%u\n",
483-
msg <= PPTP_MSG_MAX ? pptp_msg_name[msg] : pptp_msg_name[0],
491+
pptp_msg_name(msg),
484492
msg, ntohs(cid), ntohs(pcid), info->cstate, info->sstate,
485493
ntohs(info->pns_call_id), ntohs(info->pac_call_id));
486494
return NF_ACCEPT;

0 commit comments

Comments
 (0)