* [PATCH] off-by-one in DecodeQ931
@ 2016-04-23 17:08 Toby DiPasquale
2016-04-25 15:29 ` Florian Westphal
0 siblings, 1 reply; 8+ messages in thread
From: Toby DiPasquale @ 2016-04-23 17:08 UTC (permalink / raw)
To: pablo, Patrick McHardy, kadlec, davem, netfilter-devel, coreteam,
netdev
I was reviewing the H.323 conntrack helper in the kernel when I came
across what appears to be an off-by-one error in the DecodeQ931
function. The MessageType field of the Q931 record is assigned and p
is incremented, but the corresponding decrement to sz is missing,
leading the sz variable to be one more than it should be. This patch
decrements sz so it is the proper value going into the parsing of the
information elements.
Signed-off-by: Toby DiPasquale <toby@cbcg.net>
--
diff --git a/net/netfilter/nf_conntrack_h323_asn1.c
b/net/netfilter/nf_conntrack_h323_asn1.c
index bcd5ed6..68b1557 100644
--- a/net/netfilter/nf_conntrack_h323_asn1.c
+++ b/net/netfilter/nf_conntrack_h323_asn1.c
@@ -849,6 +849,7 @@ int DecodeQ931(unsigned char *buf, size_t sz, Q931 *q931)
if (sz < 1)
return H323_ERROR_BOUND;
q931->MessageType = *p++;
+ sz--;
PRINT("MessageType = %02X\n", q931->MessageType);
if (*p & 0x80) {
p++;
--
Toby DiPasquale
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: off-by-one in DecodeQ931
2016-04-23 17:08 [PATCH] off-by-one in DecodeQ931 Toby DiPasquale
@ 2016-04-25 15:29 ` Florian Westphal
2016-05-03 5:12 ` Toby DiPasquale
0 siblings, 1 reply; 8+ messages in thread
From: Florian Westphal @ 2016-04-25 15:29 UTC (permalink / raw)
To: Toby DiPasquale
Cc: pablo, Patrick McHardy, kadlec, davem, netfilter-devel, coreteam,
netdev
Toby DiPasquale <toby@cbcg.net> wrote:
> I was reviewing the H.323 conntrack helper in the kernel when I came
> across what appears to be an off-by-one error in the DecodeQ931
> function. The MessageType field of the Q931 record is assigned and p
> is incremented, but the corresponding decrement to sz is missing,
> leading the sz variable to be one more than it should be. This patch
> decrements sz so it is the proper value going into the parsing of the
> information elements.
>
> Signed-off-by: Toby DiPasquale <toby@cbcg.net>
Looks correct, BUT
> diff --git a/net/netfilter/nf_conntrack_h323_asn1.c
> b/net/netfilter/nf_conntrack_h323_asn1.c
> index bcd5ed6..68b1557 100644
> --- a/net/netfilter/nf_conntrack_h323_asn1.c
> +++ b/net/netfilter/nf_conntrack_h323_asn1.c
> @@ -849,6 +849,7 @@ int DecodeQ931(unsigned char *buf, size_t sz, Q931 *q931)
> if (sz < 1)
> return H323_ERROR_BOUND;
sz can be 1
> q931->MessageType = *p++;
> + sz--;
sz is now 0
> PRINT("MessageType = %02X\n", q931->MessageType);
> if (*p & 0x80) {
> p++;
> sz--;
-> sz (size_t) will underflow here
I'd suggest to change the if (sz < 1) to if (sz < 2) to
resolve this, the while loop below has to be taken anyway.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: off-by-one in DecodeQ931
2016-04-25 15:29 ` Florian Westphal
@ 2016-05-03 5:12 ` Toby DiPasquale
2016-05-21 0:20 ` Toby DiPasquale
2016-06-06 13:50 ` Toby DiPasquale
0 siblings, 2 replies; 8+ messages in thread
From: Toby DiPasquale @ 2016-05-03 5:12 UTC (permalink / raw)
To: Florian Westphal
Cc: pablo, Patrick McHardy, kadlec, davem, netfilter-devel, coreteam,
netdev
On Mon, Apr 25, 2016 at 11:29 AM, Florian Westphal <fw@strlen.de> wrote:
> -> sz (size_t) will underflow here
>
> I'd suggest to change the if (sz < 1) to if (sz < 2) to
> resolve this, the while loop below has to be taken anyway.
Thanks, Florian! Updated patch below:
Signed-off-by: Toby DiPasquale <toby@cbcg.net>
diff --git a/net/netfilter/nf_conntrack_h323_asn1.c
b/net/netfilter/nf_conntrack_h323_asn1.c
index bcd5ed6..89b2e46 100644
--- a/net/netfilter/nf_conntrack_h323_asn1.c
+++ b/net/netfilter/nf_conntrack_h323_asn1.c
@@ -846,9 +846,10 @@ int DecodeQ931(unsigned char *buf, size_t sz, Q931 *q931)
sz -= len;
/* Message Type */
- if (sz < 1)
+ if (sz < 2)
return H323_ERROR_BOUND;
q931->MessageType = *p++;
+ sz--;
PRINT("MessageType = %02X\n", q931->MessageType);
if (*p & 0x80) {
p++;
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: off-by-one in DecodeQ931
2016-05-03 5:12 ` Toby DiPasquale
@ 2016-05-21 0:20 ` Toby DiPasquale
2016-06-06 13:50 ` Toby DiPasquale
1 sibling, 0 replies; 8+ messages in thread
From: Toby DiPasquale @ 2016-05-21 0:20 UTC (permalink / raw)
To: Florian Westphal
Cc: pablo, Patrick McHardy, kadlec, davem, netfilter-devel, coreteam,
netdev
I'm a bit new to this; is this patch OK?
On Tue, May 3, 2016 at 1:12 AM, Toby DiPasquale <toby@cbcg.net> wrote:
> On Mon, Apr 25, 2016 at 11:29 AM, Florian Westphal <fw@strlen.de> wrote:
>> -> sz (size_t) will underflow here
>>
>> I'd suggest to change the if (sz < 1) to if (sz < 2) to
>> resolve this, the while loop below has to be taken anyway.
>
> Thanks, Florian! Updated patch below:
>
> Signed-off-by: Toby DiPasquale <toby@cbcg.net>
>
> diff --git a/net/netfilter/nf_conntrack_h323_asn1.c
> b/net/netfilter/nf_conntrack_h323_asn1.c
> index bcd5ed6..89b2e46 100644
> --- a/net/netfilter/nf_conntrack_h323_asn1.c
> +++ b/net/netfilter/nf_conntrack_h323_asn1.c
> @@ -846,9 +846,10 @@ int DecodeQ931(unsigned char *buf, size_t sz, Q931 *q931)
> sz -= len;
>
> /* Message Type */
> - if (sz < 1)
> + if (sz < 2)
> return H323_ERROR_BOUND;
> q931->MessageType = *p++;
> + sz--;
> PRINT("MessageType = %02X\n", q931->MessageType);
> if (*p & 0x80) {
> p++;
--
Toby DiPasquale
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: off-by-one in DecodeQ931
2016-05-03 5:12 ` Toby DiPasquale
2016-05-21 0:20 ` Toby DiPasquale
@ 2016-06-06 13:50 ` Toby DiPasquale
2016-06-06 14:35 ` Florian Westphal
1 sibling, 1 reply; 8+ messages in thread
From: Toby DiPasquale @ 2016-06-06 13:50 UTC (permalink / raw)
To: Florian Westphal
Cc: pablo, Patrick McHardy, kadlec, davem, netfilter-devel, coreteam,
netdev
Is this latest patch OK?
On Tue, May 3, 2016 at 1:12 AM, Toby DiPasquale <toby@cbcg.net> wrote:
> On Mon, Apr 25, 2016 at 11:29 AM, Florian Westphal <fw@strlen.de> wrote:
>> -> sz (size_t) will underflow here
>>
>> I'd suggest to change the if (sz < 1) to if (sz < 2) to
>> resolve this, the while loop below has to be taken anyway.
>
> Thanks, Florian! Updated patch below:
>
> Signed-off-by: Toby DiPasquale <toby@cbcg.net>
>
> diff --git a/net/netfilter/nf_conntrack_h323_asn1.c
> b/net/netfilter/nf_conntrack_h323_asn1.c
> index bcd5ed6..89b2e46 100644
> --- a/net/netfilter/nf_conntrack_h323_asn1.c
> +++ b/net/netfilter/nf_conntrack_h323_asn1.c
> @@ -846,9 +846,10 @@ int DecodeQ931(unsigned char *buf, size_t sz, Q931 *q931)
> sz -= len;
>
> /* Message Type */
> - if (sz < 1)
> + if (sz < 2)
> return H323_ERROR_BOUND;
> q931->MessageType = *p++;
> + sz--;
> PRINT("MessageType = %02X\n", q931->MessageType);
> if (*p & 0x80) {
> p++;
--
Toby DiPasquale
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: off-by-one in DecodeQ931
2016-06-06 13:50 ` Toby DiPasquale
@ 2016-06-06 14:35 ` Florian Westphal
2016-06-06 14:55 ` Pablo Neira Ayuso
0 siblings, 1 reply; 8+ messages in thread
From: Florian Westphal @ 2016-06-06 14:35 UTC (permalink / raw)
To: Toby DiPasquale
Cc: Florian Westphal, pablo, Patrick McHardy, kadlec, davem,
netfilter-devel, coreteam, netdev
Toby DiPasquale <toby@cbcg.net> wrote:
> Is this latest patch OK?
Yes, I don't know why it wasn't applied yet.
Pablo?
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: off-by-one in DecodeQ931
2016-06-06 14:35 ` Florian Westphal
@ 2016-06-06 14:55 ` Pablo Neira Ayuso
2016-06-12 23:29 ` Toby DiPasquale
0 siblings, 1 reply; 8+ messages in thread
From: Pablo Neira Ayuso @ 2016-06-06 14:55 UTC (permalink / raw)
To: Florian Westphal
Cc: Toby DiPasquale, Patrick McHardy, kadlec, davem, netfilter-devel,
coreteam, netdev
On Mon, Jun 06, 2016 at 04:35:55PM +0200, Florian Westphal wrote:
> Toby DiPasquale <toby@cbcg.net> wrote:
> > Is this latest patch OK?
>
> Yes, I don't know why it wasn't applied yet.
>
> Pablo?
This doesn't apply.
$ git am /tmp/off-by-one-in-DecodeQ931.patch -s
Applying: off-by-one in DecodeQ931
error: patch failed: net/netfilter/nf_conntrack_h323_asn1.c:846
error: net/netfilter/nf_conntrack_h323_asn1.c: patch does not apply
Patch failed at 0001 off-by-one in DecodeQ931
The copy of the patch that failed is found in:
patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Could you resubmit using git-format-patch? Thanks.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: off-by-one in DecodeQ931
2016-06-06 14:55 ` Pablo Neira Ayuso
@ 2016-06-12 23:29 ` Toby DiPasquale
0 siblings, 0 replies; 8+ messages in thread
From: Toby DiPasquale @ 2016-06-12 23:29 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: Florian Westphal, Patrick McHardy, kadlec, davem, netfilter-devel,
coreteam, netdev
[-- Attachment #1: Type: text/plain, Size: 978 bytes --]
Attached is the patch generated with git format-patch.
On Mon, Jun 6, 2016 at 10:55 AM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Mon, Jun 06, 2016 at 04:35:55PM +0200, Florian Westphal wrote:
>> Toby DiPasquale <toby@cbcg.net> wrote:
>> > Is this latest patch OK?
>>
>> Yes, I don't know why it wasn't applied yet.
>>
>> Pablo?
>
> This doesn't apply.
>
> $ git am /tmp/off-by-one-in-DecodeQ931.patch -s
> Applying: off-by-one in DecodeQ931
> error: patch failed: net/netfilter/nf_conntrack_h323_asn1.c:846
> error: net/netfilter/nf_conntrack_h323_asn1.c: patch does not apply
> Patch failed at 0001 off-by-one in DecodeQ931
> The copy of the patch that failed is found in:
> patch
> When you have resolved this problem, run "git am --continue".
> If you prefer to skip this patch, run "git am --skip" instead.
> To restore the original branch and stop patching, run "git am --abort".
>
> Could you resubmit using git-format-patch? Thanks.
--
Toby DiPasquale
[-- Attachment #2: off-by-one.patch --]
[-- Type: application/octet-stream, Size: 805 bytes --]
From 2cada5fdce0ecd5b445a360917431cf0dfffa4bb Mon Sep 17 00:00:00 2001
From: Toby DiPasquale <toby@cbcg.net>
Date: Sun, 12 Jun 2016 19:27:53 -0400
Subject: [PATCH] fix off-by-one in DecodeQ931
---
net/netfilter/nf_conntrack_h323_asn1.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/netfilter/nf_conntrack_h323_asn1.c b/net/netfilter/nf_conntrack_h323_asn1.c
index bcd5ed6..89b2e46 100644
--- a/net/netfilter/nf_conntrack_h323_asn1.c
+++ b/net/netfilter/nf_conntrack_h323_asn1.c
@@ -846,9 +846,10 @@ int DecodeQ931(unsigned char *buf, size_t sz, Q931 *q931)
sz -= len;
/* Message Type */
- if (sz < 1)
+ if (sz < 2)
return H323_ERROR_BOUND;
q931->MessageType = *p++;
+ sz--;
PRINT("MessageType = %02X\n", q931->MessageType);
if (*p & 0x80) {
p++;
--
2.7.4
^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-06-12 23:29 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-04-23 17:08 [PATCH] off-by-one in DecodeQ931 Toby DiPasquale
2016-04-25 15:29 ` Florian Westphal
2016-05-03 5:12 ` Toby DiPasquale
2016-05-21 0:20 ` Toby DiPasquale
2016-06-06 13:50 ` Toby DiPasquale
2016-06-06 14:35 ` Florian Westphal
2016-06-06 14:55 ` Pablo Neira Ayuso
2016-06-12 23:29 ` Toby DiPasquale
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).