diff mbox

af_key: parse and send SADB_X_EXT_NAT_T_OA extension

Message ID 1232523247-6893-1-git-send-email-timo.teras@iki.fi
State Rejected, archived
Delegated to: David Miller
Headers show

Commit Message

Timo Teras Jan. 21, 2009, 7:34 a.m. UTC
Parse and send SADB_X_EXT_NAT_T_OA along with other NAT-T extensions.

Signed-off-by: Timo Teras <timo.teras@iki.fi>
---
 net/key/af_key.c |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

Comments

Herbert Xu Jan. 21, 2009, 10:33 p.m. UTC | #1
Timo Teras <timo.teras@iki.fi> wrote:
> Parse and send SADB_X_EXT_NAT_T_OA along with other NAT-T extensions.
> 
> Signed-off-by: Timo Teras <timo.teras@iki.fi>

Whatever application that you have in mind that's going to use
this should switch over to netlink.

Thanks,
David Miller Jan. 21, 2009, 10:40 p.m. UTC | #2
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Thu, 22 Jan 2009 09:33:29 +1100

> Timo Teras <timo.teras@iki.fi> wrote:
> > Parse and send SADB_X_EXT_NAT_T_OA along with other NAT-T extensions.
> > 
> > Signed-off-by: Timo Teras <timo.teras@iki.fi>
> 
> Whatever application that you have in mind that's going to use
> this should switch over to netlink.

Agreed, I won't be applying this patch.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Timo Teras Jan. 22, 2009, 5:56 a.m. UTC | #3
David Miller wrote:
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Date: Thu, 22 Jan 2009 09:33:29 +1100
> 
>> Timo Teras <timo.teras@iki.fi> wrote:
>>> Parse and send SADB_X_EXT_NAT_T_OA along with other NAT-T extensions.
>>>
>>> Signed-off-by: Timo Teras <timo.teras@iki.fi>
>> Whatever application that you have in mind that's going to use
>> this should switch over to netlink.
> 
> Agreed, I won't be applying this patch.

It's for ipsec-tools. Currently the NAT OA in xfrm structs isn't
really used, but I need it later when implementing the NHRP/GRE/NAT
traversal I've sent couple mails about [1,2].

I do understand that netlink/xfrm is better, and I have it on
my long todo list. But rewriting 30-40 thousand lines of code
isn't going to happen that soon.

Is there any particular reason why setting NAT-OA info should/
must be done using netlink? Or is this just a way to try to
put more pressure for the change to happen?

I find it very confusing that NAT_T_[SD]PORT are supported, but
NAT_T_OA is not which is part of the same set of extensions
(required by some other kernels to make nat-t work in first
place; to fix up the checksums in transport mode SAs).

Even though if kernel currently does not use that information it
might confuse applications to get mismatching information back.

Currently if pfkey adds an NAT_T SA, and someone queries it
via xfrm you get back a random NAT OA. It's because
pfkey_msg2xfrm_state() kmallocs() the encap struct but does not
zero out or copy in the encap_oa field. IMHO, it's better to
just copy the information. But checking that again now, it looks
like we would need to memset() the encap_oa also when NAT_T_OA
is not present.

Also I find it a bit confusing which things are to be allowed
in pfkey and which not. We've had bigger fixes/changes to pfkey
in past like MIGRATE rewrite, etc.

- Timo

[1] http://marc.info/?l=linux-netdev&m=122232910618099&w=4
[2] http://marc.info/?l=linux-netdev&m=123252926623554&w=2

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller Jan. 22, 2009, 6:03 a.m. UTC | #4
From: Timo Teräs <timo.teras@iki.fi>
Date: Thu, 22 Jan 2009 07:56:57 +0200

> Is there any particular reason why setting NAT-OA info should/
> must be done using netlink? Or is this just a way to try to
> put more pressure for the change to happen?

Because it isn't deprecated if we keep adding features to it.

> Also I find it a bit confusing which things are to be allowed
> in pfkey and which not. We've had bigger fixes/changes to pfkey
> in past like MIGRATE rewrite, etc.

That was a mistake on my part, I shouldn't have allowed the
pfkey side of the changes there.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Timo Teras Jan. 22, 2009, 6:14 a.m. UTC | #5
David Miller wrote:
> From: Timo Teräs <timo.teras@iki.fi>
> Date: Thu, 22 Jan 2009 07:56:57 +0200
> 
>> Is there any particular reason why setting NAT-OA info should/
>> must be done using netlink? Or is this just a way to try to
>> put more pressure for the change to happen?
> 
> Because it isn't deprecated if we keep adding features to it.

