chiark / gitweb /
secnet.git
10 years agoudp: Support IPv6 (mostly)
Ian Jackson [Tue, 2 Sep 2014 08:05:30 +0000 (09:05 +0100)]
udp: Support IPv6 (mostly)

Specifically:

 * struct udp now contains an array of (up to three) pairs of iaddr,
   fd.  Code which deals with the fd and addr has been updated to use
   loops etc. as appropriate.

 * The sockets are created with the right protocol family value.
   For AF_INET6, we set IPV6_V6ONLY.

 * Specifically, when transmitting, we try all appropriate sockets and
   compute the persistent-failure indication as required.

 * And a comm_addr now contains an `int ix' for udp.c's benefit,
   particularly when logging.

 * We use string_item_to_iaddr to convert the string to a socket
   address, rather than string_item_to_ipaddr.  The latter can cope
   only with IPv4 (and is now used only for private vpn addrs,
   proxies, etc.).

 * The default is now to create both IPv6 and IPv4 sockets.

Left undone are:

 * The special secnet proxy protocol has a 4-byte address prepended
   which implies IPv4.  I don't intend to fix this.

 * The authbind support for IPv6 will be in a future patch.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agoudp.c: Remove some (ab)use of variable name `i'
Ian Jackson [Tue, 2 Sep 2014 07:59:44 +0000 (08:59 +0100)]
udp.c: Remove some (ab)use of variable name `i'

I find it very odd to find `item_t *i' etc.  I would like to be able
to use `int i'.  So change some uses of `i' to `item'.  (`j' in this
function will go away in the next patch so isn't worth renaming.)

No functional change.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agoProvide string_item_to_iaddr
Ian Jackson [Tue, 2 Sep 2014 08:04:27 +0000 (09:04 +0100)]
Provide string_item_to_iaddr

This will be used shortly.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agoProvide ARRAY_SIZE
Ian Jackson [Tue, 2 Sep 2014 07:58:24 +0000 (08:58 +0100)]
Provide ARRAY_SIZE

No call sites yet.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agoMake list_length and string_item_to_ipaddr const-correct.
Ian Jackson [Tue, 2 Sep 2014 07:56:50 +0000 (08:56 +0100)]
Make list_length and string_item_to_ipaddr const-correct.

No functional change.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agoudp: Break out udp_make_socket
Ian Jackson [Tue, 2 Sep 2014 06:41:37 +0000 (07:41 +0100)]
udp: Break out udp_make_socket

Make this into a function by itself and adjust its arguments so that
when we support multiple sockets (for multiple addresses so that we
can have multiple AFs) we can just call it for each one.

No functional change.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agoipv6: Support printing, comparing, etc. IPv6 addresses
Ian Jackson [Sun, 29 Jun 2014 22:10:31 +0000 (23:10 +0100)]
ipv6: Support printing, comparing, etc. IPv6 addresses

If we support IPv6, convert addresses with adns_addr2text.  Otherwise
stick with inet_ntoa.

With these changes, there is nothing remaining that will actually
crash secnet if it is passed an IPv6 address.  However, it is not yet
possible to mention IPv6 addresses in the configuration, and the udp
transport needs dual stack support.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agoipv6: check for support in system and in adns
Ian Jackson [Sun, 29 Jun 2014 22:58:03 +0000 (23:58 +0100)]
ipv6: check for support in system and in adns

We #define CONFIG_IPV6 if the system has AF_INET6 and adns has
adns_addr2text (which only the IPv6-capable adns has).

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agoautoconf: Update to autoconf 2.69
Ian Jackson [Sun, 29 Jun 2014 22:54:52 +0000 (23:54 +0100)]
autoconf: Update to autoconf 2.69

Rerun autoconf (Debian 2.69-1 i386) to update the configure script.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agoipv6: More buffers in iaddr_to_string
Ian Jackson [Sun, 29 Jun 2014 22:15:58 +0000 (23:15 +0100)]
ipv6: More buffers in iaddr_to_string

We are going to have addresses of multiple address families in various
places, which will mean more calls to iaddr_to_string for the benefit
of the same logging statement.

Increase the number of static buffers used by iaddr_to_string from 2
to 8.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agocomm etc.: Provide comm_addr_equal
Ian Jackson [Sun, 5 Oct 2014 11:03:21 +0000 (12:03 +0100)]
comm etc.: Provide comm_addr_equal

Abolish the rule that a comm_addr has zeroes in all its holes.
Provide comm_addr_equal instead.

We can get rid of a lot of calls to FILLZERO.

In resolver.c we no longer need to copy the fields of ia individually.
We still need to look at the incoming address family since util.c
aborts on unknown AFs and adns (perhaps a new version or something)
might have sent us things we don't understand.  (Also reorganise the
loop/switch a little to get `wslot++' out of the `case'.)

We have to move the declaration of iaddr_equal.

Abolish transport_addrs_equal and replace it at call sites with the
new comm_addr_equal.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agoipv6: introduce union iaddr
Ian Jackson [Wed, 26 Feb 2014 15:57:21 +0000 (15:57 +0000)]
ipv6: introduce union iaddr

Replace many occurrences of sockaddr_in by a new union, iaddr.

