chiark / gitweb /
secnet.git
3 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>
3 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>
3 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>
3 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>
3 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>
3 years agochangelog, Makefile.in: finalise 0.3.4 base.fuzz-slip-decoder.2 base.fuzz-slip-decoder.v2
Ian Jackson [Mon, 22 Sep 2014 15:17:02 +0000 (16:17 +0100)]
changelog, Makefile.in: finalise 0.3.4

3 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>
3 years agochangelog, Makefile.in: finalise 0.3.3
Ian Jackson [Fri, 19 Sep 2014 22:51:40 +0000 (23:51 +0100)]
changelog, Makefile.in: finalise 0.3.3

3 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>
3 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>
3 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>
3 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>
3 years agochangelog, Makefile.in: finalise 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

3 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>
3 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>
3 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>
3 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>
3 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>
3 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>
3 years agochangelog, Makefile.in: finalise 0.3.2
Ian Jackson [Thu, 26 Jun 2014 19:29:54 +0000 (20:29 +0100)]
changelog, Makefile.in: finalise 0.3.2

3 years agochangelog, Makefile.in: finalise 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

3 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>
3 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>
3 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.

3 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.

3 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>
3 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.

3 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.

3 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.

3 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>
3 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

3 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>
3 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>
3 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>
3 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>
3 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>
3 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>
3 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>
3 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>
3 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>
3 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>
3 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>
3 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>
3 years agochangelog, Makefile.in: finalise 0.3.1
Ian Jackson [Thu, 15 May 2014 00:10:34 +0000 (01:10 +0100)]
changelog, Makefile.in: finalise 0.3.1

3 years agochangelog, Makefile.in: finalise 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

3 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>
3 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>
3 years agochangelog, Makefile.in: finalise 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

3 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>
3 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>
3 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>
3 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>
3 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>
3 years agochangelog, Makefile.in: finalise 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

3 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>
3 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>
3 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>
3 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>
3 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>
3 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>
3 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>
3 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>
3 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>
3 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>
3 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>
3 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>
3 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>
3 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>
3 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>
3 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>
3 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>
3 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>
3 years agonetlink: Remove a newline from p-t-p startup message
Ian Jackson [Sun, 5 Jan 2014 15:33:27 +0000 (15:33 +0000)]
netlink: Remove a newline from p-t-p startup message

Another newline will be printed later, after the routes.  With one
link per tunnel it's better to make these messages (of which there are
many) one line each.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
3 years agonetlink: Avoid crash with clientless netlink
Ian Jackson [Sun, 5 Jan 2014 15:32:05 +0000 (15:32 +0000)]
netlink: Avoid crash with clientless netlink

In some pathological configurations, it can happen that a packet is
received from the kernel by a netlink which has no clients (that is,
where netlink_inst_reg has not been called).

Don't crash when this happens; instead, print a log message including
the source and destination addresses.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
3 years agonetlink: Break out netlink_client_deliver
Ian Jackson [Sun, 5 Jan 2014 15:30:09 +0000 (15:30 +0000)]
netlink: Break out netlink_client_deliver

Provide a helper function for calling client->deliver, and use it at
the two call sites.  We are going to fix a bug in this area and want
to bring the implementations together so we have only one place to
fix.

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

I have verified that I caught all of the call sites by experimentally
changing the field name from "deliver" to "deliverx" in netlink.h, and
inspecting the locations of the resulting compiler errors.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
4 years agoFix formatting error in secnet.8 manpage.
Ian Jackson [Sun, 1 Sep 2013 20:09:02 +0000 (21:09 +0100)]
Fix formatting error in secnet.8 manpage.

4 years agomore release checklist changes
Ian Jackson [Sun, 1 Sep 2013 19:59:39 +0000 (20:59 +0100)]
more release checklist changes

4 years agoUpdates to release checklist in Makefile.in.
Ian Jackson [Sun, 1 Sep 2013 19:46:03 +0000 (20:46 +0100)]
Updates to release checklist in Makefile.in.

4 years agoRelease of 0.3.0. No code changes since 0.3.0~beta3.
Ian Jackson [Sun, 1 Sep 2013 19:31:29 +0000 (20:31 +0100)]
Release of 0.3.0.  No code changes since 0.3.0~beta3.

4 years agochangelog, Makefile: finalise 0.3.0~beta3
Ian Jackson [Mon, 5 Aug 2013 10:54:28 +0000 (11:54 +0100)]
changelog, Makefile: finalise 0.3.0~beta3

4 years agochangelog: 0.3.0~beta3~~iwj1
Ian Jackson [Mon, 5 Aug 2013 10:50:24 +0000 (11:50 +0100)]
changelog: 0.3.0~beta3~~iwj1