I would not consider this a new feature. It just makes pfkey
act consistently. If you don't want it supported, it'd make
more sense to not #define SADB_X_EXT_NAT_T_OA and drop all of
the verification code already present than to silently
ignore it. Make kernel return an error if some tried using it.

Now you give impression that it's supported but you just drop
it silently.

>> Also I find it a bit confusing which things are to be allowed
>> in pfkey and which not. We've had bigger fixes/changes to pfkey
>> in past like MIGRATE rewrite, etc.
> 
> That was a mistake on my part, I shouldn't have allowed the
> pfkey side of the changes there.

I see.

- Timo
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller Jan. 22, 2009, 6:21 a.m. UTC | #6
From: Timo Teräs <timo.teras@iki.fi>
Date: Thu, 22 Jan 2009 08:14:13 +0200

> David Miller wrote:
> > From: Timo Teräs <timo.teras@iki.fi>
> > Date: Thu, 22 Jan 2009 07:56:57 +0200
> > 
> >> Is there any particular reason why setting NAT-OA info should/
> >> must be done using netlink? Or is this just a way to try to
> >> put more pressure for the change to happen?
> > 
> > Because it isn't deprecated if we keep adding features to it.
> 
> I would not consider this a new feature. It just makes pfkey
> act consistently. If you don't want it supported, it'd make
> more sense to not #define SADB_X_EXT_NAT_T_OA and drop all of
> the verification code already present than to silently
> ignore it. Make kernel return an error if some tried using it.

Fair enough, sounds like a reasonable argument.

Herbert what do you think?  The proposal is that we just reflect the
value we get, rather than acting upon it.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Herbert Xu Jan. 22, 2009, 6:22 a.m. UTC | #7
On Thu, Jan 22, 2009 at 08:14:13AM +0200, Timo Teräs wrote:
>
> I would not consider this a new feature. It just makes pfkey
> act consistently. If you don't want it supported, it'd make
> more sense to not #define SADB_X_EXT_NAT_T_OA and drop all of
> the verification code already present than to silently
> ignore it. Make kernel return an error if some tried using it.
> 
> Now you give impression that it's supported but you just drop
> it silently.

Timo I think we need to draw a line somewhere, otherwise we're
just providing life support to an interface that should die.

IMO if there is a bug in pfkey that breaks a major existing
app (i.e., ipsec-tools/racoon), then we should fix that.

But if it's something that the existing apps don't care about
then no we need to just stop extending the interface.

Cheers,
Herbert Xu Jan. 22, 2009, 6:32 a.m. UTC | #8
On Wed, Jan 21, 2009 at 10:21:12PM -0800, David Miller wrote:
>
> > I would not consider this a new feature. It just makes pfkey
> > act consistently. If you don't want it supported, it'd make
> > more sense to not #define SADB_X_EXT_NAT_T_OA and drop all of
> > the verification code already present than to silently
> > ignore it. Make kernel return an error if some tried using it.
> 
> Fair enough, sounds like a reasonable argument.
> 
> Herbert what do you think?  The proposal is that we just reflect the
> value we get, rather than acting upon it.

The only references to SADB_X_EXT_NAT_T_OA in our kernel currently
are for the user => kernel direction.  This patch does kernel =>
user so I don't see a connection.

In any case I just grabbed the latest ipsec-tools source and it
doesn't use this stuff at all so IMO this is a new feature.

Cheers,
Timo Teras Jan. 22, 2009, 6:39 a.m. UTC | #9
Herbert Xu wrote:
> On Wed, Jan 21, 2009 at 10:21:12PM -0800, David Miller wrote:
>>> I would not consider this a new feature. It just makes pfkey
>>> act consistently. If you don't want it supported, it'd make
>>> more sense to not #define SADB_X_EXT_NAT_T_OA and drop all of
>>> the verification code already present than to silently
>>> ignore it. Make kernel return an error if some tried using it.
>> Fair enough, sounds like a reasonable argument.
>>
>> Herbert what do you think?  The proposal is that we just reflect the
>> value we get, rather than acting upon it.
> 
> The only references to SADB_X_EXT_NAT_T_OA in our kernel currently
> are for the user => kernel direction.  This patch does kernel =>
> user so I don't see a connection.
>
> In any case I just grabbed the latest ipsec-tools source and it
> doesn't use this stuff at all so IMO this is a new feature.