Everywhere that fills in an address has been modified to look into the
subfields of iaddr.  Also, replace references to the size of a
sockaddr_in by the new function iaddr_socklen.

But there is not yet any support for a union iaddr to contain anything
other than a sockaddr_in.  This will be added gradually in forthcoming
patches, starting at consumers and working back.

Additionally, a couple of places that specified a port and address as
a uint16_t and uint32_t have been converted.

We have changed only transport addresses - that is, addresses on the
public network.  VPN addresses remain IPv4 only.

We provide a few helper functions for manipulating union iaddr, such
as iaddr_to_string (which replaces saddr_to_string).

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agosite: Remove "wishful thinking" from transport address handling comment
Ian Jackson [Fri, 19 Sep 2014 21:02:30 +0000 (22:02 +0100)]
site: Remove "wishful thinking" from transport address handling comment

We have now completed the implementation of the algorithms described
in the comment.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agosite: Change default number of mobile peers
Ian Jackson [Fri, 3 Oct 2014 17:32:25 +0000 (18:32 +0100)]
site: Change default number of mobile peers

As the comment has it:

   - The default number of addrs to keep is 3, or 4 if we have a
     configured name or address.  That's space for two configured
     addresses (one IPv6 and one IPv4), plus two received addresses.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agosite: Permit multiple peer addresses even if peer is static
Ian Jackson [Sat, 20 Sep 2014 00:14:17 +0000 (01:14 +0100)]
site: Permit multiple peer addresses even if peer is static

This is necessary to permit multiple addresses of multiple address
families.  We (arbitrarily) set the default limit to 3.

Abolish the MAX_MOBILE_PEERS_MAX constant and size the peer addresses
array by MAX_PEER_ADDRS directly.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agoresolver: construct comm_addr; honour multiple addresses from the resolver
Ian Jackson [Sat, 28 Jun 2014 16:32:34 +0000 (17:32 +0100)]
resolver: construct comm_addr; honour multiple addresses from the resolver

We move construction of the comm_addr into the resolver.  The comm_if
and port are supplied to it by site and filled in by the resolver.
This allows the resolver to return a complete comm_addr array.

While we're here, we make an adns_r_addr query instead of an adns_r_a
query.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agosite: transport peers: Update bulk of code for multiple addresses
Ian Jackson [Sat, 28 Jun 2014 16:15:37 +0000 (17:15 +0100)]
site: transport peers: Update bulk of code for multiple addresses

Make the transport_peers functions which receive name resolution
information cope with multiple addresses.

(We cannot yet receive multiple addresses from the resolver.  That
will come next.)

This is just plumbing: no functional change (other than tiny
changes to log messages) in this patch.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agosite: Provide transport_record_peers to cope with multiple addresses
Ian Jackson [Sat, 28 Jun 2014 16:11:07 +0000 (17:11 +0100)]
site: Provide transport_record_peers to cope with multiple addresses

This is a complete replacement of transport_record_peer by this new
function.  The semantics are similar to the old function, except that
the new one copes with multiple addresses at once (ensuring that they
arrive, in order, at the front of the array).  It now needs its
caller to call transport_peers_expire.

We provide a convenience function transport_expire_record_peers for
the various call sites that want to call expire and then record.

As yet, there are no callers of transport_record_peers which pass
naddrs!=1 so there is no overall functional change.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agosite: transport peers: Delete or demote unsuitable peers addresses
Ian Jackson [Sat, 28 Jun 2014 14:32:19 +0000 (15:32 +0100)]
site: transport peers: Delete or demote unsuitable peers addresses

If comm signals that the address is unuseable (ie we have no IPv4 or
IPv6 interface or routing), delete the address.  Or, if we are mobile,
demote it to the end of the list (since we might gain appropriate
routing in the future).

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agocomm_if: Define the meaning of ->sendmsg returning false
Ian Jackson [Sat, 28 Jun 2014 13:26:56 +0000 (14:26 +0100)]
comm_if: Define the meaning of ->sendmsg returning false

site's transport logic is going to want to know when a failure occurs
which is attributable to the address being unsuitable for the local
network environment (eg v4 address on v6-only host).

Use the boolean return value from sendmsg for that.

At the moment all the callers ignore the return value, and the only
actual sendmsg function always returns true.  This is consistent with
the new semantics.

Therefore, no functional change in this patch.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agosite: transport peers: Notes on multi-address-family (IPv6) support
Ian Jackson [Sat, 28 Jun 2014 13:18:19 +0000 (14:18 +0100)]
site: transport peers: Notes on multi-address-family (IPv6) support

Update the comment about transport peer address handling.  This
defines the new regime for dual-stack support, which are going to
implement in the following patches.

No functional change.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agosite: transport peers: Formalise interface to transport peers
Ian Jackson [Wed, 25 Jun 2014 20:32:45 +0000 (21:32 +0100)]
site: transport peers: Formalise interface to transport peers

Make the interface to the transport peers functions more formal:
define when each function is called and what (roughly) it should do.

Remove the predeclaration of transport_record_peer.  This is now an
internal function for the transport peer management code; there are no
callers in the body of site.c and we can remove the declaration.

