On Mon, 2018-09-17 at 12:35 -0400, Paul Moore wrote:
On Mon, Sep 17, 2018 at 11:12 AM Stefan Nuernberger snu@amazon.com 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 snu@amazon.com Reviewed-by: David Woodhouse dwmw@amazon.co.uk Reviewed-by: Simon Veith sveith@amazon.de Cc: stable@vger.kernel.org
net/ipv4/cipso_ipv4.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c index 82178cc69c96..f291b57b8474 100644 --- a/net/ipv4/cipso_ipv4.c +++ b/net/ipv4/cipso_ipv4.c @@ -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) @@ -1522,9 +1522,11 @@ unsigned char *cipso_v4_optptr(const struct sk_buff *skb) int optlen; int taglen;
- for (optlen = iph->ihl*4 - sizeof(struct iphdr); optlen > 0; ) { + for (optlen = iph->ihl*4 - sizeof(struct iphdr); optlen > 1; ) { switch (optptr[0]) { case IPOPT_CIPSO: + if (!optptr[1] || optptr[1] > optlen) + return NULL; return optptr; case IPOPT_END: return NULL; @@ -1534,6 +1536,10 @@ unsigned char *cipso_v4_optptr(const struct sk_buff *skb) default: taglen = optptr[1]; }
+ if (!taglen || taglen > optlen) + break;
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.
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