There hasn't been new release for ipsec-tools for a while.
It's been in ipsec-tools CVS since 2007-12-12. And I know many
who are using the CVS code in production.

Again, my argument is that the code does not modify any headers,
add/modify structures, extensions or messages. So in kernel
point of view it's not a new feature. From userland point of
view it makes kernel act consistently as now the kernel claims
to support something (by having the defines, checking it,
validating it) but just ignores it silently.

Now the split is artificial: the pfkey code knows about nat-oa
but silently ignores it instead of passing it to xfrm. It made
more sense if it was properly or not knowing at all about it.

- Timo
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Herbert Xu Jan. 22, 2009, 6:47 a.m. UTC | #10
On Thu, Jan 22, 2009 at 08:39:35AM +0200, Timo Teräs wrote:
> 
> There hasn't been new release for ipsec-tools for a while.
> It's been in ipsec-tools CVS since 2007-12-12. And I know many
> who are using the CVS code in production.

If they've had it since 2007 and only just realised that it
doesn't work then it sounds like it doesn't really matter anyway.

Cheers,
Timo Teras Jan. 22, 2009, 6:54 a.m. UTC | #11
Herbert Xu wrote:
> On Thu, Jan 22, 2009 at 08:39:35AM +0200, Timo Teräs wrote:
>> There hasn't been new release for ipsec-tools for a while.
>> It's been in ipsec-tools CVS since 2007-12-12. And I know many
>> who are using the CVS code in production.
> 
> If they've had it since 2007 and only just realised that it
> doesn't work then it sounds like it doesn't really matter anyway.

That ipsec-tools feature works on *BSD. Works on Linux too
as kernel does not (yet) use that for anything except reporting
it back. Other OSes might use it already to e.g. fix-up the
packet checksums in transport mode SAs; I believe Linux just
recalculates the checksum.

The future patch I have in my mind I've been talking about,
does make use of NAT-OA. So that's why I noticed it only
just now. Btw, could someone comment on the idea of passing
NAT-OA to neighbour cache and make xfrm use it when choosing
which xfrm state to use?

- Timo

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Herbert Xu Jan. 22, 2009, 7:39 a.m. UTC | #12
On Thu, Jan 22, 2009 at 08:54:02AM +0200, Timo Teräs wrote:
>
> That ipsec-tools feature works on *BSD. Works on Linux too
> as kernel does not (yet) use that for anything except reporting
> it back. Other OSes might use it already to e.g. fix-up the
> packet checksums in transport mode SAs; I believe Linux just
> recalculates the checksum.

Right, the reason nobody has noticed is because transport mode
NAT-T is inherently insecure (and difficult to deploy if you have
multiple peers behind one gateway) so nobody uses it in the field.

So I think there is even less reason to do this for af_key.

If you don't have the time to convert racoon, couldn't you look
at one of the *swans and use that as the basis of your work?

> The future patch I have in my mind I've been talking about,
> does make use of NAT-OA. So that's why I noticed it only
> just now. Btw, could someone comment on the idea of passing
> NAT-OA to neighbour cache and make xfrm use it when choosing
> which xfrm state to use?

I'm concerned about using NAT-OA for anything policy related
because it is impossible to authenticate or verify.

Cheers,
Timo Teras Jan. 22, 2009, 8:31 a.m. UTC | #13
Herbert Xu wrote:
> On Thu, Jan 22, 2009 at 08:54:02AM +0200, Timo Teräs wrote:
>> That ipsec-tools feature works on *BSD. Works on Linux too
>> as kernel does not (yet) use that for anything except reporting
>> it back. Other OSes might use it already to e.g. fix-up the
>> packet checksums in transport mode SAs; I believe Linux just
>> recalculates the checksum.
> 
> Right, the reason nobody has noticed is because transport mode
> NAT-T is inherently insecure (and difficult to deploy if you have
> multiple peers behind one gateway) so nobody uses it in the field.

Yes, if you have TCP or UDP as upper protocol. DMVPN has GRE as
upper protocol, so most of the problems are handled at GRE
layer with NHRP, dynamic routing protocols, etc.

> So I think there is even less reason to do this for af_key.
>
> If you don't have the time to convert racoon, couldn't you look
> at one of the *swans and use that as the basis of your work?

I did look into them in the beginning. They have a bit different
way of looking into ipsec connections than dmvpn. So I thought
ipsec-tools would be easiest to modify. I did not tie opennhrp
to ipsec-tools specifically (ipsec glue is a script) so it's
changeable.

