netdev - Re: [RFC][PATCH] xfrm: do not leak ESRCH to user space

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]

Message-Id: <1225249103.7276.56.camel@sebastian.kern.oss.ntt.co.jp>
Date:	Wed, 29 Oct 2008 11:58:23 +0900
From:	Fernando Luis Vázquez Cao 
	<fernando@....ntt.co.jp>
To:	David Miller <davem@...emloft.net>
Cc:	netdev@...r.kernel.org
Subject: Re: [RFC][PATCH] xfrm: do not leak ESRCH to user space

On Tue, 2008-10-28 at 15:58 -0700, David Miller wrote:
> From: Fernando Luis Vázquez Cao <fernando@....ntt.co.jp>
> Date: Fri, 24 Oct 2008 10:05:00 +0900
> 
> > On Thu, 2008-10-23 at 14:11 -0700, David Miller wrote:
> > > From: Fernando Luis Vázquez Cao <fernando@....ntt.co.jp>
> > > Date: Thu, 23 Oct 2008 23:27:19 +0900
> > > 
> > > > I noticed that, under certain conditions, ESRCH can be leaked from the
> > > > xfrm layer to user space through sys_connect. In particular, this seems
> > > > to happen reliably when the kernel fails to resolve a template either
> > > > because the AF_KEY receive buffer being used by racoon is full or
> > > > because the SA entry we are trying to use is in XFRM_STATE_EXPIRED
> > > > state.
> > > > 
> > > > However, since this could be a transient issue it could be argued that
> > > > EAGAIN would be more appropriate. Besides this error code is not even
> > > > documented in the man page for sys_connect (as of man-pages 3.07).
> > > > 
> > > > What is the expected behavior (I could not find anything in the RFCs)?
> > > > Should we just fix the connect(2) man page instead?
> > > 
> > > I think this case requires some care.
> > > 
> > > -EAGAIN tells the caller that it is a temporary failure and that
> > > retrying can be expected to succeed eventually (some resource is not
> > > available at the moment).  So applications loop when they see this
> > > error returned, they will try again.
> > > 
> > > But that's not what is happening when ESRCH is signalled.  We found
> > > no matching policy, and we've done nothing to make such a policy
> > > be found in the (near) future.  It is more of a hard failure, which
> > > should not necessarily be retried over and over again.
> > > 
> > > So converting this to -EAGAIN doesn't seem correct at all.
> > 
> > That would be so if -ESRCH did not happen to be a transient error.
> 
> It is not set in transient conditions as far as I can see.
> 
> Look at xfrm_state_find() which is where this error is generated and
> then propagates down to xfrm_tmpl_resolve_one().
> 
> In xfrm_state_find() if an acquire is in progress to resolve the
> entry, the code explicitly converts all errors into -EAGAIN.

Let me quote the problematic part of that function:
    ....
    } else if (x->km.state == XFRM_STATE_ACQ) {
            acquire_in_progress = 1;
    } else if (x->km.state == XFRM_STATE_ERROR ||
               x->km.state == XFRM_STATE_EXPIRED) {
            if (xfrm_selector_match(&x->sel, fl, x->sel.family) &&
                security_xfrm_state_pol_flow_match(x, pol, fl))
                    error = -ESRCH;
    }
    ....

If the kernel enters the last branch acquire_in_progress will not be set
and it could propagate down to xfrm_tmpl_resolve_one(). The reason is
that in xfrm_timer_handler() we put any entry that expired when acquire
is in progress in XFRM_STATE_EXPIRED state __before__ sleeping for two
seconds, which means the kernel will not set acquire_in_progress to 1 in
the code above.

> > Looking at the code, the window during which an entry is in
> > XFRM_STATE_EXPIRED state seems to be about 2 seconds in the worst case.
> > Connection attempts before and after that window would most likely
> > result in a successful connection or -EAGAIN, respectively. Would not it
> > make sense to return -EAGAIN also during that 2 seconds window?
> 
> Only if an acquire has been triggered and is in progress, which as
> explained above the code already seems to handle.
As explained above that does not seem to be the case.

Besides, there is also the case that km_query() in xfrm_state_find()
fails in which we would leak -ESRCH to user space too. As mentioned in a
previous email, returning -EAGAIN is arguably more appropriate if the
cause of km_query failing was that racoon's receive buffer was full
(could happen under heavy load), because as racoon reads from it the
problem will go away.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html