4 years agosite: Initialise st->scratch with SETUP_BUFFER_LEN space.
Ian Jackson [Mon, 5 Aug 2013 10:45:50 +0000 (11:45 +0100)]
site: Initialise st->scratch with SETUP_BUFFER_LEN space.

Otherwise, if we're unlucky, we end up trying to construt a PROD
packet before we have ever used buffer_copy on scratch, which causes
an assertion failure ("buf->size <= buf->len - amount" at util.c:257).

Reported-by: Simon Tatham <anakin@pobox.com>
Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
4 years agochangelog: Mention netlink short packet discard.
Ian Jackson [Thu, 1 Aug 2013 16:27:58 +0000 (17:27 +0100)]
changelog: Mention netlink short packet discard.

4 years agonetlink: Safely discard short packets
Ian Jackson [Thu, 1 Aug 2013 16:36:39 +0000 (17:36 +0100)]
netlink: Safely discard short packets

Short packets arriving at the netlink should not be processed.

Previously, we suffered buffer read overruns.  In principle this might
be some kind of security problem, as it might make it possible to read
and have returned parts of the buffer which were used previously for
other data.  I haven't analysed this possibility in any detail.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
4 years agochangelog: Describe 0.3.0~beta2 master wip-rebasing
Ian Jackson [Thu, 25 Jul 2013 17:30:54 +0000 (18:30 +0100)]
changelog: Describe 0.3.0~beta2

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
4 years agosite: New PROD message
Ian Jackson [Thu, 25 Jul 2013 17:30:54 +0000 (18:30 +0100)]
site: New PROD message

We introduce a new message PROD which requests that the peer initiate
a key exchange with us, if it doesn't already have a key.

This helps significantly reduce a possible "dead period" when one end
of a connection is restarted during key exchange.  The dead period is
now limited to the time taken for the interrupted key exchange to time
out.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
4 years agoslip: Buffer management (max_start_pad) fixes
Ian Jackson [Thu, 25 Jul 2013 17:30:54 +0000 (18:30 +0100)]
slip: Buffer management (max_start_pad) fixes

Nothing in slip.c calls buffer_init for the first packet.  We don't
normally notice this because userv-ipif _both_ prints a confirmation
END byte right away, _and_ bookends packets with ENDs.

But this should be fixed.  Otherwise we fail an assertion when we try
to prepend things to the first data packet.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
4 years agoudp.c: call buffer_init
Ian Jackson [Thu, 25 Jul 2013 17:30:53 +0000 (18:30 +0100)]
udp.c: call buffer_init

Nothing in udp.c call buffer_init.  This might result in start padding
underflows (assertion failures) if packets come in via routes that
don't strip (much) off the front and then go out via routes that do
add lots at the front.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
4 years agomax_start_pad: calculate globally, not via client graph
Ian Jackson [Thu, 25 Jul 2013 17:30:53 +0000 (18:30 +0100)]
max_start_pad: calculate globally, not via client graph

There is quite a lot of plumbing of max_start_pad values from one
place to another.  Sadly this plumbing is not complete, which can lead
to crashes as the start padding is exhausted.  And completing it is a
lot of work which would be difficult to get correct, even if that's
possible in principle.

