IPsec: stack problems

[Maxime Villard]

Hi,
I'm a little concerned about the stack usage in the IPsec code. Note that what
I'm talking about here occurs _after_ authentication.

Typically, when an IPv4-AH packet is received, the code path is:

ip_input
(*pr_input) = ipsec_common_input
ah_input
crypto_dispatch
[several crypto functions are called]
ah_input_cb
ipsec4_common_input_cb
(*pr_input) = depends on the packet

These functions are nested, so the stack consumption grows on each call.
The main issue is that the call to pr_input at the end could put us back into
ah_input, if the AH-encapsulated payload is itself an AH-encapsulated payload.
If the third payload is again AH, we're again back in ah_input, and so on.

It is not complicated to end up with:

ip_input
(*pr_input) = ipsec_common_input
ah_input
crypto_dispatch
[several crypto functions are called]
ah_input_cb
ipsec4_common_input_cb
(*pr_input) = ipsec_common_input
ah_input
crypto_dispatch
[several crypto functions are called]
ah_input_cb
ipsec4_common_input_cb
(*pr_input) = ipsec_common_input
ah_input
crypto_dispatch
[several crypto functions are called]
ah_input_cb
ipsec4_common_input_cb
(*pr_input) = ipsec_common_input
ah_input
crypto_dispatch
[several crypto functions are called]
ah_input_cb
ipsec4_common_input_cb
(*pr_input) = ah_input
ah_input
crypto_dispatch
[several crypto functions are called]
ah_input_cb
ipsec4_common_input_cb
(*pr_input) = ah_input
...

At some point a stack overflow occurs and the kernel crashes. We could fix
this by adding, in ipsec4_common_input_cb (and in the IPv6 equivalent):

if (prot == IPPROTO_AH || prot == IPPROTO_ESP ||
prot == IPPROTO_IPCOMP) {
goto bad;
}

But I think this wouldn't solve the problem in tunnel mode. You could have
prot = IPPROTO_IPV4, and encapsw4.pr_input = ipsec_common_input, so the issue
is still there.

I think we need to check the packet's history against the IPsec configuration,
probably with a tag...

Maxime

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Joerg Sonnenberger]

Post by Maxime Villard
I'm a little concerned about the stack usage in the IPsec code. Note that what
I'm talking about here occurs _after_ authentication.

I think that is a known design issue of the IPsec code. FreeBSD has been
talking about similar issues for years, too.

Post by Maxime Villard
ip_input
(*pr_input) = ipsec_common_input
ah_input
crypto_dispatch
[several crypto functions are called]
ah_input_cb
ipsec4_common_input_cb
(*pr_input) = depends on the packet

I wonder if the best appoach wouldn't be to cut the stack at this point
and defer the packet back to a netisr.

Joerg

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Maxime Villard]

Post by Joerg Sonnenberger

Post by Maxime Villard
I'm a little concerned about the stack usage in the IPsec code. Note that what
I'm talking about here occurs _after_ authentication.

I think that is a known design issue of the IPsec code. FreeBSD has been
talking about similar issues for years, too.

Post by Maxime Villard
ip_input
(*pr_input) = ipsec_common_input
ah_input
crypto_dispatch
[several crypto functions are called]
ah_input_cb
ipsec4_common_input_cb
(*pr_input) = depends on the packet

I wonder if the best appoach wouldn't be to cut the stack at this point
and defer the packet back to a netisr.

Frank Kardel suggested the same thing (in an off-list email), here's my
answer to him. Basically I'm not sure if it breaks assumptions deep in the
opencrypto code.

Maxime



-------- Message transféré --------
Sujet : Re: IPsec: stack problems
Date : Thu, 1 Mar 2018 08:02:07 +0100

Post by Joerg Sonnenberger
[...]

In fact, the crypto code was written with the assumption that when
crypto_dispatch returns, there is no further crypto processing.

If the packet is repushed, this assumption does not hold anymore, and I'm not
sure whether it wouldn't break things.

But otherwise yes, it would be nice to repush the packet.

Maxime

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Joerg Sonnenberger]

Post by Maxime Villard
In fact, the crypto code was written with the assumption that when
crypto_dispatch returns, there is no further crypto processing.
If the packet is repushed, this assumption does not hold anymore, and I'm not
sure whether it wouldn't break things.
But otherwise yes, it would be nice to repush the packet.

I don't understand that. The lower layers already expect the decrypted
data, so crypto processing has to be done at this point anyway?

Joerg

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Maxime Villard]

Post by Joerg Sonnenberger

Post by Maxime Villard
In fact, the crypto code was written with the assumption that when
crypto_dispatch returns, there is no further crypto processing.
If the packet is repushed, this assumption does not hold anymore, and I'm not
sure whether it wouldn't break things.
But otherwise yes, it would be nice to repush the packet.

I don't understand that. The lower layers already expect the decrypted
data, so crypto processing has to be done at this point anyway?

I meant to say that I'm not sure that there aren't many design changes needed
in order to repush the packet.

My main concern is that I don't understand why the authors of the IPsec code
did not repush the packet like the rest of the encapsulation protocols. There
must have been some reason - like zeroing out secret structures once the
packet has gone through the network stack entirely, I don't know.

Also, crypto_dispatch already has a batching mechanism (CRYPTO_F_BATCH) - but
it appears to be unused, so I'm not sure what we intended to do there either.

Maxime

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Joerg Sonnenberger]

Post by Maxime Villard

Post by Joerg Sonnenberger

Post by Maxime Villard
In fact, the crypto code was written with the assumption that when
crypto_dispatch returns, there is no further crypto processing.
If the packet is repushed, this assumption does not hold anymore, and I'm not
sure whether it wouldn't break things.
But otherwise yes, it would be nice to repush the packet.

I don't understand that. The lower layers already expect the decrypted
data, so crypto processing has to be done at this point anyway?

I meant to say that I'm not sure that there aren't many design changes needed
in order to repush the packet.

Note that I am not saying to push it back into the normal inet/inet6
ISR, but have one dedicated to IPsec.

Joerg

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Thor Lancelot Simon]

Post by Maxime Villard
Also, crypto_dispatch already has a batching mechanism (CRYPTO_F_BATCH) - but
it appears to be unused, so I'm not sure what we intended to do there either.

Remember there are two ways into this code -- isn't it used by the
/dev/crypto interface? Given I would have been the one who made those
changes, it's embarrassing to say I can't remember -- but it's been a
while.