Page MenuHomeFreeBSD

IPSec: support for large replay windows
ClosedPublic

Authored by emeric.poupon_stormshield.eu on Nov 8 2016, 9:14 AM.

Details

Summary

This patch aims to support replay window sizes up to 2^32 - 32 packets.

Since the previous algorithm, based on bit shifting, does not scale with large replay windows, the algorithm used here is based on RFC 6479: IPsec Anti-Replay Algorithm without Bit Shifting.
The replay window will be fast to be updated, but will cost as many bits in RAM as its size.

The previous implementation did not provide a lock on the replay window, which may lead to replay issues.

Test Plan

Tests have been made using charon, with various replay window sizes.
With this new algorithm, the input throughput is stable whatever the anti replay window size

Some automated tests have been written to check the replay algorithm performs well on packet drops or on packet reordering.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

emeric.poupon_stormshield.eu retitled this revision from to IPSec: support for large replay windows.
emeric.poupon_stormshield.eu updated this object.
emeric.poupon_stormshield.eu edited the test plan for this revision. (Show Details)
net/pfkeyv2.h
100 ↗(On Diff #22078)

From my point of view, such changes aren't allowed. This structure is defined in RFC2367 and changing the size of fields and even the size of structure will lead to ABI breakage.
I think the proper way is introduce some new structure like sadb_sa2 was added.

154 ↗(On Diff #22078)

This also is objectionable change. RFC2367 says that mostly all fields of structures are in host byte order.
For big-endian systems it is backward compatible, but for little-endian it seems isn't.

netipsec/ipsec.c
1496 ↗(On Diff #22078)

I'd prefer to move this lock acquire/release into xform_xxx code. This will allow to reduce the number of its acquiring and also reduce conflicts with my oncoming changes :)

1560 ↗(On Diff #22078)

The same...

net/pfkeyv2.h
100 ↗(On Diff #22078)

I understand your point of view.

But this raises other questions:

  • can we extend non standardized messages to add a new field? I am thinking about the non standard sadb_x_sa2 struct you talked about
  • what can we do if both legacy and new fields are filled in? Which one takes the priority?
  • how can we update setkey to display the correct replay window?

The drawback is that we have to make changes in strongSwan.

netipsec/ipsec.c
1496 ↗(On Diff #22078)

I am not sure to understand, what did you mean exactly?

net/pfkeyv2.h
100 ↗(On Diff #22078)

can we extend non standardized messages to add a new field? I am thinking about the non standard sadb_x_sa2 struct you talked about

From my research it looks like only Linux and FreeBSD/NetBSD supports SADB_EXT_SA2 message type. But since Linux defines it in its header, I think its changing is undesirable.
You can add new structure and type. Then use it in SADB_ADD and SADB_UPDATE messages.

what can we do if both legacy and new fields are filled in? Which one takes the priority?

First of we should use value from SADB_EXT_SA. Then if new extension type is present, we override replay value.
When kernel reports SA to the userland, we look into the value, if size of sadb_sa_replay isn't enough to report current value, we can set it to maximum value and add new extension header with correct value. When sadb_sa_replay is enough, I think there is no need to add new extension header in the reply.

how can we update setkey to display the correct replay window?

I think we can just modify setkey in the base. Then this code can be used as a reference for strongswan/ipsec-tools.

netipsec/ipsec.c
1496 ↗(On Diff #22078)

Do not pay attention. I will start publish the code next week, so you will see what I mean.

net/pfkeyv2.h
100 ↗(On Diff #22078)

First of we should use value from SADB_EXT_SA. Then if new extension type is present, we override replay value.

I guess we should disable the replay window if the regular field is non zero and the extension field is set to 0?

Ok for your proposal, what about something like that?

In net/pfkeyv2.h:

/* Additional large replay window support */
struct sadb_x_sa_replay {
  u_int32_t sadb_x_sa_replay;
  u_int32_t sadb_x_sa_replay_reserved;
};  
_Static_assert(sizeof(struct sadb_x_sa_replay) == 8, "struct size mismatch");

I think it may be better to specify the replay window in packet unit instead of byte unit. It would make the code easier to read and maintain.
On Linux, the regular sadb_replay field is the width of the replay window, and we had to patch charon for this specific FreeBSD behavior.

Here is what the RFC says on the field: "The size of the replay window, if not zero. If zero, then no replay window is in use".

What do you think?

net/pfkeyv2.h
100 ↗(On Diff #22078)

I guess we should disable the replay window if the regular field is non zero and the extension field is set to 0?

I think so. Since application knows about our extension header, we expect its value has highest priority. But probably, when the kernel returns back the reply, it should reset replay value in SADB_EXT_SA to zero and omit SADB_EXT_SA_REPLAY header.

Ok for your proposal, what about something like that?

Looks good for me.

I think it may be better to specify the replay window in packet unit instead of byte unit. It would make the code easier to read and maintain.

You are the first who implementing this, so I think this is at your discretion.

emeric.poupon_stormshield.eu updated this object.

Hello,
Please find an updated version using extension messages.
I also checked the new behavior with a patched version of charon.

Edit: I think we may dump the counter of the SAs without locking the secasvar

sys/netipsec/key.c
6986 ↗(On Diff #22258)

It seems you missed m_cat() call here.

emeric.poupon_stormshield.eu added inline comments.
sys/netipsec/key.c
6986 ↗(On Diff #22258)

Yes thanks for the catch!

emeric.poupon_stormshield.eu marked an inline comment as done.

Added missing m_cat call.

ae edited edge metadata.
This revision is now accepted and ready to land.Nov 17 2016, 12:27 PM
This revision was automatically updated to reflect the committed changes.