* [PATCH 1/1] netfilter: h323: avoid potential attack
@ 2016-01-27 14:40 Zhouyi Zhou
2016-01-27 15:06 ` kbuild test robot
2016-01-27 15:57 ` Eric Dumazet
0 siblings, 2 replies; 3+ messages in thread
From: Zhouyi Zhou @ 2016-01-27 14:40 UTC (permalink / raw)
To: pablo, kaber, kadlec, davem, netfilter-devel, coreteam, netdev,
linux-kernel
Cc: Zhouyi Zhou, Zhouyi Zhou
From: Zhouyi Zhou <yizhouzhou@ict.ac.cn>
I think hackers chould build a malicious h323 packet to overflow
the pointer p which will panic during the memcpy(addr, p, len)
For example, he may fabricate a very large taddr->ipAddress.ip;
Signed-off-by: Zhouyi Zhou <zhouzhouyi@gmail.com>
---
net/netfilter/nf_conntrack_h323_main.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/net/netfilter/nf_conntrack_h323_main.c b/net/netfilter/nf_conntrack_h323_main.c
index 9511af0..3b3dd8c 100644
--- a/net/netfilter/nf_conntrack_h323_main.c
+++ b/net/netfilter/nf_conntrack_h323_main.c
@@ -110,6 +110,10 @@ int (*nat_q931_hook) (struct sk_buff *skb,
static DEFINE_SPINLOCK(nf_h323_lock);
static char *h323_buffer;
+#define CHECK_BOUND(p, n) do { \
+ if (((p - h323_buffer) + n) > 65536) \
+ return 0; \
+} while (0)
static struct nf_conntrack_helper nf_conntrack_helper_h245;
static struct nf_conntrack_helper nf_conntrack_helper_q931[];
@@ -247,6 +251,8 @@ static int get_h245_addr(struct nf_conn *ct, const unsigned char *data,
return 0;
}
+ CHECK_BOUND(p, len);
+
memcpy(addr, p, len);
memset((void *)addr + len, 0, sizeof(*addr) - len);
memcpy(port, p + len, sizeof(__be16));
@@ -669,6 +675,8 @@ int get_h225_addr(struct nf_conn *ct, unsigned char *data,
return 0;
}
+ CHECK_BOUND(p, len);
+
memcpy(addr, p, len);
memset((void *)addr + len, 0, sizeof(*addr) - len);
memcpy(port, p + len, sizeof(__be16));
--
1.7.10.4
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH 1/1] netfilter: h323: avoid potential attack
2016-01-27 14:40 [PATCH 1/1] netfilter: h323: avoid potential attack Zhouyi Zhou
@ 2016-01-27 15:06 ` kbuild test robot
2016-01-27 15:57 ` Eric Dumazet
1 sibling, 0 replies; 3+ messages in thread
From: kbuild test robot @ 2016-01-27 15:06 UTC (permalink / raw)
To: Zhouyi Zhou
Cc: kbuild-all, pablo, kaber, kadlec, davem, netfilter-devel,
coreteam, netdev, linux-kernel, Zhouyi Zhou, Zhouyi Zhou
[-- Attachment #1: Type: text/plain, Size: 6526 bytes --]
Hi Zhouyi,
[auto build test ERROR on nf-next/master]
[also build test ERROR on v4.5-rc1 next-20160127]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
url: https://github.com/0day-ci/linux/commits/Zhouyi-Zhou/netfilter-h323-avoid-potential-attack/20160127-225253
base: https://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf-next master
config: x86_64-randconfig-x016-01270835 (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All error/warnings (new ones prefixed by >>):
net/netfilter/nf_conntrack_h323_main.c: In function 'get_h245_addr':
>> net/netfilter/nf_conntrack_h323_main.c:114:11: error: invalid operands to binary - (have 'const unsigned char *' and 'char *')
if (((p - h323_buffer) + n) > 65536) \
^
>> net/netfilter/nf_conntrack_h323_main.c:254:2: note: in expansion of macro 'CHECK_BOUND'
CHECK_BOUND(p, len);
^
net/netfilter/nf_conntrack_h323_main.c: In function 'get_h225_addr':
>> net/netfilter/nf_conntrack_h323_main.c:114:11: error: invalid operands to binary - (have 'const unsigned char *' and 'char *')
if (((p - h323_buffer) + n) > 65536) \
^
net/netfilter/nf_conntrack_h323_main.c:678:2: note: in expansion of macro 'CHECK_BOUND'
CHECK_BOUND(p, len);
^
vim +114 net/netfilter/nf_conntrack_h323_main.c
108 __be16 port, struct nf_conntrack_expect *exp)
109 __read_mostly;
110
111 static DEFINE_SPINLOCK(nf_h323_lock);
112 static char *h323_buffer;
113 #define CHECK_BOUND(p, n) do { \
> 114 if (((p - h323_buffer) + n) > 65536) \
115 return 0; \
116 } while (0)
117
118 static struct nf_conntrack_helper nf_conntrack_helper_h245;
119 static struct nf_conntrack_helper nf_conntrack_helper_q931[];
120 static struct nf_conntrack_helper nf_conntrack_helper_ras[];
121
122 /****************************************************************************/
123 static int get_tpkt_data(struct sk_buff *skb, unsigned int protoff,
124 struct nf_conn *ct, enum ip_conntrack_info ctinfo,
125 unsigned char **data, int *datalen, int *dataoff)
126 {
127 struct nf_ct_h323_master *info = nfct_help_data(ct);
128 int dir = CTINFO2DIR(ctinfo);
129 const struct tcphdr *th;
130 struct tcphdr _tcph;
131 int tcpdatalen;
132 int tcpdataoff;
133 unsigned char *tpkt;
134 int tpktlen;
135 int tpktoff;
136
137 /* Get TCP header */
138 th = skb_header_pointer(skb, protoff, sizeof(_tcph), &_tcph);
139 if (th == NULL)
140 return 0;
141
142 /* Get TCP data offset */
143 tcpdataoff = protoff + th->doff * 4;
144
145 /* Get TCP data length */
146 tcpdatalen = skb->len - tcpdataoff;
147 if (tcpdatalen <= 0) /* No TCP data */
148 goto clear_out;
149
150 if (*data == NULL) { /* first TPKT */
151 /* Get first TPKT pointer */
152 tpkt = skb_header_pointer(skb, tcpdataoff, tcpdatalen,
153 h323_buffer);
154 BUG_ON(tpkt == NULL);
155
156 /* Validate TPKT identifier */
157 if (tcpdatalen < 4 || tpkt[0] != 0x03 || tpkt[1] != 0) {
158 /* Netmeeting sends TPKT header and data separately */
159 if (info->tpkt_len[dir] > 0) {
160 pr_debug("nf_ct_h323: previous packet "
161 "indicated separate TPKT data of %hu "
162 "bytes\n", info->tpkt_len[dir]);
163 if (info->tpkt_len[dir] <= tcpdatalen) {
164 /* Yes, there was a TPKT header
165 * received */
166 *data = tpkt;
167 *datalen = info->tpkt_len[dir];
168 *dataoff = 0;
169 goto out;
170 }
171
172 /* Fragmented TPKT */
173 pr_debug("nf_ct_h323: fragmented TPKT\n");
174 goto clear_out;
175 }
176
177 /* It is not even a TPKT */
178 return 0;
179 }
180 tpktoff = 0;
181 } else { /* Next TPKT */
182 tpktoff = *dataoff + *datalen;
183 tcpdatalen -= tpktoff;
184 if (tcpdatalen <= 4) /* No more TPKT */
185 goto clear_out;
186 tpkt = *data + *datalen;
187
188 /* Validate TPKT identifier */
189 if (tpkt[0] != 0x03 || tpkt[1] != 0)
190 goto clear_out;
191 }
192
193 /* Validate TPKT length */
194 tpktlen = tpkt[2] * 256 + tpkt[3];
195 if (tpktlen < 4)
196 goto clear_out;
197 if (tpktlen > tcpdatalen) {
198 if (tcpdatalen == 4) { /* Separate TPKT header */
199 /* Netmeeting sends TPKT header and data separately */
200 pr_debug("nf_ct_h323: separate TPKT header indicates "
201 "there will be TPKT data of %hu bytes\n",
202 tpktlen - 4);
203 info->tpkt_len[dir] = tpktlen - 4;
204 return 0;
205 }
206
207 pr_debug("nf_ct_h323: incomplete TPKT (fragmented?)\n");
208 goto clear_out;
209 }
210
211 /* This is the encapsulated data */
212 *data = tpkt + 4;
213 *datalen = tpktlen - 4;
214 *dataoff = tpktoff + 4;
215
216 out:
217 /* Clear TPKT length */
218 info->tpkt_len[dir] = 0;
219 return 1;
220
221 clear_out:
222 info->tpkt_len[dir] = 0;
223 return 0;
224 }
225
226 /****************************************************************************/
227 static int get_h245_addr(struct nf_conn *ct, const unsigned char *data,
228 H245_TransportAddress *taddr,
229 union nf_inet_addr *addr, __be16 *port)
230 {
231 const unsigned char *p;
232 int len;
233
234 if (taddr->choice != eH245_TransportAddress_unicastAddress)
235 return 0;
236
237 switch (taddr->unicastAddress.choice) {
238 case eUnicastAddress_iPAddress:
239 if (nf_ct_l3num(ct) != AF_INET)
240 return 0;
241 p = data + taddr->unicastAddress.iPAddress.network;
242 len = 4;
243 break;
244 case eUnicastAddress_iP6Address:
245 if (nf_ct_l3num(ct) != AF_INET6)
246 return 0;
247 p = data + taddr->unicastAddress.iP6Address.network;
248 len = 16;
249 break;
250 default:
251 return 0;
252 }
253
> 254 CHECK_BOUND(p, len);
255
256 memcpy(addr, p, len);
257 memset((void *)addr + len, 0, sizeof(*addr) - len);
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 18860 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH 1/1] netfilter: h323: avoid potential attack
2016-01-27 14:40 [PATCH 1/1] netfilter: h323: avoid potential attack Zhouyi Zhou
2016-01-27 15:06 ` kbuild test robot
@ 2016-01-27 15:57 ` Eric Dumazet
1 sibling, 0 replies; 3+ messages in thread
From: Eric Dumazet @ 2016-01-27 15:57 UTC (permalink / raw)
To: Zhouyi Zhou
Cc: pablo, kaber, kadlec, davem, netfilter-devel, coreteam, netdev,
linux-kernel, Zhouyi Zhou
On Wed, 2016-01-27 at 22:40 +0800, Zhouyi Zhou wrote:
> From: Zhouyi Zhou <yizhouzhou@ict.ac.cn>
>
> I think hackers chould build a malicious h323 packet to overflow
> the pointer p which will panic during the memcpy(addr, p, len)
>
> For example, he may fabricate a very large taddr->ipAddress.ip;
>
> Signed-off-by: Zhouyi Zhou <zhouzhouyi@gmail.com>
> ---
> net/netfilter/nf_conntrack_h323_main.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/net/netfilter/nf_conntrack_h323_main.c b/net/netfilter/nf_conntrack_h323_main.c
> index 9511af0..3b3dd8c 100644
> --- a/net/netfilter/nf_conntrack_h323_main.c
> +++ b/net/netfilter/nf_conntrack_h323_main.c
> @@ -110,6 +110,10 @@ int (*nat_q931_hook) (struct sk_buff *skb,
>
> static DEFINE_SPINLOCK(nf_h323_lock);
> static char *h323_buffer;
> +#define CHECK_BOUND(p, n) do { \
> + if (((p - h323_buffer) + n) > 65536) \
> + return 0; \
> +} while (0)
>
Do not add 'return X;' or 'goto something;' in macros please.
Even referring to 'h323_buffer' is not nice, and of course 65536 is
another 'magic' value.
Even if h323_buffer was allocated to hold 65536 bytes, the various
skb_header_pointer() calls only populated a part of it.
I understand there is a bad precedent in
net/netfilter/nf_conntrack_h323_asn1.c, but it is not a good reason.
Anyway, if the issue is real, you do not take into account the 2 extra
bytes for the port.
memcpy(port, p + len, sizeof(__be16));
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-01-27 15:57 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-01-27 14:40 [PATCH 1/1] netfilter: h323: avoid potential attack Zhouyi Zhou
2016-01-27 15:06 ` kbuild test robot
2016-01-27 15:57 ` Eric Dumazet
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).