Basically NHRP works as private IP to public IP mapping
protocol. And wants to be in charge of establishing the IPsec
SAs dynamically. Basically the option of having "unnamed"
connections distinguished by IP:s was the criteria why I went
with ipsec-tools.

The (quickish) look on *swans would imply that for each
dynamic connection I would need to hand create a new connection
entity. Could be scriptable, but then I would somehow
need to make up connection names based on the IPs and always
inject full config by the admin tool.

>> The future patch I have in my mind I've been talking about,
>> does make use of NAT-OA. So that's why I noticed it only
>> just now. Btw, could someone comment on the idea of passing
>> NAT-OA to neighbour cache and make xfrm use it when choosing
>> which xfrm state to use?
> 
> I'm concerned about using NAT-OA for anything policy related
> because it is impossible to authenticate or verify.

I'm not sure if you are familiar how DMVPN works (= IPsec
transport mode + NBMA GRE + NHRP), but I can explain it more
detailed if you want to hear more about it.

In any case the NAT-OA would be used as hint to choose which
cached xfrm state the global xfrm policy "encrypt all GRE
traffic" would return when kernel sends packets out from
the GRE tunnel interface.

Basically, OpenNHRP would:
1. Make ipsec-tools (or some other keying manager) create
   new SAs for the dynamic IPsec connection. The KM would
   inject the NAT-OA for the negotiated xfrm state.

2. OpenNHRP after ensuring the IPsec SA is there, would
   inject GRE interface neighbor entry saying:
      "packets to this IP in GRE subnet, go to this public
       IP. do use this NAT-OA as hint when choosing xfrm state."
   This would be sent via Netlink.

This all works already, if all DMVPN nodes have different
public IP (xfrm chooses correct SA to use based on public IP).

But if there's multiple nodes behind same public IP the xfrm
has two (or more) choices for which xfrm state to use. The
NAT-OA can be used as distinguishing factor. So when ip_gre
sends out packet, the xfrm can choose the correct xfrm state.

Would that sound acceptable?

- Timo
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Herbert Xu Jan. 22, 2009, 8:50 a.m. UTC | #14
On Thu, Jan 22, 2009 at 10:31:32AM +0200, Timo Teräs wrote:
> 
> The (quickish) look on *swans would imply that for each
> dynamic connection I would need to hand create a new connection
> entity. Could be scriptable, but then I would somehow
> need to make up connection names based on the IPs and always
> inject full config by the admin tool.

They have templates for inbound connections on servers and also
outbound connections which were used for Opportunistic Encryption.
So you can adapt these to generate connections on demand.

Also creating/destroying connections at run-time is really easy
without generating a full config at all by using ipsec whack.
 
> But if there's multiple nodes behind same public IP the xfrm
> has two (or more) choices for which xfrm state to use. The
> NAT-OA can be used as distinguishing factor. So when ip_gre
> sends out packet, the xfrm can choose the correct xfrm state.

That's precisely what you don't want to do unless you trust all
your peers equally.  The reason is that NAT-OA is not authenticated,
i.e., I can choose any address to send over as NAT-OA and there
is nothing you can do to stop me.

It was only ever intended to be used for fixing up the checksum
so you can't safely use it for making policy/routing decisions.

Cheers,
Timo Teras Jan. 22, 2009, 9:24 a.m. UTC | #15
Herbert Xu wrote:
> On Thu, Jan 22, 2009 at 10:31:32AM +0200, Timo Teräs wrote:
>> The (quickish) look on *swans would imply that for each
>> dynamic connection I would need to hand create a new connection
>> entity. Could be scriptable, but then I would somehow
>> need to make up connection names based on the IPs and always
>> inject full config by the admin tool.
> 
> They have templates for inbound connections on servers and also
> outbound connections which were used for Opportunistic Encryption.
> So you can adapt these to generate connections on demand.
> 
> Also creating/destroying connections at run-time is really easy
> without generating a full config at all by using ipsec whack.

I think I looked into openswan int he beginning in detail
and it had various problems. I think there wasn't netlink
stuff there either when I looked into it.

Looking at strongswan now, it does look a lot more usable.

But I don't like how IKEv2 is complitely separate from
IKEv1. Different daemons, different whack interface, etc.

There was some other things too why I decided to go for
ipsec-tools in the beginning. But it doesn't really matter
as long as I get IPsec.

So for the time being, I can have my patched kernel, fix
some of the swans and switch to it, or even write my own
IPsec keying manager if it comes to that. IPsec KM is not
that relevant for me, as long as it works.

