netdev - Re: Netlink XFRM socket subsystem NULL pointer dereference

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 19 Oct 2017 14:33:20 +0300
From:   Timo Teras <timo.teras@....fi>
To:     Herbert Xu <herbert@...dor.apana.org.au>
Cc:     Eric Dumazet <edumazet@...gle.com>,
        Steffen Klassert <steffen.klassert@...unet.com>,
        netdev@...r.kernel.org
Subject: Re: Netlink XFRM socket subsystem NULL pointer dereference

On Thu, 19 Oct 2017 17:57:04 +0800
Herbert Xu <herbert@...dor.apana.org.au> wrote:

> On Thu, Oct 19, 2017 at 05:26:25PM +0800, Herbert Xu wrote:
> >
> > So it's an netlink API issue.  It is possible for cb->done to be
> > called without cb->dump ever being called.  And xfrm_user doesn't
> > deal with that.  Let me survey the others to see whether we should
> > fix this in netlink, xfrm, or both.  
> 
> OK it looks like an XFRM bug pure and simple.  Funnily enough
> the xfrm sa dump code which got changed by the same commit that
> added this bug isn't buggy.
> 
> ---8<---
> Subject: ipsec: Fix aborted xfrm policy dump crash
> 
> An independent security researcher, Mohamed Ghannam, has reported
> this vulnerability to Beyond Security's SecuriTeam Secure Disclosure
> program.
> 
> The xfrm_dump_policy_done function expects xfrm_dump_policy to
> have been called at least once or it will crash.  This can be
> triggered if a dump fails because the target socket's receive
> buffer is full.
> 
> This patch fixes it by using the cb->start mechanism to ensure that
> the initialisation is always done regardless of the buffer situation.
> 
> Fixes: 4c563f7669c1 ("[XFRM]: Speed up xfrm_policy and xfrm_state...")

This is not correct. The original commit works just fine.

The bug was introduced later. And SA side had the same issue, it got
fixed in commit:
1ba5bf993c6a3142e18e "xfrm: fix crash in XFRM_MSG_GETSA netlink handler"

The commit introducing these issues is
12a169e7d8f4b1c95252 "ipsec: Put dumpers on the dump list".

where the walker state was modified to contain list entry instead of
pointer reference.

At that time there was no .start which got added just few years ago. I
suggest to do the same fix for SA side since it had same issue fixed on
the other commit. Your approach with defining the .start is cleaner.

With updated Fixed line, and preferably applying the same to SA side,
you can add:
 Acked-by: Timo Teräs <timo.teras@....fi>

Thanks,
Timo