No functional change.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agosubnet_to_string: Do not allocate
Ian Jackson [Fri, 19 Sep 2014 22:37:58 +0000 (23:37 +0100)]
subnet_to_string: Do not allocate

None of the three call sites want to keep the value for any length of
time - they just use it right away.  Replace the allocation with a use
of the round-robin buffers from ipaddr_getbuf, and remove the frees at
the call sites.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agoUse memcpy helpers and FILLZERO
Ian Jackson [Thu, 2 Oct 2014 14:28:54 +0000 (15:28 +0100)]
Use memcpy helpers and FILLZERO

Replace various calls to memcpy and memset with equivalent calls to
these macros.

There are still a couple of open-coded memcpy(,buf_unprepend(),)
in udp.c's proxy code which will be done later.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agoProvide various wrappers for memcpy (COPY_OBJ, BUF_...)
Ian Jackson [Thu, 2 Oct 2014 14:26:58 +0000 (15:26 +0100)]
Provide various wrappers for memcpy (COPY_OBJ, BUF_...)

These wrappers avoid specifying the size separately or twice, and
COPY_OBJ can check for type safety.

No call sites yet.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agosite: Permit transport-peers-max to be equal to MAX_PEER_ADDRS
Ian Jackson [Fri, 3 Oct 2014 20:21:39 +0000 (21:21 +0100)]
site: Permit transport-peers-max to be equal to MAX_PEER_ADDRS

Not just one less.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agoMakefile.in: Use -MMD, not depend.sh
Ian Jackson [Thu, 2 Oct 2014 15:29:21 +0000 (16:29 +0100)]
Makefile.in: Use -MMD, not depend.sh

Generate the dependency information in .d files automatically with
-MMD, rather than explicitly in the Makefile.  This is faster, and
more reliable.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agoautoconf: Provide an autogen.sh
Ian Jackson [Sun, 5 Oct 2014 20:05:58 +0000 (21:05 +0100)]
autoconf: Provide an autogen.sh

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agotest-example: Provide a fuzzer for the slip decoder base.ipv6.v3 base.ipv6.v4
Ian Jackson [Sun, 21 Sep 2014 16:53:41 +0000 (17:53 +0100)]
test-example: Provide a fuzzer for the slip decoder

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agoslip: Do not malloc the userv activation context etc.
Ian Jackson [Mon, 22 Sep 2014 14:40:40 +0000 (15:40 +0100)]
slip: Do not malloc the userv activation context etc.

This is unnecessary, as its lifetime does not exceed that of the stack
frame.  Replace all the fixed-size malloc/free pairs with local
variables.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agofds: Introduce pipe_cloexec()
Ian Jackson [Sat, 27 Sep 2014 10:41:53 +0000 (11:41 +0100)]
fds: Introduce pipe_cloexec()

Replace all calls to pipe() with this new function, which checks
errors for us, and also sets both fds to close-on-exec.

There are some minor functional changes:
 * Error messages from pipe() failing are now less detailed about the
   context.  This is not important.
 * The signal self-pipe is now cloexec too.  This is at worst harmless.
 * When execing userv-ipif we rely on cloexec to close the spare
   copies of the pipe ends.
 * The stderr self-pipe spare writing end is redudantly made cloexec
   even though it is about to be closed shortly afterwards.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agofds: Simplify fd close condition in tun_set_route
Ian Jackson [Sat, 27 Sep 2014 10:58:42 +0000 (11:58 +0100)]
fds: Simplify fd close condition in tun_set_route

Recreating the condition under which the fd was opened is confusing
and fragile.  Instead, simply close it if we opened it, which we can
tell from the value of the variable (because we initialise it to -1 at
the top).

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agofds: Provide cloexec() and use it in udp.c and tun.c
Ian Jackson [Mon, 22 Sep 2014 14:51:30 +0000 (15:51 +0100)]
fds: Provide cloexec() and use it in udp.c and tun.c

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agochangelog, Makefile.in: finalise 0.3.4 base.fuzz-slip-decoder.2 base.fuzz-slip-decoder.v2 v0.3.4
Ian Jackson [Mon, 22 Sep 2014 15:17:02 +0000 (16:17 +0100)]
changelog, Makefile.in: finalise 0.3.4

10 years agoSECURITY: fixed fix to buffer handling
Simon Tatham [Mon, 22 Sep 2014 09:28:05 +0000 (10:28 +0100)]
SECURITY: fixed fix to buffer handling

The implementation of buf_remaining_space in 92795040 was entirely
broken.  It failed to take buf->size into account at all !

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agochangelog, Makefile.in: finalise 0.3.3 v0.3.3
Ian Jackson [Fri, 19 Sep 2014 22:51:40 +0000 (23:51 +0100)]
changelog, Makefile.in: finalise 0.3.3

10 years agobuffers: Rename buffer_if.len to buffer_if.alloclen.
Ian Jackson [Fri, 19 Sep 2014 23:05:19 +0000 (00:05 +0100)]
buffers: Rename buffer_if.len to buffer_if.alloclen.

This field contains the total amount of space allocated, starting at
base, which may be less than the amount of space available after
start.

Rename it to help avoid confusion.  This also enabled me to review
every site where this variable was used to verify that the length
checks are all now correct.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agobuffers: Introduce buf_remaining_space
Ian Jackson [Fri, 19 Sep 2014 23:03:20 +0000 (00:03 +0100)]
buffers: Introduce buf_remaining_space

