Re: [PATCH net] net/ipv4: defensive cipso option parsing
From: Paul Moore <paul@paul-moore.com>
Date: 2018-09-17 22:03:46
Also in:
stable
On Mon, Sep 17, 2018 at 11:12 AM Stefan Nuernberger [off-list ref] wrote:
quoted hunk ↗ jump to 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(-)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;
....
}
optlen -= taglen;
optptr += taglen;
}-- paul moore www.paul-moore.com