>> But if there's multiple nodes behind same public IP the xfrm
>> has two (or more) choices for which xfrm state to use. The
>> NAT-OA can be used as distinguishing factor. So when ip_gre
>> sends out packet, the xfrm can choose the correct xfrm state.
> 
> That's precisely what you don't want to do unless you trust all
> your peers equally.  The reason is that NAT-OA is not authenticated,
> i.e., I can choose any address to send over as NAT-OA and there
> is nothing you can do to stop me.
> 
> It was only ever intended to be used for fixing up the checksum
> so you can't safely use it for making policy/routing decisions.

In DMVPN/GRE case, the NAT-OA from IPsec would not be used
unless the NAT-OA is set on neighbour cache. This would not
happen unless NHRP can authenticate it. In DMVPN case you need
a valid certificate to give the ranom NAT-OA in any case. So
if you lie about your NAT-OA I can just revoke you.

Or do you have, other recommendations how to distinguish peers
behind same public IP than NAT-OA? Maybe we add the certificate
subject to xfrm state and neighbour cache. And use that?
That would certainly be authenticated. Would that be a better
approach? Or in general, allow a random token to be associated
with an xfrm state, so that we can connect neighbor cache in
gre interface to that specific state.

- Timo
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Herbert Xu Jan. 22, 2009, 9:41 a.m. UTC | #16
On Thu, Jan 22, 2009 at 11:24:41AM +0200, Timo Teräs wrote:
>
> In DMVPN/GRE case, the NAT-OA from IPsec would not be used
> unless the NAT-OA is set on neighbour cache. This would not
> happen unless NHRP can authenticate it. In DMVPN case you need
> a valid certificate to give the ranom NAT-OA in any case. So
> if you lie about your NAT-OA I can just revoke you.

I don't see how NHRP authentication would help if the attacker
takes over someone else's address and causes packets for the
third party to go to it.

As to revoking the attacker's access, that's like saying "I'll
run an open telnet port but if you try to sniff my psasword I'll
revoke your access" :)

> Or do you have, other recommendations how to distinguish peers
> behind same public IP than NAT-OA? Maybe we add the certificate
> subject to xfrm state and neighbour cache. And use that?

Yes I think that is a much better solution for this.

Cheers,
Timo Teras Jan. 22, 2009, 10 a.m. UTC | #17
[Offtopic: looking latest openswan code it does look a lot
better too. It looked quite different when checked it out
some years ago.]

Herbert Xu wrote:
> On Thu, Jan 22, 2009 at 11:24:41AM +0200, Timo Teräs wrote:
>> In DMVPN/GRE case, the NAT-OA from IPsec would not be used
>> unless the NAT-OA is set on neighbour cache. This would not
>> happen unless NHRP can authenticate it. In DMVPN case you need
>> a valid certificate to give the ranom NAT-OA in any case. So
>> if you lie about your NAT-OA I can just revoke you.
> 
> I don't see how NHRP authentication would help if the attacker
> takes over someone else's address and causes packets for the
> third party to go to it.
> 
> As to revoking the attacker's access, that's like saying "I'll
> run an open telnet port but if you try to sniff my psasword I'll
> revoke your access" :)

Sorry, I misunderstood the point in the beginning. Yes, if
someone else has valid cert, and has possession of the same
public IP and knows the private-ip I am using, and makes
SA with that info, he might be able to steal my traffic.

Good point.

And thinking more about it, NAT-OA might be even same for
multiple separate clients (if they are double natted).

>> Or do you have, other recommendations how to distinguish peers
>> behind same public IP than NAT-OA? Maybe we add the certificate
>> subject to xfrm state and neighbour cache. And use that?
> 
> Yes I think that is a much better solution for this.

Still, connecting it with variable amount length of data
(like cert subject) might be cumbersome (extending the
APIs, structs, especially neighbor cache).

Then again it might not be easy to come up with identifying
information if we limit it to 32-bits or 64-bits or so.

Ok, I'll think about this more. Ideas would be appreciated
here.

- Timo

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Herbert Xu Jan. 22, 2009, 10:10 a.m. UTC | #18
On Thu, Jan 22, 2009 at 12:00:22PM +0200, Timo Teräs wrote:
>
> Still, connecting it with variable amount length of data
> (like cert subject) might be cumbersome (extending the
> APIs, structs, especially neighbor cache).

Well you can have a translation table in user-space that turns
the cert into something more digestable, like a u32 or even an
IP address.