This calculates the remaining space available to append to a buffer.

Use it in tun_afterpoll and udp_afterpoll (no functional change),
slip_unstuff and buf_append (fixes what appear to be latent bugs).

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agoipaddr_to_string: SECURITY: Do not allocate
Ian Jackson [Fri, 19 Sep 2014 22:35:06 +0000 (23:35 +0100)]
ipaddr_to_string: SECURITY: Do not allocate

ipaddr_to_string is used in many places including runtime logging.
Handling its memory allocation is annoyingly fiddly.  Indeed there is
at least one possible memory leak, which represents a potential denial
of service bug.

None of the callers keep the answers for any length of time.

So make it return the next one of a series of round-robin buffers,
instead, and remove all the freeing at all the call sites.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agoudp: SECURITY: Pass correct size argument to recvfrom
Ian Jackson [Fri, 19 Sep 2014 22:21:22 +0000 (23:21 +0100)]
udp: SECURITY: Pass correct size argument to recvfrom

Otherwise we risk overflowing the buffer.  This is a critical security
problem.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agochangelog, Makefile.in: finalise 0.3.3~beta1 debian/0.3.3_beta1
Ian Jackson [Thu, 18 Sep 2014 23:19:31 +0000 (00:19 +0100)]
changelog, Makefile.in: finalise 0.3.3~beta1

10 years agosite: transport peers: Use source of NAK packets as reply address
Ian Jackson [Wed, 25 Jun 2014 20:43:00 +0000 (21:43 +0100)]
site: transport peers: Use source of NAK packets as reply address

If we get a NAK from our current peer and initiate a key exchange, we
should take the source address of the NAK as a hint for the peer's
public address.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agosite: transport peers: MSG1: use transport_compute_setupinit_peers
Ian Jackson [Wed, 25 Jun 2014 20:32:03 +0000 (21:32 +0100)]
site: transport peers: MSG1: use transport_compute_setupinit_peers

This implies a functional change: now we start out with the data
transport peers.  For a mobile peer this is a bugfix; for a non-mobile
peer it implies no functional change.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agosite: transport_peers: Rename incoming_packet_addr
Ian Jackson [Wed, 25 Jun 2014 20:26:29 +0000 (21:26 +0100)]
site: transport_peers: Rename incoming_packet_addr

Rename the prod_hint_addr argument; we are going to use it for other
things too.

No functional change.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agosite: transport peers: Break out transport_resolve_complete,_tardy
Ian Jackson [Wed, 25 Jun 2014 17:37:24 +0000 (18:37 +0100)]
site: transport peers: Break out transport_resolve_complete,_tardy

Make two new functions
  transport_resolve_complete
  transport_resolve_complete_tardy
which encapsulate the transport peers manipulations for these two
situations.

No functional change.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agotest-example: Provide clean target in Makefile
Ian Jackson [Sun, 14 Sep 2014 16:00:06 +0000 (17:00 +0100)]
test-example: Provide clean target in Makefile

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agomake-secnet-sites: Put our path component at the beginning
Ian Jackson [Sun, 14 Sep 2014 15:57:28 +0000 (16:57 +0100)]
make-secnet-sites: Put our path component at the beginning

