From: Stefan Nuernberger <hidden> Date: 2018-09-17 15:11:49
commit 40413955ee26 ("Cipso: cipso_v4_optptr enter infinite loop") fixed
a possible infinite loop in the IP option parsing of CIPSO. The fix
assumes that ip_options_compile filtered out all zero length options and
that no other one-byte options beside IPOPT_END and IPOPT_NOOP exist.
While this assumption currently holds true, add explicit checks for zero
length and invalid length options to be safe for the future. Even though
ip_options_compile should have validated the options, the introduction of
new one-byte options can still confuse this code without the additional
checks.
Signed-off-by: Stefan Nuernberger <redacted>
Reviewed-by: David Woodhouse <redacted>
Reviewed-by: Simon Veith <redacted>
Cc: stable@vger.kernel.org
---
net/ipv4/cipso_ipv4.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
From: Paul Moore <paul@paul-moore.com> Date: 2018-09-17 22:03:46
On Mon, Sep 17, 2018 at 11:12 AM Stefan Nuernberger [off-list ref] wrote:
quoted hunk
commit 40413955ee26 ("Cipso: cipso_v4_optptr enter infinite loop") fixed
a possible infinite loop in the IP option parsing of CIPSO. The fix
assumes that ip_options_compile filtered out all zero length options and
that no other one-byte options beside IPOPT_END and IPOPT_NOOP exist.
While this assumption currently holds true, add explicit checks for zero
length and invalid length options to be safe for the future. Even though
ip_options_compile should have validated the options, the introduction of
new one-byte options can still confuse this code without the additional
checks.
Signed-off-by: Stefan Nuernberger <redacted>
Reviewed-by: David Woodhouse <redacted>
Reviewed-by: Simon Veith <redacted>
Cc: stable@vger.kernel.org
---
net/ipv4/cipso_ipv4.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
I tend to think that you reach a point where you simply need to trust
that the stack is doing the right thing and that by the time you hit a
certain point you can safely assume that the packet is well formed,
but I'm not going to fight about that here.
Regardless of the above, I don't like how you're doing the option
length check twice in this code, that looks ugly to me, I think we can
do better. How about something like this:
for (...) {
switch(optptr[0]) {
case IPOPT_END:
return NULL;
case IPOPT_NOOP:
taglen = 1;
default:
taglen = optptr[1];
}
if (taglen == 0 || taglen > optlen)
return NULL;
if (optptr[0] == IPOPT_CIPSO)
return optptr;
....
}
From: Stefan Nuernberger <hidden> Date: 2018-09-17 23:17:42
commit 40413955ee26 ("Cipso: cipso_v4_optptr enter infinite loop") fixed
a possible infinite loop in the IP option parsing of CIPSO. The fix
assumes that ip_options_compile filtered out all zero length options and
that no other one-byte options beside IPOPT_END and IPOPT_NOOP exist.
While this assumption currently holds true, add explicit checks for zero
length and invalid length options to be safe for the future. Even though
ip_options_compile should have validated the options, the introduction of
new one-byte options can still confuse this code without the additional
checks.
Signed-off-by: Stefan Nuernberger <redacted>
Cc: David Woodhouse <redacted>
Cc: Simon Veith <redacted>
Cc: stable@vger.kernel.org
---
net/ipv4/cipso_ipv4.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
From: Nuernberger, Stefan <hidden> Date: 2018-09-17 23:31:01
On Mon, 2018-09-17 at 12:35 -0400, Paul Moore wrote:
On Mon, Sep 17, 2018 at 11:12 AM Stefan Nuernberger [off-list ref]
wrote:
quoted
commit 40413955ee26 ("Cipso: cipso_v4_optptr enter infinite loop")
fixed
a possible infinite loop in the IP option parsing of CIPSO. The fix
assumes that ip_options_compile filtered out all zero length
options and
that no other one-byte options beside IPOPT_END and IPOPT_NOOP
exist.
While this assumption currently holds true, add explicit checks for
zero
length and invalid length options to be safe for the future. Even
though
ip_options_compile should have validated the options, the
introduction of
new one-byte options can still confuse this code without the
additional
checks.
Signed-off-by: Stefan Nuernberger <redacted>
Reviewed-by: David Woodhouse <redacted>
Reviewed-by: Simon Veith <redacted>
Cc: stable@vger.kernel.org
---
net/ipv4/cipso_ipv4.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
@@ -1512,7 +1512,7 @@ static int cipso_v4_parsetag_loc(const struct
cipso_v4_doi *doi_def,
*
* Description:
* Parse the packet's IP header looking for a CIPSO
option. Returns a pointer
- * to the start of the CIPSO option on success, NULL if one if not
found.
+ * to the start of the CIPSO option on success, NULL if one is not
found.
*
*/
unsigned char *cipso_v4_optptr(const struct sk_buff *skb)
I tend to think that you reach a point where you simply need to trust
that the stack is doing the right thing and that by the time you hit
a
certain point you can safely assume that the packet is well formed,
but I'm not going to fight about that here.
Regardless of the above, I don't like how you're doing the option
length check twice in this code, that looks ugly to me, I think we
can
do better. How about something like this:
for (...) {
switch(optptr[0]) {
case IPOPT_END:
return NULL;
case IPOPT_NOOP:
taglen = 1;
default:
taglen = optptr[1];
}
if (taglen == 0 || taglen > optlen)
return NULL;
if (optptr[0] == IPOPT_CIPSO)
return optptr;
....
}
You're right, that looks much better. I sent around a new patch.
quoted
optlen -= taglen;
optptr += taglen;
}
Amazon Development Center Germany GmbH
Berlin - Dresden - Aachen
main office: Krausenstr. 38, 10117 Berlin
Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
Ust-ID: DE289237879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B
From: Paul Moore <paul@paul-moore.com> Date: 2018-09-18 02:00:21
On Mon, Sep 17, 2018 at 1:49 PM Stefan Nuernberger [off-list ref] wrote:
commit 40413955ee26 ("Cipso: cipso_v4_optptr enter infinite loop") fixed
a possible infinite loop in the IP option parsing of CIPSO. The fix
assumes that ip_options_compile filtered out all zero length options and
that no other one-byte options beside IPOPT_END and IPOPT_NOOP exist.
While this assumption currently holds true, add explicit checks for zero
length and invalid length options to be safe for the future. Even though
ip_options_compile should have validated the options, the introduction of
new one-byte options can still confuse this code without the additional
checks.
Signed-off-by: Stefan Nuernberger <redacted>
Cc: David Woodhouse <redacted>
Cc: Simon Veith <redacted>
Cc: stable@vger.kernel.org
---
net/ipv4/cipso_ipv4.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
See my previous comments about the necessity of this patch, but beyond
that it looks fine to me.
Acked-by: Paul Moore <paul@paul-moore.com>
Not worth re-spinning this patch, but looking at this a bit closer, we
could probably optimize the "optlen > 1" tweak a bit further by using
CIPSO_V4_HDR_LEN instead of "1" since we only care about CIPSO headers
here.
Although given the nature of IPv4 options, I'm not sure this would
ever really have an impact, let alone a noticeable impact.
quoted hunk
switch (optptr[0]) {
- case IPOPT_CIPSO:
- return optptr;
case IPOPT_END:
return NULL;
case IPOPT_NOOP:
commit 40413955ee26 ("Cipso: cipso_v4_optptr enter infinite loop") fixed
a possible infinite loop in the IP option parsing of CIPSO. The fix
assumes that ip_options_compile filtered out all zero length options and
that no other one-byte options beside IPOPT_END and IPOPT_NOOP exist.
While this assumption currently holds true, add explicit checks for zero
length and invalid length options to be safe for the future. Even though
ip_options_compile should have validated the options, the introduction of
new one-byte options can still confuse this code without the additional
checks.
Signed-off-by: Stefan Nuernberger <redacted>
Applied to net-next.
This is not 'net' nor -stable material. I'm hesitent about this
change as-is, and ip_options_compile() is not changing semantics in
-stable in the way that you say can cause problems.