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; .... }
optlen -= taglen; optptr += taglen; }