Cheers,
Timo Teras Jan. 23, 2009, 6:18 a.m. UTC | #19
David Miller wrote:
> From: Timo Teräs <timo.teras@iki.fi>
>> David Miller wrote:
>>> From: Timo Teräs <timo.teras@iki.fi>
>>>> Is there any particular reason why setting NAT-OA info should/
>>>> must be done using netlink? Or is this just a way to try to
>>>> put more pressure for the change to happen?
>>> Because it isn't deprecated if we keep adding features to it.
>> I would not consider this a new feature. It just makes pfkey
>> act consistently. If you don't want it supported, it'd make
>> more sense to not #define SADB_X_EXT_NAT_T_OA and drop all of
>> the verification code already present than to silently
>> ignore it. Make kernel return an error if some tried using it.
> 
> Fair enough, sounds like a reasonable argument.
> 
> Herbert what do you think?  The proposal is that we just reflect the
> value we get, rather than acting upon it.

Going back to the original patch. Knowing that it's bad idea to
use it for my other purposes, I have no strong feelings to push
for this.

Then again, since as discussed earlier, encap_oa is not used for
anything (and never will be). It's just stored and given back if
requested, so this patch adds nothing new; it just stores and
gives the attribute back if the SA is dumped.

Do note that currently pfkey_msg2xfrm_state() currently allocates
xfrm_encap_tmpl with kmalloc() and leaves encap_oa uninitialized,
thus all SAs with NAT-T created via pfkey currently show random
crap in encap_oa when dumped via "ip xfrm state".

Also the patch I sent first should memset() the encap_oa if
SADB_X_EXT_NAT_T_OA is not present.

If you still consider that the copying of that extension is
inappropriate then maybe memset():ing the encap_oa to zero
would be acceptable?

Cheers,
  Timo

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Herbert Xu Jan. 23, 2009, 9:09 a.m. UTC | #20
On Fri, Jan 23, 2009 at 08:18:33AM +0200, Timo Teräs wrote:
.
> If you still consider that the copying of that extension is
> inappropriate then maybe memset():ing the encap_oa to zero
> would be acceptable?

Yes I think doing the memset/zero assignment is not only reasonable
but a requirement since it has security implications.

Thanks,
diff mbox

Patch

diff --git a/net/key/af_key.c b/net/key/af_key.c
index f8bd8df..f3e9998 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -824,6 +824,7 @@  static struct sk_buff *__pfkey_xfrm_state2msg(struct xfrm_state *x,
 		size += sizeof(struct sadb_x_nat_t_type);
 		size += sizeof(struct sadb_x_nat_t_port);
 		size += sizeof(struct sadb_x_nat_t_port);
+		size += sizeof(struct sadb_address) + sockaddr_size;
 	}
 
 	skb =  alloc_skb(size + 16, GFP_ATOMIC);
@@ -1031,6 +1032,20 @@  static struct sk_buff *__pfkey_xfrm_state2msg(struct xfrm_state *x,
 		n_port->sadb_x_nat_t_port_exttype = SADB_X_EXT_NAT_T_DPORT;
 		n_port->sadb_x_nat_t_port_port = natt->encap_dport;
 		n_port->sadb_x_nat_t_port_reserved = 0;
+
+		/* original address */
+		addr = (struct sadb_address*) skb_put(skb,
+					      sizeof(struct sadb_address)+sockaddr_size);
+		addr->sadb_address_len =
+			(sizeof(struct sadb_address)+sockaddr_size)/
+			sizeof(uint64_t);
+		addr->sadb_address_exttype = SADB_X_EXT_NAT_T_OA;
+		addr->sadb_address_proto = 0;
+		addr->sadb_address_reserved = 0;
+		addr->sadb_address_prefixlen =
+			pfkey_sockaddr_fill(&natt->encap_oa, 0,
+					    (struct sockaddr *) (addr + 1),
+					    x->props.family);
 	}
 
 	/* security context */
@@ -1285,6 +1300,11 @@  static struct xfrm_state * pfkey_msg2xfrm_state(struct net *net,
 				ext_hdrs[SADB_X_EXT_NAT_T_DPORT-1];
 			natt->encap_dport = n_port->sadb_x_nat_t_port_port;
 		}
+		if (ext_hdrs[SADB_X_EXT_NAT_T_OA-1]) {
+			struct sadb_address* n_oa =
+				ext_hdrs[SADB_X_EXT_NAT_T_OA-1];
+			pfkey_sadb_addr2xfrm_addr(n_oa, &natt->encap_oa);
+		}
 	}
 
 	err = xfrm_init_state(x);