Instead, we take a different approach.  We calculate a single global
max_pad_start value that can be used everywhere.  It is the sum of:

 * Headers that "site" instances might need add to the start of
   packets (source and destination site indices, message type both
   inside and outside the encryption;

 * Anything that "transform" instances might need to add to the start
   of packets.  This depends on the transforms, but since it isn't a
   priori predictable which path any particular incoming packet might
   take, we have to take the worst case.

   These transform instances are applied only by "site" and each
   packet goes through at most on "forward" transform instance.

 * Anything that "comm" instances might need to add.  This is
   currently only needed for the proxy feature.

This is based on the assumption that a packet may follow one of these
paths:

    comm -->- site_incoming --.         ,-- site_outgoing -->- comm
                               \       /
                                netlink
                               /       \
    tun/slip ----->-----------'         `------>---------- tun/slip

On the inbound side, tun and slip have to set up the buffer with the
necessary start padding.

site_incoming only removes stuff from the beginning (since it uses
transform->reverse).  netlink doesn't add or remove anything.

It is site_outgoing and comm which may need to prepend things.  site
additionally calls transform->forwards.

If in the future a module appears which can take packets out of the
RHS of this diagram and feed them back into the left, it may have to
do something about the buffer.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
4 years agoUse FORMAT everywhere, and fix up the errors it finds
Ian Jackson [Thu, 25 Jul 2013 17:30:53 +0000 (18:30 +0100)]
Use FORMAT everywhere, and fix up the errors it finds

Many printf-like variadic functions weren't properly decorated with
FORMAT annotations.  Add them everywhere.

It is not possible to annotate a function pointer type, so that
doesn't work for the function pointers in struct log_if.  However, we
have convenience wrapper functions slilog and vslilog, which are
appropriately decorated and therefore safer.  Change all call sites to
use those instead, and leave a comment.  (Rename the function pointer
variable names so that we don't miss any call sites.)

Fix the bugs that this new compiler checking reveals.  These are
nearly all in fatal error reporting, and not very scary.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
4 years agosite: support multiple transforms
Ian Jackson [Thu, 25 Jul 2013 17:30:53 +0000 (18:30 +0100)]
site: support multiple transforms

The "transform" key in site's dictionary argument can now be a list,
as well as just a single transform.

We use 16 bits of the capability mechanism to advertise the transforms
we support; the config is supposed to nominate a transform capability
number (from 0 to 15) for each transform closure - although the
default numbers are sufficient if you don't need to do parameter
rollover.

The receiver of MSG2 intersects the two bitmaps and chooses the best
transform, and states its choice in MSG3.

A protocol downgrade attack is prevented by the fact that the
capability bitmaps are advertised in the signed parts of MSG3 and
MSG4.  (If the one in MSG4 doesn't match what was in MSG2, the MSG4 is
rejected and presumably the key exchange fails.)

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
4 years agosite, transform: per-transform-instance max_start_pad
Ian Jackson [Thu, 25 Jul 2013 17:30:53 +0000 (18:30 +0100)]
site, transform: per-transform-instance max_start_pad

Replicate the max_start_pad value from the transform interface into
the instance interface.

Use the instance's version in site.c.

While we're at it, add an assertion to confirm that the value of
max_start_pad passed to buffer_init doesn't overrun the buffer.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
4 years agosite, netlink: abolish max_end_pad and min_end_pad
Ian Jackson [Thu, 25 Jul 2013 17:30:52 +0000 (18:30 +0100)]
site, netlink: abolish max_end_pad and min_end_pad

There is much code to compute these values, but they are never used
anywhere.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
4 years agosite: dynamically create and destroy transform instances
Ian Jackson [Thu, 25 Jul 2013 17:30:52 +0000 (18:30 +0100)]
site: dynamically create and destroy transform instances

Rather than making three transform instances at setup time, and then
using setkey on them, we create transform instances as needed and
destroy them when we delete their keys.

This is necessary because we are going to support multiple different
kinds of transform, so each one of the three transforms might be of
different kinds (supplied by different secnet modules) at different
times.

The variables current.transform, auxiliary_key.transform and
new_transform can all be NULL now.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
4 years agosite: Check transform errors; factor out transform handling
Ian Jackson [Thu, 25 Jul 2013 17:30:52 +0000 (18:30 +0100)]
site: Check transform errors; factor out transform handling

Make sure we always check the error return from transform->forwards
and ->backwards.  Otherwise logic errors in the site state machine
might result in us sending out packets with unencrypted insider
plaintext, or the like, due to the transform being unkeyed when we try
to use it.

Factor some repeated idioms for transform handling into a set of new
functions.  This will make the next patch much easier.

We arrange to pass dispose_transform a pointer to the actual state
variable where the transform is kept; this means that it can be
changed to update that pointer.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
4 years agosite: interpret first 4 bytes of extrainfo as capabilities
Ian Jackson [Thu, 25 Jul 2013 17:30:52 +0000 (18:30 +0100)]
site: interpret first 4 bytes of extrainfo as capabilities

Define the first four bytes of the additional data (after the nul in
site names in MSG1..4) in the sender's name field to be a capability
bitmask.  Currently no capability flags are defined.

To support this, replace extrainfo and its _len in struct parsedname
with a struct buffer_if.

We record the capabilities from MSG1 or MSG3, and check that they
haven't changed when processing MSG2..4, as applicable.  (Because
older secnets don't cope with flags in MSG1, we have to define two
kinds of capability flag: ones expected to be in MSG1, and ones which
can appear later in the exchange.)

The support for "Early" capabilities is intended to be used in the
future to support algorithm agility in key exchange.

We also advertise our own capabilities (currently, all-bits-zero) in
our own messages.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
4 years agosite: use unaligned.h's functions, not pointer cast and ntohl
Ian Jackson [Thu, 25 Jul 2013 17:30:51 +0000 (18:30 +0100)]
site: use unaligned.h's functions, not pointer cast and ntohl

Switch site.c to using unaligned.h's functions for accessing
multi-byte values inside messages.  There were a few places where this
construction was used:
   something = ntohl(*(uint32_t*)(buf->start + offset));
It is much clearer to use this equivalent construction:
   something = get_uint32(buf->start + offset);

Also the packet's message type was extracted from the message using
get_uint32 and then put through ntohl.  This is, of course, wrong.
Currently all the message type codes are palindromes, so it doesn't
matter in practice.  Fix it anyway.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
4 years agosite: Extra info in name fields for MSG1, clearer processing
Ian Jackson [Thu, 25 Jul 2013 17:30:51 +0000 (18:30 +0100)]
site: Extra info in name fields for MSG1, clearer processing

We change the parsing of the names in MSG1 packets to align it with
that used for MSG2..4, abolishing the simple memcmp-based "setupsig"
arrangement.

This means that MSG1 packets, like MSG2..4, can contain extra
information in the name fields.

This results in the following table of behaviours (where "old secnet"
is the version before we introduced additional data at all):
  MSG    Sent name          Old secnet           New secnet
  1      expected           ok                   ok
  1      expected\0extra    rejected             ok, extra data
  1      expected/suffix    rejected             rejected
  1      expect             rejected             rejected
  2-4    expected           ok                   ok
  2-4    expected\0extra    ok, extra ignored    ok, extra data
  2-4    expected/suffix    wrongly accepted     rejected
  2-4    expect             read overrun         rejected

After the new secnet is widely deployed, we will be able to use the
extra data field in MSG1 for public key algorithm agility and public
key rollover.

Also, intend to introduce a new packet which (like MSG1) is addressed
by name rather than the site id; this change makes that easier too.

In detail, we:
 * Make site_incoming extract the message type earlier, and use
   the message type to choose how to check the address.
 * Abolish st->setupsig and st->setupsiglen;
 * Break out a function named_for_us, which uses unpick_msg
   and then checks the names;
 * Arrange to keep the unpicked message from named_for_us and
   reuse it in process_msg1.
 * Eliminate checking dest==0 for name-addressed packets.

This last point constitutes a change to the implemented protocol.  The
old code would treat a packet as name-addressed iff the destination
"index" was zero.  However, the requirement to send a zero in this
field is not documented.  After this patch, peers which send MSG1 with
a non-zero destination index will have their messages honoured rather
than discarded.  But we do document the restriction.

There is no other significant functional implication of this last
point.  Peers which send non-MSG1s with zero destination index will
have their packets rejected both before and afterwards (although there
may be a change in whether a NAK is sent and in logging) but anyway
those peers are broken because all of our generated indexes are
nonzero.

A downside of this change is that we will now unpick an incoming MSG1
repeatedly until we find which site instance wants it.  But this
unpicking is not particularly arduous.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
4 years agosite: fix site name checking leaving room for expansion
Ian Jackson [Thu, 25 Jul 2013 17:30:51 +0000 (18:30 +0100)]
site: fix site name checking leaving room for expansion

Previously, secnet would only check that the site names sent by the
peer had the expected site names as prefixes.  This would be a
security bug on a vpn where one site name is the prefix of another
site name.

In fact, if the peer sends a short name, secnet will suffer a buffer
read overrun and maybe crash - although this is exploitable only for
DoS at worst, and not as an information leak.

(With reasonable peers, it won't cause packets to be mishandled,
because the next field after each name string is a 16-byte integer
giving a name or public key byte count, which would have to contain a
value greater than 8000 to look like printable characters.)

Fix this bug.

However, we use this as an opportunity to provide some space (in the
signed part of MSG2..4) for future extensions.  (At present there is
nothing in the parts of MSG3 and MSG4 covered by the signature which
is not supposedly entirely fixed - apart from the amount of
zero-padding at the MS end of our DH public exponent.  This is bad
because it provides no way to do transport protocol upgrade/downgrade
negotiation without leaving open a protocol version downgrade attack.)

After this change the name fields in MSG2..4 are, following the 16-bit
length, either just the characters of the name, or the name, followed
by a nul byte, followed by zero or more bytes of additional data.

Additional data beyond what the receiver expects (which currently
means any additional data), is to be ignored.

Let us work through some examples.  Suppose the expected site name is
"expected".  Here are the behaviours in relation to various names
being transmitted in the packet.  (This applies both to the local
name, and to the remote name; "rejected" means other instances of the
protocol engine get a go, and perhaps the instance with the correct
names will accept the message.)
  MSG    Sent name          Old secnet           New secnet
  1      expected           ok                   ok
  1      expected\0extra    rejected             rejected
  1      expected/suffix    rejected             rejected
  1      expect             rejected             rejected
  2-4    expected           ok                   ok
  2-4    expected\0extra    ok, extra ignored    ok, extra data
  2-4    expected/suffix    wrongly accepted     rejected
  2-4    expect             read overrun         rejected

We will do the same for MSG1 in a later commit.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
4 years agoNOTES: Remove unimplemented protocol negotiation
Ian Jackson [Thu, 25 Jul 2013 17:30:51 +0000 (18:30 +0100)]
NOTES: Remove unimplemented protocol negotiation

The protocol negotiation mechanism documented in NOTES is not
implemented.  Remove it from the document.

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