Otherwise installing the modern `ipaddr' python module (as found eg
python-ipaddr.deb) breaks secnet, because it will appear on the path
before our own copy of the Cendio Systems AB one.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agochangelog, Makefile.in: finalise 0.3.2 v0.3.2
Ian Jackson [Thu, 26 Jun 2014 19:29:54 +0000 (20:29 +0100)]
changelog, Makefile.in: finalise 0.3.2

10 years agochangelog, Makefile.in: finalise 0.3.2~beta1 debian/0.3.2_beta1
Ian Jackson [Fri, 6 Jun 2014 00:18:59 +0000 (01:18 +0100)]
changelog, Makefile.in: finalise 0.3.2~beta1

10 years agosite: Force use of configured name only if we are mobile
Ian Jackson [Mon, 2 Jun 2014 16:45:52 +0000 (17:45 +0100)]
site: Force use of configured name only if we are mobile

In c22c3541 we arranged to honour our local configured name for the
peer even if the peer initiated the key setup.  Previously we used the
address on the incoming packets.

However, this change can break some half-broken configurations, which
would otherwise mostly work.  Some of these configurations may even be
deliberate, as a kind of poor version of the mobile site feature.

But, if we are a mobile site it is very unlikely that we have a broken
name or address (or at least, if we do, that things would work well).

So, for now, restrict this new behaviour to the situation where we are
mobile.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agochangelog: Document additional name resolution
Ian Jackson [Sun, 18 May 2014 13:54:20 +0000 (14:54 +0100)]
changelog: Document additional name resolution

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agosite: Do name resolution on peer-initiated key setup too
Ian Jackson [Tue, 13 May 2014 23:40:44 +0000 (00:40 +0100)]
site: Do name resolution on peer-initiated key setup too

The current arrangement locks in the peer address used for key
exchange, for the lifetime of the key.

Most configurations do not have the secnet bind to a particular
address.  And secnet takes no particular care about the source address
on its packets.  The result is that secnet might reply from a
different address.  (Also NAT might cause this effect.)

But (unless the peer is mobile) it is not ideal to use an address
other than the configured address.  In particular, if we are mobile
then the network environment, and our routing to the peer, might
change so that the previous source address is not valid.  This could
result in an extended failure (of up to the key expiry lifetime).

Arguably this is a configuration error, but there is another reason to
dislike the current behaviour: it has the rather odd property that if
an opponent (or incompetent middlebox) reroutes/NATs the packets
during key exchange, the entire dataflow (at least in one direction)
might end up sent via a bizarre route (or, if the environment changes,
not delivered).

We still want to use the peer's address as a hint though: otherwise we
would have to stall a peer-initiated key setup while resolution takes
place.

So:

 * Initiate a peer address lookup when we get an incoming MSG1, if we
   have a configured name or address.  We do this in parallel with the
   key exchange.  (As a result it is possible that a peer address
   lookup might complete well after the key exchange has finished.)

 * Except, when the incoming MSG1 has crossed with ours, we must
   already have done the the lookup so do not do it again.

 * And, in this latter case do not unconditionally record the incoming
   peer address; instead, treat it as a "msgok"; otherwise we might
   unjustifiably overwrite an address we got from the configuration
   with the incoming address from the packet.

 * The two points above mean moving the transport_record_peer from
   process_msg1 into its two call sites, since the logic now needs to
   differ between them.

 * Handle the results of the MSG1-prompted peer address lookup.  In
   SITE_RESOLVE we do as we did before.  In most of the other states,
   we record the address and use it for future communications.

 * If the resolution fails with a non-mobile site, keep using the
   apparently-working peer address(es).

 * With a mobile site the currently working peer address might stop
   working so this is not acceptable.  In that case, make arrangements
   that a failed peer address lookup will be retried quickly - but in
   the meantime, there is no need to halt the packet flow.

 * If the attempt to submit the resolver query fails, just use the
   apparent peer address as before.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
---
Changes in v2:
  * Run the resolution after entering the new state, not before.
    Otherwise if we get the callback reentrantly, we get a bit
    confused.

10 years agosite: Log when resolution completes
Ian Jackson [Sat, 17 May 2014 15:57:10 +0000 (16:57 +0100)]
site: Log when resolution completes

This helps with debugging dns and reentrancy problems.

Also, assert in ensure_resolving that we have an address.  This makes
it slightly clearer that callers are expected to have checked this.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
---
New patch in v2 of the series.

10 years agosite: Make local_mobile be a site state variable
Ian Jackson [Tue, 13 May 2014 23:18:38 +0000 (00:18 +0100)]
site: Make local_mobile be a site state variable

We are going to want to know whether we are mobile to decide how to
handle certain name resolution failures.

Also fix a typo in a comment.

No functional change in this patch.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agosite: Explicitly track name resolution status
Ian Jackson [Tue, 13 May 2014 20:17:20 +0000 (21:17 +0100)]
site: Explicitly track name resolution status

Introduce a new variable st->resolving which tracks whether we have an
outstanding name resolution request.  This makes it safe to (try to)
start name resolution (via the new function ensure_resolving) multiple
times etc.

No resulting functional change from just this patch.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
---
Changes in v2:
  * Do slightly complicated dance with st->resolving, which is needed
    because of the reentrancy hazard posted by resolver->request.  In
    v1 of the series there was a bug here which could cause the site
    state machine to lock up.

10 years agosite: Fix bugs when resolver request submission fails
Ian Jackson [Tue, 13 May 2014 20:08:03 +0000 (21:08 +0100)]
site: Fix bugs when resolver request submission fails

Previously, if adns_submit failed:
 - the struct query in resolver.c was leaked
 - nothing was logged
 - the return value from resolver->request was ignored so the site
   state machine would hang

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
---
Changes in v2:
  * Fixed typo in commit message.

10 years agosite: Document some reentrancy hazards in comments
Ian Jackson [Sat, 17 May 2014 15:31:21 +0000 (16:31 +0100)]
site: Document some reentrancy hazards in comments

No functional change.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
---
New patch in v2 of the series.

10 years agoWhen printing version (eg during startup), use value from git-describe
Ian Jackson [Sun, 18 May 2014 13:50:04 +0000 (14:50 +0100)]
When printing version (eg during startup), use value from git-describe

Thus include git commit id where applicable.

Some complications in the Makefile[.in] are needed to ensure that the
version is regenerated iff required.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agochangelog: Document logging and security fix
Ian Jackson [Sun, 18 May 2014 10:48:29 +0000 (11:48 +0100)]
changelog: Document logging and security fix

10 years agosite logging: Log peer addresses on timeout
Ian Jackson [Sun, 11 May 2014 18:12:56 +0000 (19:12 +0100)]
site logging: Log peer addresses on timeout

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agocomm: Introduce comm_addr_to_string
Ian Jackson [Sun, 11 May 2014 18:12:03 +0000 (19:12 +0100)]
comm: Introduce comm_addr_to_string

Convenience function for calling addr_to_string.  Use it where
appropriate.

No functional change.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agosite logging: Break out logtimeout
Ian Jackson [Sun, 11 May 2014 17:26:57 +0000 (18:26 +0100)]
site logging: Break out logtimeout

We're going to add something to log peer addresses on timeout, so we
need to centralise these two timeout logging calls.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agosite logging: introduce vslog
Ian Jackson [Sun, 11 May 2014 17:26:14 +0000 (18:26 +0100)]
site logging: introduce vslog

No functional change.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agosite logging: Use [v]slilog_part in slog
Ian Jackson [Sun, 11 May 2014 17:10:58 +0000 (18:10 +0100)]
site logging: Use [v]slilog_part in slog

Eliminates a pointless log message assembly buffer.

No ultimate functional change.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agosite logging: Break out event_log_priority
Ian Jackson [Sun, 11 May 2014 17:07:38 +0000 (18:07 +0100)]
site logging: Break out event_log_priority

We're going to want to call this in more places.  While we're at it,
line the switch statement up more prettily.

No functional change.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agoMakefile.in: introduce -Wunused-function
Ian Jackson [Sun, 11 May 2014 15:36:52 +0000 (16:36 +0100)]
Makefile.in: introduce -Wunused-function

And delete the two unused logging functions log_multi and syslog_log.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agolog: Introduce slilog_part; abolish log_if->logfn
Ian Jackson [Sun, 11 May 2014 15:28:33 +0000 (16:28 +0100)]
log: Introduce slilog_part; abolish log_if->logfn

[v]Message provides a facility for sending messages to the system log
which are assembled out of pieces.  This is quite useful.  Generalise
it to other logger interfaces too:
  * Move the functionality out of vMessage into a new function
    vslilog_part.  Provide slilog_part too.
  * Move the assembly buffer: it was a static variable (used only
    for the system log); now it is a member of the log_if.  (Yes,
    the log_if, not the private state, because it applies for all
    loggers and is used by common code ie vslilog_part.)
  * Initialise log_if->buff[0] everywhere.
  * Rename LOG_MESSAGE_BUFLEN from MESSAGE_BUFLEN since now it
    has to live in secnet.h.

Also, remove log_if->logfn.  All the call sites use vlogfn.  Doing
this in this patch makes it easy to see that we haven't missed any
places where we should be initialising log_if->buff[0].

We currently have -Wunused ie -Wunused-functions, so for now we leave
the pointless definitions of the various loggers' logfns.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agosite: SECURITY: Properly update full peer address array
Ian Jackson [Thu, 15 May 2014 00:54:18 +0000 (01:54 +0100)]
site: SECURITY: Properly update full peer address array

If we already have the maximum number of peer addresses, do not
stuff the peer address into the wrong slot.

If a site instance is configured with the maximum permissible limit on
the number of mobile peer addresses (ie with mobile-peers-max set to
5), this overruns the transport peers array.  In such a configuration
this is a security problem.  It looks like a denial of service and
privilege escalation can't be ruled out.  Configurations without
mobile peers are not affected.

Otherwise it simply means the address is ignored.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agosecnet.h: Change bool_t to a C99 _Bool
Ian Jackson [Thu, 15 May 2014 00:31:56 +0000 (01:31 +0100)]
secnet.h: Change bool_t to a C99 _Bool

This will (a) stop misleading readers of the code (b) make it possible
to write code expecting an implicit !! to be applied to assignments to
booleans (c) possibly make secnet smaller or faster.

I don't expect this to produce any functional change, but I haven't
reviewed every bool_t in secnet to check.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agodebian/changelog: Start new version 0.3.2~~
Ian Jackson [Thu, 15 May 2014 00:29:13 +0000 (01:29 +0100)]
debian/changelog: Start new version 0.3.2~~

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agoMakefile.in: Improve push rune in release checklist
Ian Jackson [Thu, 15 May 2014 00:28:14 +0000 (01:28 +0100)]
Makefile.in: Improve push rune in release checklist

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agochangelog, Makefile.in: finalise 0.3.1 v0.3.1
Ian Jackson [Thu, 15 May 2014 00:10:34 +0000 (01:10 +0100)]
changelog, Makefile.in: finalise 0.3.1

10 years agochangelog, Makefile.in: finalise 0.3.1~beta3 debian/0.3.1_beta3
Ian Jackson [Thu, 8 May 2014 18:53:52 +0000 (19:53 +0100)]
changelog, Makefile.in: finalise 0.3.1~beta3

10 years agoMakefile.in: pass -Wno-unused-parameter explicitly
Ian Jackson [Wed, 7 May 2014 17:32:06 +0000 (18:32 +0100)]
Makefile.in: pass -Wno-unused-parameter explicitly

One would think that -Wno-unused would suffice.  However, in gcc 4.8.2
it doesn't, even though it does in 4.7.2.  This is arguably a bug in
gcc (filed as Debian #747345), but it looks like we will have to work
around it.

This avoids this error message:
 util.c:202:29: error: unused parameter 'phase' [-Werror=unused-parameter]
and several like it.

Reported-by: Stephen Early <sde@individualpubs.co.uk>
Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
CC: Stephen Early <sde@individualpubs.co.uk>
10 years agonetlink: fix const-correctness of ip_fast_csum fallback
Ian Jackson [Wed, 7 May 2014 16:54:24 +0000 (17:54 +0100)]
netlink: fix const-correctness of ip_fast_csum fallback

211cd627 corrected the constness of ip_csum and ip_fast_csum.  But the
latter has two implementations, and we missed the C fallback.  This
resulted in the following compile error on non-i386 platforms:
  netlink.c:606:2: error: passing argument 1 of `ip_fast_csum'
  discards `const' qualifier from pointer target type [-Werror]

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
CC: Stephen Early <sde@individualpubs.co.uk>
10 years agochangelog, Makefile.in: finalise 0.3.1~beta2 debian/0.3.1_beta2
Ian Jackson [Sat, 3 May 2014 17:58:51 +0000 (18:58 +0100)]
changelog, Makefile.in: finalise 0.3.1~beta2

10 years agonetlink: Generate ICMP correctly if point-to-point
Ian Jackson [Sat, 3 May 2014 11:04:32 +0000 (12:04 +0100)]
netlink: Generate ICMP correctly if point-to-point

In point-to-point configurations, we need to make sure that ICMP we
generate (a) has the right source address (we have to borrow the
address of the peer or the local host, depending) and (b) is delivered
in the right direction (back to wherever the bad packet came from).

To this end netlink_icmp_tmpl now takes an explicit ICMP source
address parameter, for netlink_icmp_simple to provide the correct
address.

We replicate a small amount of logic from netlink_incoming (the choice
between netlink_client_deliver, netlink_host_deliver, and
netlink_packet_forward/netlink_packet_deliver).  But netlink_incoming
is not suitable because it is intended only for packets from outside
secnet.  For example, in a non-ptp configuration it will reject
packets whose source address is secnet's address.  And writing it out
again is arguably clearer anyway.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agonetlink: Move local_address into struct netlink
Ian Jackson [Sat, 3 May 2014 11:10:11 +0000 (12:10 +0100)]
netlink: Move local_address into struct netlink

All the actual netlinks have this, and proper ICMP generation on
point-to-point links is going to need it.

The dummy "null" netlink previously didn't have this parameter; now,
it is mandatory.  This is an incompatible configuration change, but
only for configurations which contain a null netlink - which we think
is only done for testing or debugging.

No other functional change.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agonetlink: Plumb "sender" through to ICMP generation
Ian Jackson [Sat, 3 May 2014 10:49:35 +0000 (11:49 +0100)]
netlink: Plumb "sender" through to ICMP generation

In order for a point-to-point netlink to correctly construct and
deliver ICMP, it needs to know which way the original packet was
going.  So provide the ICMP generation code with this information, in
the form of the "sender" client.

No functional change.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agonetlink: Break out sender_name
Ian Jackson [Sat, 3 May 2014 11:19:42 +0000 (12:19 +0100)]
netlink: Break out sender_name

We are going to want to add another call site.

No functional change.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agonetlink: rename "client" to "sender" in many places
Ian Jackson [Sat, 3 May 2014 10:47:33 +0000 (11:47 +0100)]
netlink: rename "client" to "sender" in many places

In the bulk of the netlink code, the name "client" was used to refer
to the originating netlink_client (or NULL if the packet was
originated from the host or netlink itself).

This is a bit confusing.  Rename the variable to "sender", describing
its function rather than its type.

No functional change.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agochangelog, Makefile.in: finalise 0.3.1~beta1 debian/0.3.1_beta1
Ian Jackson [Thu, 1 May 2014 18:05:33 +0000 (19:05 +0100)]
changelog, Makefile.in: finalise 0.3.1~beta1

10 years agosite: Negotiate (configurable) MTU wip.frag.v1
Ian Jackson [Mon, 21 Apr 2014 21:11:34 +0000 (22:11 +0100)]
site: Negotiate (configurable) MTU

Calculate the inter-site mtu for each virtual link to a peer site, and
honour it.  This is the maximum of our desired mtu, and the peer's.

Here our desired MTU is normally our local kernel netlink mtu, but it
can be configured separately.

Extend MSG3/MSG4 optional data to contain an MTU advertisement.

The result is that overall it is possible to set an mtu-target which
is lower than the vpn's conventional mtu and expect shorter
on-the-wire packets to a particular site - provided that site's peers
are all upgraded.

In this patch we also update NOTES for the new protocol and README for
the new configuration option.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agosite: Remove clone-and-hack of signature verification
Ian Jackson [Mon, 21 Apr 2014 21:31:55 +0000 (22:31 +0100)]
site: Remove clone-and-hack of signature verification

process_msg3 and process_msg4 shared some signature checking etc.
code.  Move it into a common function.  No functional change other
than to error messages.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agonetlink: Advise netlink clients of the local link MTU
Ian Jackson [Mon, 21 Apr 2014 19:43:06 +0000 (20:43 +0100)]
netlink: Advise netlink clients of the local link MTU

From the netlink client's point of view this is advisory: it may be
that other peers (perhaps reached via that netlink) would prefer a
larger or smaller MTU.

This information will be consumed in later patches.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agonetlink: fix IP length check (SECURITY)
Ian Jackson [Sun, 13 Apr 2014 23:45:35 +0000 (00:45 +0100)]
netlink: fix IP length check (SECURITY)

This would erroneously abort on some very short packets.

This is a DoS vulnerability, exposed to internal sites only.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agonetlink: Only complain about initial frags for us
Ian Jackson [Sun, 13 Apr 2014 14:43:15 +0000 (15:43 +0100)]
netlink: Only complain about initial frags for us

secnet has no reassembly code and logs whenever it receives fragments.
Change this to only log when receiving initial fragments; this reduces
noise in the log.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agofragmentation: Fragment packets as required
Ian Jackson [Thu, 20 Mar 2014 02:01:53 +0000 (02:01 +0000)]
fragmentation: Fragment packets as required

netlink now honours its clients' and its local link's declared mtus.
If the packet is too large, it fragments it if possible, or perhaps
sends an ICMP Frag Needed error.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agoutil.h: Provide MIN and MAX macros
Ian Jackson [Mon, 21 Apr 2014 23:15:50 +0000 (00:15 +0100)]
util.h: Provide MIN and MAX macros

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agonetlink: Provide MDEBUG macro
Ian Jackson [Sun, 13 Apr 2014 14:05:40 +0000 (15:05 +0100)]
netlink: Provide MDEBUG macro

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agonetlink: Break out netlink_host_deliver
Ian Jackson [Sun, 13 Apr 2014 14:33:50 +0000 (15:33 +0100)]
netlink: Break out netlink_host_deliver

Provide a helper function for calling deliver_to_host, and use it at
the two call sites.

A side effect is that packets which came via point-to-point links are
properly counted.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agonetlink: Abolish client param to netlink_icmp_simple
Ian Jackson [Sun, 13 Apr 2014 14:29:39 +0000 (15:29 +0100)]
netlink: Abolish client param to netlink_icmp_simple

This parameter is not used (and some call sites pass NULL, anyway).

No functional change.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agofragmentation: Rename "frag_off" field to "frag"
Ian Jackson [Thu, 20 Mar 2014 02:00:59 +0000 (02:00 +0000)]
fragmentation: Rename "frag_off" field to "frag"

This 16-bit superfield contains all of the fragmentation info, not
just the offset.

No functional change.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agonetlink: Make ip_csum and ip_fast_csum const-correct
Ian Jackson [Thu, 20 Mar 2014 01:33:59 +0000 (01:33 +0000)]
netlink: Make ip_csum and ip_fast_csum const-correct

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agonetlink: Be more conservative about ICMP errors
Ian Jackson [Thu, 20 Mar 2014 00:18:21 +0000 (00:18 +0000)]
netlink: Be more conservative about ICMP errors

Default to not sending ICMP error messages for unknown incoming ICMP
type codes.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agonetlink: Set "unused" in ICMP header (SECURITY)
Ian Jackson [Wed, 19 Mar 2014 22:07:12 +0000 (22:07 +0000)]
netlink: Set "unused" in ICMP header (SECURITY)

Previously, the "unused" field in our ICMP messages was left
uninitialised (!)

This is a security problem, at least in principle, as the field would
as a result contain bits of previous packets.  In practice, the
information leaked could be IP options, TCP ports and sequence
numbers, or UDP ports, length and/or checksum, or similar information
for other protocols, so the impact is limited.

Set the field to 0.  Also, make provision for netlink_icmp_simple's
callers to be able to specify a different value, if desired.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agofragmentation: Fix fragmentation field check
Ian Jackson [Wed, 19 Mar 2014 21:50:36 +0000 (21:50 +0000)]
fragmentation: Fix fragmentation field check

When an incoming packet is for secnet itself, secnet checks the
fragmentation field in the IP header.

(Contrary to the spec, secnet discards fragmented packets addressed to
its own private address; however, this is a tolerable defect as secnet
never sends packets of its own apart from ICMP errors and ICMP Echo
Reply.)

However, secnet would incorrectly check the reserved flag bit in the
16-bit fragmentation superfield.  Fix this.  Also introduce some
manifest constants for the bits in the 16-bit fragmentation
superfield.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agoslip: Drop packets >mtu (SECURITY)
Ian Jackson [Sun, 13 Apr 2014 14:07:38 +0000 (15:07 +0100)]
slip: Drop packets >mtu (SECURITY)

Trying to send them to the kernel crashes userv-ipif.
This is a DoS vulnerability, exposed to internal sites only.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agotest-example: USE mtu of 1400 not 500 (!)
Ian Jackson [Wed, 9 Apr 2014 17:27:13 +0000 (18:27 +0100)]
test-example: USE mtu of 1400 not 500 (!)

500 is less than the IPv4 minimum MTU.  1400 is the conventional value
on the SGO VPN and is a plausible choice for vpns running mostly over
reasonably sane ethernets.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
10 years agotest-example: Provide test which uses unshare(8)
Ian Jackson [Sun, 13 Apr 2014 14:10:28 +0000 (15:10 +0100)]
test-example: Provide test which uses unshare(8)

This allows more realistic testing, with the "outside" copy of secnet
in a separate environment with its own instance of the network stack.

We have to go through some contortions to get the user a shell in the
"outside" environment, since unshare -n also breaks AF_UNIX, and we
want to keep the terminal secnet is invoked in just for secnet.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>