chiark / gitweb /
6 years agochangelog: Mention netlink short packet discard. debian/0.3.0_beta2
Ian Jackson [Thu, 1 Aug 2013 16:27:58 +0000 (17:27 +0100)]
changelog: Mention netlink short packet discard.

6 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 <>
6 years agochangelog: Describe 0.3.0~beta2
Ian Jackson [Thu, 25 Jul 2013 17:30:54 +0000 (18:30 +0100)]
changelog: Describe 0.3.0~beta2

Signed-off-by: Ian Jackson <>
6 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

Signed-off-by: Ian Jackson <>
6 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 <>
6 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 <>
6 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

    comm -->- site_incoming --.         ,-- site_outgoing -->- comm
                               \       /
                               /       \
    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 <>
6 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 <>
6 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

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

Signed-off-by: Ian Jackson <>
6 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

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

Signed-off-by: Ian Jackson <>
6 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 <>
6 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 <>
6 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 <>
6 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"

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

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 <>
6 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 <>
6 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 <>
6 years agoNOTES: Remove paragraph about slow-to-prepare messages
Ian Jackson [Thu, 25 Jul 2013 17:30:51 +0000 (18:30 +0100)]
NOTES: Remove paragraph about slow-to-prepare messages

NOTES contained this paragraph:

  Some messages may take a long time to prepare (software modexp on slow
  machines); this is a "please wait" message to indicate that a message
  is in preparation.

This paragraph was immediately after the description of the msg0(msg9)
packet - ie, the ordinary data packet.  It is therefore at the very
least misplaced.

In the git history this paragraph was introduced in "Import release
0.1.14" (4f5e39ec).  However, the diff for that commit (ie the diff
between 0.1.13 and 0.1.4) does not show any code which might
correspond to this comment.

There is not currently any code in site.c which responds to a message
of this kind.  I have had a quick look for code which sends such a
message and failed to find any.

So I think this paragraph represents a never-implemented intent, and
should be removed.  It certainly predates the "hacky parallelism" by a
long way.

Signed-off-by: Ian Jackson <>
6 years agoNOTES: Improve documentation of NAKs.
Ian Jackson [Thu, 25 Jul 2013 17:30:50 +0000 (18:30 +0100)]
NOTES: Improve documentation of NAKs.

Improve the description of the function of NAK packets.

Document an ancient form of NAK, MSG8.

Add a missing heading "Other messages" and put the description of the
NAK message and of the obsolete NAK in it.

Signed-off-by: Ian Jackson <>
6 years agosite: Send NAKs for undecryptable data packets (msg0)
Ian Jackson [Thu, 25 Jul 2013 17:30:50 +0000 (18:30 +0100)]
site: Send NAKs for undecryptable data packets (msg0)

Packets which are not understood are supposed in general to produce
NAKs, to let the peer know that we don't have the key they were using.

However, previously this would only happen if the incoming packet had
a local site index which was not in use.  But it can happen that a
particular index value is reused by a recently restarted secnet.
Previously, in this case, data packets would simply be thrown away

With this change, undecryptable data packets always generate a NAK.
(But we don't generate a NAK if the packet was decryptable, but
suffered from sequence number skew - if we did, then network glitches
would trigger spurious key exchanges.)

This is particularly relevant for mobile sites, as it can happen that
the fixed site doesn't have an address for the mobile site - so the
association will remain stuck in a broken state until the mobile site
is also restarted.

There is still a potential problem when a site is restarted mid key
exchange.  The peer will refuse to start a new key exchange (because
of the retry timeout) and the restarted site may not know it's
necessary.  This will be dealt with in a later patch.

Signed-off-by: Ian Jackson <>
6 years agoudp, util: Break out send_nak function
Ian Jackson [Thu, 25 Jul 2013 17:30:50 +0000 (18:30 +0100)]
udp, util: Break out send_nak function

Move the code in udp.c which constructs NAKs into its own function
in util.c.  This will make it easier to reuse.

Signed-off-by: Ian Jackson <>
6 years agoudp.c: Do not send NAKs in response to NAKs
Ian Jackson [Thu, 25 Jul 2013 17:30:50 +0000 (18:30 +0100)]
udp.c: Do not send NAKs in response to NAKs

If an incoming packet isn't name-addressed and has an invalid
destination site id, udp.c would send a NAK.  This is not a good idea
- if somehow the source site id was wrong too, it will result in a NAK

This is a security vulnerability as it can be used by an attacker to
trigger an unending NAK storm.

Also, improve the message printed when a NAK is sent by udp.c because
no site wanted to handle it.

Signed-off-by: Ian Jackson <>
6 years agomagic: Introduce LABEL_NAK
Ian Jackson [Thu, 25 Jul 2013 17:30:50 +0000 (18:30 +0100)]
magic: Introduce LABEL_NAK

Replace ad-hoc comments on the message number of NAK packets with an
explicit #define.  No functional change.

Signed-off-by: Ian Jackson <>
6 years agotransform: Provide Serpent-EAX transform
Ian Jackson [Thu, 25 Jul 2013 17:30:49 +0000 (18:30 +0100)]
transform: Provide Serpent-EAX transform

This provides an alternative to the (rather badly broken)
serpent256-cbc transform.

In this patch, there is not yet any algorith negotiation for a smooth
upgrade, nor any changes to the example configurations.

Signed-off-by: Ian Jackson <>
6 years agotransform: Pass a direction flag to the transform
Ian Jackson [Thu, 25 Jul 2013 17:30:49 +0000 (18:30 +0100)]
transform: Pass a direction flag to the transform

The same transform is used for inbound and outbound packets.

The transform should know which direction these packets are flowing
in; that (a) allows a transform to reject packets which are "looping
back" so to speak, and (b) makes it easier for a transform to generate
unique nonces.

This will be used by the forthcoming EAX transform.  It is combined
with the sequence number (the same values of which are used by both
ends) to make the nonce, which must be unique across the single shared
key, ie unique across both flows.

Signed-off-by: Ian Jackson <>
6 years agotransform: Allow DH to set the key size
Ian Jackson [Thu, 25 Jul 2013 17:30:49 +0000 (18:30 +0100)]
transform: Allow DH to set the key size

It turns out that the current Serpent CBC-MAC transform takes the raw
DH shared secret, and parcels it up in byte ranges for the various
uses (some of which are published).  Well, obviously this is not a
good idea.

But also it means the interface isn't set up to allow the size of the
key data provided to the transform to be determined by the size of the
DH modulus.

Fix this latter interface problem.  Now a transform can set its keylen
to 0, meaning it will be provided with the whole of the DH private

We don't use this new feature yet.  We can't make the existing
transform use it without breaking compatibility, but it will be used
by the new EAX-based transform.

Signed-off-by: Ian Jackson <>
6 years agotransform: split out transform-common.h
Ian Jackson [Thu, 25 Jul 2013 17:30:49 +0000 (18:30 +0100)]
transform: split out transform-common.h

To avoid too much duplication, some boilerplate and helpful code from
transport.c is now brought out into macros in transport-common.h.

It will be reused in the later commits introducing the EAX transform.

Also, rename transform.c to transform-cbcmac.c, etc.

Signed-off-by: Ian Jackson <>
6 years agoEAX: provide an implementation of EAX
Ian Jackson [Thu, 25 Jul 2013 17:30:48 +0000 (18:30 +0100)]
EAX: provide an implementation of EAX

EAX is a reasonably well-regarded Authenticated Encryption block
cipher mode.  We intend to replace the existing CBC and CBC-MAC
transform with EAX.  EAX can be used with any block cipher, but we
will use it with Serpent.

In this patch we provide an implementation of EAX itself.

This primary consists of eax.c, the actual implementation of the EAX

To test that our implementation is correct, we use aes.[ch], which we
copied from qemu in the previous commit, to run the EAX-AES test
vectors.  These are cut-and-pasted out of the EAX paper.

(To do this we need to make aes.[ch] compile in our environment - but
the changes are minimal.  We also improve the copyright notices.)

Then for completeness we also provide EAX-Serpent and EAX-Serpent-BE
test vectors and the corresponding test code.  The EAX-Serpent test
vectors are from Mark Wooding.  The EAX-SerpentBE test vectors were
generated by this very code, so aren't independently verified.

(The implementation of what is now consttime_curious_multiply, and the
comment preeding it, was provided by Mark.  I have lightly edited it
to conform to the coding style etc. of the rest of the file.  Mark
also contributed improvements to alg_omac_t_k.)

Signed-off-by: Ian Jackson <>
Signed-off-by: Mark Wooding <>
6 years agocrypto: Copy an AES (Rijndael) implementation into tree
Ian Jackson [Thu, 25 Jul 2013 17:30:48 +0000 (18:30 +0100)]
crypto: Copy an AES (Rijndael) implementation into tree

We are going to want an implementation of AES so that we can run
publicly-provided test vectors of our EAX implementation.

qemu seem to have a version with a fairly convenient shape, so lift it
wholesale into our tree, from upstream qemu (their git commit

The files in this patch are _exactly_ as copied from that qemu tree,
so that we can separate out our own changes.

Signed-off-by: Ian Jackson <>
6 years agocrypto: Copy a SHA512 implementation into tree
Ian Jackson [Thu, 25 Jul 2013 17:30:48 +0000 (18:30 +0100)]
crypto: Copy a SHA512 implementation into tree

We are going to want an implementation of SHA512 (initially for
hashing DH secrets into EAX keys).

Copy the version from coreutils 8.5, along with u64.h.

In this patch, we commit exactly the files from coreutils.  They will
be made to compile later.  Doing it this way means we can more easily
isolate changes we have to make.

Copying these files from coreutils makes secnet GPL-3+.

Signed-off-by: Ian Jackson <>
6 years agoserpent: Ad-hoc debugging facility
Ian Jackson [Thu, 25 Jul 2013 17:30:48 +0000 (18:30 +0100)]
serpent: Ad-hoc debugging facility

Provide an ad-hoc debugging facility in serpent.c.

If the "#if 0" is changed to "#if 1", the key material, plaintext and
ciphertext of all Serpent operations is printed in hex to stderr.

We provide a new header file hexdebug.h to facilitate this.  And we
use this new header file in the "#if 0" debugging in transform_setkey.

No functional change.

Signed-off-by: Ian Jackson <>
6 years agoserpent: Provide little-endian version too, but ours is big
Ian Jackson [Thu, 25 Jul 2013 17:30:48 +0000 (18:30 +0100)]
serpent: Provide little-endian version too, but ours is big

Apparently, almost everyone else is using little-endian Serpent.

Since we are going to introduce a new transform, we can make our new
transform have conventional little-endian Serpent.  But we need to
retain the big-endian one for compatibility.

In this patch:
 * Make serpent.h compile-time bytesexual by providing a new definition
   of GETPUT_CP.  We default to (conventional) little-endian.
 * The big-endian and little-endian versions have different names:
   decorate the function names.  serpent.h declares both sets.
 * Provide serpentbe.c which compiles to a .o implementing the big-endian
   version under the new names.  Build only that one.
 * Change all the call sites to use the big-endian Serpent.
 * Mention this issue in the documentation.

Ultimately, no functional change in this patch.

Regarding the endianness of Serpent, Mark Wooding writes:

  I've understood how my Serpent implementation differs from Secnet's, and
  have reproduced [Ian's EAX] test vectors.

  NIST have managed to completely screw up their archive of the AES
  contest pages, but the WayBack Machine works fine -- even on the various
  PDF documents.

  The sorry tale begins with the initial Request for Candidate Algorithm
  Nominations, wherein the specification[1] for the test files unhelpfully
  muddles things by implying a big-endian representation in the test
  vector files, e.g., in 3.1,

  : Each of the possible key basis vectors is tested in this manner, by
  : shifting the "1" a single position at a time, starting at the most
  : significant (left-most) bit position of the key.

  It doesn't help that the original Serpent C implementations in the
  submission package[2] failed to implement the defined API correctly, and
  implicitly coerced BYTE pointer (from the defined entry point
  `blockEncrypt' to an `unsigned long' pointer as an argument to the
  internal `serpent_encrypt' function.  The Java version carefully picks
  words out of its input byte array in a little-endian order.

  Secnet's implementation is derived from this original reference
  implementation.  Originally, (v0.03) it had the same bug (only with
  `uint8_t' and `uint32_t'), but was patched (v0.1.16) to correct the
  obvious dependency on host endianness.  Unfortunately, this patch is
  wrong: it creates a perfectly portable completely-byte-reversed Serpent
  compatible with nothing else.  I've not found many other
  implementations, but where I have, they agree with me and the Java
  reference, and not with Secnet.


Signed-off-by: Ian Jackson <>
6 years agoserpent, transform: rework GET_32BIT_MSB_FIRST, PUT_...
Ian Jackson [Thu, 25 Jul 2013 17:30:47 +0000 (18:30 +0100)]
serpent, transform: rework GET_32BIT_MSB_FIRST, PUT_...

The macros GET_32BIT_MSB_FIRST and PUT_32BIT_MSB_FIRST duplicate
functionality available in unaligned.h, namely get_uint32 and
put_uint32.  Replace all uses outside serpent.c with calls to
get_uint32 and put_uint32.

We are going to make our serpent implementation compile-time
bytesexual, so we do retain a separate implementation of this
functionality in a form which will make that easier.  In particular,
they take arguments for the base and length of the array in
which 32-bit we are making the 32-bit access.

Also, this disentangles serpent.c from secnet.h.  To make serpent.c
easier to reuse, remove the #include of secnet.h (and replace it with
stdint.h, which we do need and were getting via secnet.h).

No functional change.

Signed-off-by: Ian Jackson <>
6 years agoserpent: const-correct
Ian Jackson [Thu, 25 Jul 2013 17:30:47 +0000 (18:30 +0100)]
serpent: const-correct

Decorate serpent_encrypt, serpent_decrypt and serpent_makekey with
appropriate consts in their arguments.

Signed-off-by: Ian Jackson <>
6 years agotransform: Do not look at any bytes of PKCS#5 padding other than the last
Ian Jackson [Thu, 25 Jul 2013 17:30:47 +0000 (18:30 +0100)]
transform: Do not look at any bytes of PKCS#5 padding other than the last

This might avoid some timing-related information leaks.  In principle
this is a protocol change: we now no longer use actual PKCS#5 padding;
instead, we use a padding scheme where all but the last byte of the
padding may be sent as anything and are ignored by the receiver.

Signed-off-by: Ian Jackson <>
6 years agomemcmp: Introduce and use consttime_memeq
Ian Jackson [Thu, 25 Jul 2013 17:30:47 +0000 (18:30 +0100)]
memcmp: Introduce and use consttime_memeq

We need to use a constant-time memcmp in MAC checking, to avoid
leaking (to an adversary) how much of the MAC is right.  (This would
be especially dangerous if our MAC was outside the encryption, which
thankfully it isn't.)

The use of "volatile" on the accumulator prevents the compiler from
optimising away any of the updates to the accumulator, each of which
depends on all the bits of the two bytes being compared.  So that
stops the compiler shortcutting the computation.  This also prevents
the compiler spotting our boolean canonicalisation, and forces it to
do it the way we say.

This is true according to the spec by virtue of C99 6.7.3(6).

In an attempt to get the compiler to eliminate the pointless repeated
loading and storing of the single-byte accumulator value, I have
specified it as "register volatile".  There is no rule against
"register volatile", but my compiler ignores the "register".

To double check that all is well, here is an annotated disassembly,
from this command line:

  gcc -save-temps -DHAVE_CONFIG_H -I. -I. -Wall -Wwrite-strings -g -O2 \
      -Werror -W -Wno-unused -Wno-pointer-sign -Wstrict-prototypes \
      -Wmissing-prototypes -Wmissing-declarations -Wnested-externs \
      -Wredundant-decls -Wpointer-arith -Wformat=2 -Winit-self \
      -Wswitch-enum -Wunused-variable -Wbad-function-cast \
      -Wno-strict-aliasing -fno-strict-aliasing -c util.c -o util.o

This is the relevant part of util.s, as generated by gcc 4.4.5-8
i486-linux-gnu, with my annotations:

  .globl consttime_memeq
  .type   consttime_memeq, @function
  .loc 1 367 0
  pushl   %ebp
  .cfi_def_cfa_offset 8
  movl    %esp, %ebp
  .cfi_offset 5, -8
  .cfi_def_cfa_register 5
  pushl   %edi
  pushl   %esi
  pushl   %ebx
  subl    $16, %esp
  .loc 1 367 0
  movl    16(%ebp), %ebx ebx : n
  .cfi_offset 3, -20
  .cfi_offset 6, -16
  .cfi_offset 7, -12
  movl    8(%ebp), %esi esi : s1in
  movl    12(%ebp), %edi edi : s2in
  .loc 1 369 0
  movb    $0, -13(%ebp) -13(ebp) : accumulator
  .loc 1 371 0
  testl   %ebx, %ebx if (!n)
  je      .L15     goto no_bytes;
      i.e. if (n) { ...loop... }

 The compiler has chosen to invent an offset variable for controlling
 the loop, rather than pointer arithmetic.  We'll call that variable
 i.  It ranges from 0..n-1 as expected.  The compiler doesn't
 explictly compute the per-iteration pointers s1 and s2, instead using
 an addressing mode.

  xorl    %edx, %edx edx : i
  .p2align 4,,7
  .p2align 3
  .L16: more_bytes: /* loop */
  .loc 1 372 0
  movzbl  (%edi,%edx), %eax eax = *s2;
  movzbl  -13(%ebp), %ecx ecx = accumulator;
  xorb    (%esi,%edx), %al al = *s1 ^ *s2;
  addl    $1, %edx      i++;
  orl     %ecx, %eax eax = accumulator | (*s1^*s2)
  .loc 1 371 0
  cmpl    %edx, %ebx [ if (i==n) ... ]
  .loc 1 372 0
  movb    %al, -13(%ebp) accumulator = eax;
           i.e., overall,
  accumulator |= *s1 ^ *s2
  .loc 1 371 0
  jne     .L16 ... [if (i!=n)]
          goto more_bytes;
  .L15: no_bytes:
  .loc 1 374 0 /* end of loop and if */

              /* now doing the shift-by-4: */
  movzbl  -13(%ebp), %eax eax = accumulator;
  movzbl  -13(%ebp), %edx edx = accumulator;
  shrb    $4, %al     al >>= 4;
  orl     %edx, %eax eax |= edx;
  movb    %al, -13(%ebp) accumulator = eax;
           i.e., overall,
  accumulator |= accumulator >> 4;
  .loc 1 375 0
  movzbl  -13(%ebp), %eax /* same again but for shift-by-2 */
  movzbl  -13(%ebp), %edx
  shrb    $2, %al
  orl     %edx, %eax
  movb    %al, -13(%ebp)
  .loc 1 376 0
  movzbl  -13(%ebp), %eax /* same again but for shift-by-1 */
  movzbl  -13(%ebp), %edx
  shrb    %al
  orl     %edx, %eax
  movb    %al, -13(%ebp) /* now computed final accumulator */
  .loc 1 377 0
  movzbl  -13(%ebp), %eax eax = accumulator;
  andl    $1, %eax    eax &= 1;
  movb    %al, -13(%ebp) accumulator = eax;
  .loc 1 378 0
  movzbl  -13(%ebp), %eax eax = accumulator;
  xorl    $1, %eax    eax ^= 1;
  movb    %al, -13(%ebp) accumulator = eax;
  .loc 1 379 0
  movzbl  -13(%ebp), %eax eax = accumulator;
  .loc 1 380 0
  addl    $16, %esp function epilogue
  popl    %ebx      ...
  popl    %esi      ...
  popl    %edi      ...
  .loc 1 379 0
  movzbl  %al, %eax sign extend?
  .loc 1 380 0
  popl    %ebp      ...
  ret    return eax;
    i.e. return accumulator;
  .size   consttime_memeq, .-consttime_memeq

Signed-off-by: Ian Jackson <>
6 years agoutil, buffers: Preparatory improvements
Ian Jackson [Thu, 25 Jul 2013 17:30:47 +0000 (18:30 +0100)]
util, buffers: Preparatory improvements

We invent a new kind of buffer_if which is a readonly view of another
block of memory; such a view can be initialised with
buffer_view_readonly or buffer_view_clone.  This makes a convenient
function to allow reparsing a packet, or using the buffer machinery to
parse a particular existing block of memory.

Also, make buffer_assert_free and buffer_assert_used actually call
assert (as well as logging the buffer ownership).  So when these fail
(a) we don't attempt the clean teardown, and (b) we get a core dump if
those are enabled.

Signed-off-by: Ian Jackson <>
6 years agounaligned.h: rationalise; provide buf_append_uint8 et al
Ian Jackson [Thu, 25 Jul 2013 17:30:46 +0000 (18:30 +0100)]
unaligned.h: rationalise; provide buf_append_uint8 et al

Replace the formulaic macros
with some macro-generated inline functions.  These have better
typechecking, and are also better because it's not possible to
accidentally mess up one of the definitions by failing to permute it
in the right way.

Add the functions for uint8 too.  This involves providing put_uint8
and get_uint8 macros, which we do along the lines of the existing put_
and get_ macros.

Use the new uint8 function in the one place where it's currently useful.
More call sites will appear shortly.

Signed-off-by: Ian Jackson <>
6 years agorsa.c: Check public key length.
Mark Wooding [Thu, 25 Jul 2013 17:30:46 +0000 (18:30 +0100)]
rsa.c: Check public key length.

The private key is checked quite carefully -- even to a fault -- for
being sensibly sized, but the corresponding function for public keys
appears to have no checking at all.  This is a shame since message-
representative construction assumes that the message representative will
fit in a fixed-size buffer.

Fix this situation by checking public key sizes in `rsapub_apply'.

Signed-off-by: Mark Wooding <>
Signed-off-by: Ian Jackson <>
6 years agorsa.c: Replace the magic length 1024 with a (larger) constant.
Mark Wooding [Thu, 25 Jul 2013 17:30:46 +0000 (18:30 +0100)]
rsa.c: Replace the magic length 1024 with a (larger) constant.

While 15360-bit RSA keys are rather large, they're not completely beyond
the realms of possibility and it seems unreasonable to forbid
them.  (Specifically, 15360 is the length recommended by NIST for
256-bit security levels.)

Signed-off-by: Mark Wooding <>
6 years agorsa.c: Factor out constructing the EMSA-PKCS1 message representative.
Mark Wooding [Thu, 25 Jul 2013 17:30:46 +0000 (18:30 +0100)]
rsa.c: Factor out constructing the EMSA-PKCS1 message representative.

This was done in two different places for no reason I could understand.
Replace them both with a single implementation.

Signed-off-by: Mark Wooding <>
6 years agorsa.c: Fix incorrect commentary.
Mark Wooding [Thu, 25 Jul 2013 17:30:45 +0000 (18:30 +0100)]
rsa.c: Fix incorrect commentary.

The Euler function phi(n) is defined to be

phi(n) = #{ 1 < i < n | gcd(i, n) = 1 }

the number of natural numbers less than n and prime to it; equivalently,
it's the size of the multiplicative group (Z/nZ)^*.

If n = p q is the product of two primes then phi(n) = (p - 1)(q - 1).
But phi(n) is not (if n is composite) the exponent of (Z/nZ)^*.  It's
certainly true that

a^{phi(n)} = 1

for all a in (Z/nZ)^*; but the exponent of a group G is the /smallest/
positive integer e such that

a^e == 1

for all a in G.  This quantity is denoted lambda(n); in our simple case
where n = p q is the product of two primes it's true that

lambda(n) = lcm(p - 1, q - 1)

Since p and q are large primes, both p - 1 and q - 1 are even, so
lambda(n) is at least a factor of 2 smaller than phi(n).

In fact, lambda(2) = 1, lambda(2^f) = 2^{f-2} for f >= 1, and
lambda(p^f) = p^{f-1} (p - 1) for prime p > 2; and, in general, if n =
p_1^{f_1} ... p_m^{f_m} is the prime factorization of n then

lambda(n) = lcm(lambda(p_1^{f_1}), ... lambda(p_m^{f_m}))

Signed-off-by: Mark Wooding <>
6 years agoslip: tolerate SLIP decoding errors.
Simon Tatham [Tue, 25 Jun 2013 17:44:38 +0000 (17:44 +0000)]
slip: tolerate SLIP decoding errors.

slip_unstuff previously responded to bad SLIP encoding or an overlong
SLIP packet by terminating secnet completely with a fatal error or
assertion failure. It turns out that SLIP encoding corruption can
occur through external action (for example, if you attach gdb to
secnet then things get a bit confused when gdb suspends it) and so
this should be a handled error condition rather than a terminal panic.

Therefore, introduce a system whereby SLIP decoding errors result in a
warning log message followed by ignoring everything in the SLIP stream
up to the next (unescaped) END byte, whereupon we resynchronise and
start decoding again.

Signed-off-by: Simon Tatham <>
6 years agotun: add hard routes even if they are currently down.
Simon Tatham [Tue, 27 Nov 2012 19:10:24 +0000 (19:10 +0000)]
tun: add hard routes even if they are currently down.

If a netlink is configured with OPT_SOFTROUTE, we expect to be adding
and removing the kernel routing table entry as the remote site appears
and disappears, so tun_set_route should bring the route up or down
depending on route->up. However, if it isn't, then we won't be doing
that during PHASE_RUN, and therefore we should bring the route up once
and for all at startup.

Previously the state was as follows:

secnet's 'tun' netlink will add and remove kernel routing table
entries during PHASE_RUN if the OPT_SOFTROUTE option is set.
However, at startup during PHASE_GETRESOURCES it will set up routes
for a site only if that site lists an address. So if you have a site
in your sites file with no address but also haven't enabled
OPT_SOFTROUTE (e.g. because you run secnet so that it drops privs),
then no route for that site will _ever_ be set up.

These two policies don't match. We should bring a site's route(s) up
at startup in any situation where we will not be prepared to do so
dynamically during run time. In other words, routes should be added
at startup time not only if they have a fixed address parameter, but
also if they do not have OPT_SOFTROUTE set.

(See also 04f92904ea6c41517ff7154910c16ef4c3bc646b, which seems to be
fixing the analogous bug for userv-ipif.  [
bdd4351ff2fc6dc8b1dad689f751ac46347636cf which seems to be the
relevant later commit is in fact empty -iwj. ]

This commit implements this fix, and causes my home secnet
implementation to be able to route to my laptop successfully
(sgo/resolution/resolution). In order to do this I've had to move the
definition of OPT_SOFTROUTE out of netlink.c into netlink.h so that
tun_set_route can see it, which suggests a possible layering
violation, but on the other hand since the netlink_client structure is
visible outside netlink.c it seems only reasonable that the bit flags
used in its 'options' field should be visible too.

Signed-off-by: Simon Tatham <>
7 years agobuild system: debian packaging improvements debian/0.3.0_beta1
Ian Jackson [Thu, 12 Jul 2012 19:27:35 +0000 (20:27 +0100)]
build system: debian packaging improvements

7 years agochangelog: describe version 0.3.0
Ian Jackson [Thu, 12 Jul 2012 19:19:12 +0000 (20:19 +0100)]
changelog: describe version 0.3.0

7 years agosite: When if our MSG5s (or peer's MSG6s) get lost, preserve the key
Ian Jackson [Thu, 21 Jun 2012 02:06:19 +0000 (03:06 +0100)]
site: When if our MSG5s (or peer's MSG6s) get lost, preserve the key

When we time out in state SENTMSG5, keep the key we negotiated.
SENTMSG5 gives the peer permission to start sending packets with it so
we need to be able to decrypt them.  If we see such packets, we switch
to using the new key at that point and throw the old key away.

This is the final fix to the "connectivity loss during final key
setup can cause locked-up session" bug.

Signed-off-by: Ian Jackson <>
7 years agosite: Keep old keys, and allow them to be used by peer
Ian Jackson [Thu, 21 Jun 2012 01:16:12 +0000 (02:16 +0100)]
site: Keep old keys, and allow them to be used by peer

After we have switched to a new key, keep the old key around until we
see packets using the new key.

This is part of the fix to the "connectivity loss during final key
setup" bug.  Fixing this requires that both ends be willing to keep
both old and new data keys available until the peer has sent data with
the new key (which might never happen).

If there were also a key setup, the site would then need three keys:
the old data key, the current data key which the peer hasn't started
using yet, and the fresh key it is trying to negotiate.  So we have to
a third key, with its own lifetime expiry.

In the code we call the old key the "auxiliary" key because we're
going to use it for an additional fixup in the next patch.

Signed-off-by: Ian Jackson <>
7 years agosite: Generalise deletion and timeout of keys
Ian Jackson [Thu, 21 Jun 2012 01:12:43 +0000 (02:12 +0100)]
site: Generalise deletion and timeout of keys

Introduce delete_one_key, and rename delete_key to delete_keys.  We
distinguish, now, between deleting a single key, and deleting all the
keys for this site.

The expiry check calls delete_one_key rather than delete_keys, and is
likewise done with a helper function.

No functional change other than to the key expiry log message.

Signed-off-by: Ian Jackson <>
7 years agosite: Move current_transform, _key_timeout and remote_session_id into a struct
Ian Jackson [Thu, 21 Jun 2012 01:28:04 +0000 (02:28 +0100)]
site: Move current_transform, _key_timeout and remote_session_id into a struct

We are going to introduce another set of these for the previous key,
and putting them together in this struct means we will be able to talk
about themm easily.

This patch is very mechanical: we move the three variables into the
new struct, and simply search-and-replace the references.

No functional change.

Signed-off-by: Ian Jackson <>
7 years agosite: No longer track key validity ourselves
Ian Jackson [Thu, 21 Jun 2012 01:23:20 +0000 (02:23 +0100)]
site: No longer track key validity ourselves

We used to have a variable st->current_valid, which we would always
set to True when assigning a key to current_transform, and to False
when calling delkey (or on initialisation).

Now that we have transform->valid(), this is no longer needed.

We do introduce a trivial helper function current_valid(st) to help us
call it, so the places where st->current_valid was checked don't get
too ugly.

There is no intentional change to the externally-visible behaviour.

Signed-off-by: Ian Jackson <>
7 years agotransform: add ->valid() function
Ian Jackson [Thu, 21 Jun 2012 01:20:39 +0000 (02:20 +0100)]
transform: add ->valid() function

This allows outside callers to find out whether the transform has been
keyed, and can therefore relieve them of the need to track this

Signed-off-by: Ian Jackson <>
7 years agosite: Deal with losing our MSG6 - retransmit MSG6 when we see MSG5 in RUN
Ian Jackson [Wed, 20 Jun 2012 22:07:34 +0000 (23:07 +0100)]
site: Deal with losing our MSG6 - retransmit MSG6 when we see MSG5 in RUN

Fix the lost MSG6 bug in the case where we are the responder:

If we are in RUN and we see a SENTMSG5 encrypted with what we now
regard as the data transfer key, conclude that the peer must have lost
the MSG6.  Regenerate and transmit a new MSG6.

This requires a generalised version of generate_msg6 which allows the
new call site to specify which transform and session id to use, and
which doesn't set st->retries.  So break the meat of generate_msg6 out
into a new function which I have chosen to call create_msg6.  (The
fact that functions called `generate_msg*' set st->retries is slightly

We also have to add a transform argument to process_msg5 so that we
can try decrypting MSG5s with the data transfer key.

It is still possible, even after this patch, to get the two ends out
of sync: if, just after the responder has switched to the new key and
entered RUN, the network fails entirely for the key setup timeout, the
initiator will abandon the key setup and switch to WAIT, and continue
to use the new key.  However, the responder has already discarded the
old key.

To fix this is not trivial: it will require us to introduce a third
key, with its own lifetime expiry.  This will be in a later patch.

Signed-off-by: Ian Jackson <>
7 years agosite: Deal with losing peer's MSG6 - go to RUN on MSG0 with new key
Ian Jackson [Thu, 14 Jun 2012 00:31:00 +0000 (01:31 +0100)]
site: Deal with losing peer's MSG6 - go to RUN on MSG0 with new key

The original protocol, as implemented, has the following bug:

The responder sends MSG6 on its transition from SENTMSG4 to RUN, on
receipt of the initiator's MSG5.  Unlike all the other key protocol
messages, MSG6 is not retransmitted (for there is no retransmission in
RUN).  If the responder's MSG6 is lost, the initiator has no way of
knowing and won't switch to the new key.  The initator will keep
retransmitting MSG5, which the responder (in old versions of secnet)
ignores.  The responder will be sending data with the new key, which
the initiator (in old versions of secnet) ignores.  This broken
situation can persist until the initiator sends data, which it will do
with the old key, possibly causing a new key exchange attempt to be
initiated by the original responder (in old versions of secnet only),
which may or may not experience the same bug.

This bug needs to be fixed separately at both ends, ideally, so that
running a fixed version at either end avoids the bug.

Here we fix for the case where we are the initiator, as follows:

If are in SENTMSG5 and we see data encrypted with the new key,
conclude that we must have lost the MSG6 and simply switch to the new
key, and state RUN, right away.

Because the transform operates destructively (and changing that fact
about the transform API would be too intrusive), this means we need to
keep a copy of the original ciphertext in process_msg0.

For this purpose we introduce a new buffer `scratch' in the site state
structure, and a new buffer_copy utility function for making a copy of
a buffer (reallocating the destination buffer if necessary).

Signed-off-by: Ian Jackson <>
7 years agosite, transform: Do not initiate rekey when packets too much out of order
Ian Jackson [Wed, 20 Jun 2012 20:24:26 +0000 (21:24 +0100)]
site, transform: Do not initiate rekey when packets too much out of order

If packets arrive sufficiently out of order, we may unnecessarily
initiate a rekey simply because of the skew.  Solve this as follows:
 * transform->reverse gives a distinct error code for `too much skew'.
 * site does not initiate a rekey when it sees that error code.

The rekey-on-decrypt-failure feature is in principle unnecessary.
However, due to another bug it is possible for a key setup to be
regarded as successful at only one of the two ends, and the ability to
immediately do another key setup to try to fix things is useful.

In principle this new behaviour might prevent a necessary rekey: if
the sender has sent around 2^31 packets none of which were received,
the receiver would see the new packets as too old.  But: firstly, this
is very unlikely (it would have involved transmitting several Gbytes
into the void, in a period less than the maximum key lifetime).
Secondly, we ought to expire keys before then anyway so the sender
should know that the key had `worn out' (although currently this is
not enforced).

The benefit of this change is that the old behaviour would be likely
to initiate unnecessary rekeys when on unreliable links (eg, mobile
internet).  Unnecessary rekeys are a bad idea in these circumstances
not just because they clog up the link but also because they make the
association more fragile.

Signed-off-by: Ian Jackson <>
7 years agosite: Remove pointless check from decrypt_msg0
Ian Jackson [Wed, 20 Jun 2012 23:23:13 +0000 (00:23 +0100)]
site: Remove pointless check from decrypt_msg0

It is not necessary to check whether we have a current key before
attempting to unpick and decrypt a MSG0.  If we don't,
transform->reverse will simply fail due to lacking a key.

This check needs to be removed, because in forthcoming patches we will
want to be able to try to decrypt the packet with other keys, at which
point the lack of a currently valid data transfer key will not be
relevant and this check will be harmful.

Signed-off-by: Ian Jackson <>
7 years agosite: Break out separate function for decrypting msg0
Ian Jackson [Wed, 20 Jun 2012 22:38:14 +0000 (23:38 +0100)]
site: Break out separate function for decrypting msg0

The control flow here is going to become more complicated, and this
change will make the next patches, and the resulting code, clearer.

Note that process_msg0's return value is never used; it is only
defined to return bool_t so that it can use the CHECK_AVAIL macro.
Knowing this will make it easier to see that the patch is correct.

Signed-off-by: Ian Jackson <>
7 years agonetlink: abolish check_config and output_config
Ian Jackson [Wed, 13 Jun 2012 20:06:02 +0000 (21:06 +0100)]
netlink: abolish check_config and output_config

Apparently, a long time ago, MSG5 and MSG6 used to contain some
netlink configuration data, which the receiver of the MSG5 or MSG6
would check.

However, for a long time now the output_config function has been a
no-op and the check function has unconditionally eaten and discarded
anything extra in the message.

Furthermore, because the MSG6 is not retransmitted, this mechanism
couldn't be reliable without a protocol change.  So the existing
interface is defective.

So, abolish it the interface, the dummy implementation, and all the
call sites.  The check_config call sites in site.c now instead
directly discard any unexpected data at the end of MSG5 and MSG6.

This patch should cause no behavioural change in actual operation.

Signed-off-by: Ian Jackson <>
7 years agonetlink: report why a packet is bad
Ian Jackson [Sun, 25 Dec 2011 19:01:38 +0000 (19:01 +0000)]
netlink: report why a packet is bad

 <netlink>: bad IP packet from <source>
 <netlink>: bad IP packet from <source>: reason

Signed-off-by: Ian Jackson <>
7 years agosite: transport peers: fix incorrect stride when debug output enabled
Ian Jackson [Thu, 21 Jun 2012 00:24:41 +0000 (01:24 +0100)]
site: transport peers: fix incorrect stride when debug output enabled

When there are multiple peer addresses, attempts to copy them from one
table of peers addresses to another with transport_peers_copy will go
wrong because the stride argument to transport_peers_debug is wrong -
we take sizeof() the pointer rather than of the array element.

This will typically cause a segfault if it happens, but the bug can
only be triggered if LOG_PEER_ADDRS debugging is enabled.

Signed-off-by: Ian Jackson <>
7 years agomessages: add some missing newlines
Ian Jackson [Wed, 13 Jun 2012 23:44:50 +0000 (00:44 +0100)]
messages: add some missing newlines

Message and cfgfatal must be called with a message containing a
newline (or, a newline sent in a later call).  Fix a few call sites.

Signed-off-by: Ian Jackson <>
7 years agolog: Print truncated messages
Ian Jackson [Wed, 13 Jun 2012 23:37:16 +0000 (00:37 +0100)]
log: Print truncated messages

If a message doesn't survive the vsnprintf in vMessage untruncated,
the result may be that its newline is lost.  After that, buff will
never manage to gain a newline and all further messages will be

Fix this by always putting "\n\0" at the end of buff.  That way a
truncated message is printed and the buffer will be cleared out for
the next call.

This does mean that sometimes a message which is just slightly shorter
than the buffer will produce a spurious empty message sent to the
syslog.  However, the whole logging approach here will inevitably
occasionally insert spurious newlines, if only to split up long
messages; long messages may also be truncated.  The bad effect of the
change in this patch is simply to make the length at which
infelicities appear very slightly shorter.  The good effect is that it
turns a very nasty failure mode into a benign one.

So I think it is reasonable to err on the side of code which clearly
cannot go wrong in a bad way.  Other approaches which make the
infelicity threshold a couple of characters longer, or which slightly
reduce the different kinds of infelicity, are much more complicated
and therefore more likely to have bugs.

Signed-off-by: Ian Jackson <>
7 years agolog: Eliminate potential out-of-control recursion
Ian Jackson [Wed, 13 Jun 2012 23:36:15 +0000 (00:36 +0100)]
log: Eliminate potential out-of-control recursion

vMessage looks for the system log instance.  If there is one, it uses
it; otherwise it just uses stderr or stdout.

The system log instance consists, eventually, of syslog_vlog or
logfile_vlog.  Both of these functions have a fallback: if they are
properly set up they do their thing; otherwise they call vMessage.
This is wrong because in this situation they have probably been called
by vMessage so it doesn't help.

At first sight this ought to produce unbounded recursion but for
complicated reasons another bug prevents this.  Instead, messages can
just vanish.

Break out the fallback mode of [v]Message into [v]MessageFallback so
that syslog_vlog and logfile_vlog can use it directly.

Signed-off-by: Ian Jackson <>
7 years agoMakefile: honour EXTRA_CFLAGS, etc.
Ian Jackson [Wed, 13 Jun 2012 22:47:16 +0000 (23:47 +0100)]
Makefile: honour EXTRA_CFLAGS, etc.

This makes it easier to add command-line options at build-time.

Signed-off-by: Ian Jackson <>
7 years agoSECURITY: actually reject messages with improper lengths
Ian Jackson [Wed, 13 Jun 2012 20:42:32 +0000 (21:42 +0100)]
SECURITY: actually reject messages with improper lengths

transform_reverse checks to see if the incoming message is a multiple
of the block cipher block size.  If the message fails this check, it
sets *errmsg but it fails to return.  Instead, it proceeds to attempt
to decrypt it.  This will involve a buffer read/write overrun.

It transform_reverse fails to check to see if the incoming message is
long enough to contain the IV, MAC and padding.  This can easily be
exploited to cause secnet to segfault.

buffer_unprepend should check that the buffer has enough data in it to
unprepend.  With the other patches, this should be irrelevant, but it
turns various other potential read overrun bugs into crashes.

site_incoming should check that the incoming message is long enough.
Again, without this, this is an buffer read overrun or possibly a

Signed-off-by: Ian Jackson <>
7 years agouserv-ipif: Always request routes from userv, regardless of link quality
Ian Jackson [Thu, 12 Jul 2012 18:57:22 +0000 (19:57 +0100)]
userv-ipif: Always request routes from userv, regardless of link quality

Previously the userv-ipif netlink would not request (from userv) a
route for a site for which the link quality was DOWN.  The link
quality is a dynamic quantity but userv-ipif lacks any machinery for
dynamically adding routes, so this is wrong.

Instead, in userv-ipif, unconditionally add routes for all sites,
regardless of link up status.

In practice this code is run during startup and the only reason a link
might be down at that point, ie LINK_QUALITY_DOWN, is that it does not
have an address configured.  Mobile sites are often in this situation.

Signed-off-by: Ian Jackson <>
7 years agobuild system: Update build-dependency information
Matthew Vernon [Thu, 12 Jul 2012 18:42:36 +0000 (19:42 +0100)]
build system: Update build-dependency information

Update build-dependency documentation and add missing references to
flex and bison to debian/control.

Signed-off-by: Matthew Vernon <>
Signed-off-by: Ian Jackson <>
7 years agomake-secnet-sites: Do not permit "include" in simple sites files
Ian Jackson [Wed, 11 Jul 2012 00:00:17 +0000 (01:00 +0100)]
make-secnet-sites: Do not permit "include" in simple sites files

Restrict the "include" directive to the "header" of -u (groupfile
update) mode.  Callers who are simply using make-secnet-sites to
transform a (possibly untrusted) sites file into a (to be trusted)
sites.conf file should not have to worry about includes.

"include" directives are already forbidden in group files.

Signed-off-by: Ian Jackson <>
7 years agomake-secnet-sites: In -u mode, output file "dereferences" include directives
Ian Jackson [Tue, 10 Jul 2012 23:58:12 +0000 (00:58 +0100)]
make-secnet-sites: In -u mode, output file "dereferences" include directives

Whenm make-secnet-sites is writing the "output" sites file in -u
(groupfile update) mode, it includes the effective contents of files
referenced in "include" directives, rather than the "include"
directive itself.

So the "output" sites file does not any longer depend on any files
included by the header.

Signed-off-by: Ian Jackson <>
7 years agomake-secnet-sites: Do newline-trimming in pline()
Ian Jackson [Tue, 10 Jul 2012 23:56:50 +0000 (00:56 +0100)]
make-secnet-sites: Do newline-trimming in pline()

This will make life a little easier in a future patch.  While we're at
it, use rstrip rather than open-coding it.

Signed-off-by: Ian Jackson <>
7 years agomake-secnet-sites: Actually include addresses in sites.conf
Ian Jackson [Tue, 10 Jan 2012 11:50:25 +0000 (11:50 +0000)]
make-secnet-sites: Actually include addresses in sites.conf

This was broken in

   commit c9d84e31f48a04eb0af525324707f22c738cc8b7
   Author: Ian Jackson <>
   Date:   Sat Dec 17 21:21:47 2011 +0000

       make-secnet-sites: Allow sites with no address

Signed-off-by: Ian Jackson <>
7 years agomake-secnet-sites: If definition found in wrong place, bomb out
Ian Jackson [Sat, 17 Dec 2011 21:38:02 +0000 (21:38 +0000)]
make-secnet-sites: If definition found in wrong place, bomb out

If it doesn't exit here, make-secnet-sites might subsequently crash
with Python stack backtrace.

Signed-off-by: Ian Jackson <>
7 years agomake-secnet-sites: New -P <prefix> option
Ian Jackson [Sat, 17 Dec 2011 21:36:07 +0000 (21:36 +0000)]
make-secnet-sites: New -P <prefix> option

make-secnet-sites generates a config file which defines the keys
"vpn-data", "vpn", and "all-sites".

To make it possible to usefully include the output of more than one
different piece of output from make-secnet-sites, support the option
-P <prefix>, which generates a config file which defines
"<prefix>vpn-data", "<prefix>vpn", and "<prefix>all-sites".

Signed-off-by: Ian Jackson <>
7 years agomake-secnet-sites: Allow sites with no address
Ian Jackson [Sat, 17 Dec 2011 21:21:47 +0000 (21:21 +0000)]
make-secnet-sites: Allow sites with no address

Signed-off-by: Ian Jackson <>
7 years agomake-secnet-sites: Fix userv invocation after pfilepath
Ian Jackson [Sat, 17 Dec 2011 21:39:39 +0000 (21:39 +0000)]
make-secnet-sites: Fix userv invocation after pfilepath

The commit 9b8369e07aeba5ed2c69fb4a7f74d07c8cebe015
 make-secnet-sites: refactor to break out new function "pfilepath"
broke the userv service invocation, because it turned out that later
code depended on the "headerinput" variable whose assignment had been
removed and replaced by a call to pfilepath.

Make pfilepath return the lines read from the file and assign the
result to headerinput.

Signed-off-by: Ian Jackson <>
7 years agonetlink: Fix up link down behaviour
Ian Jackson [Sat, 17 Dec 2011 21:24:35 +0000 (21:24 +0000)]
netlink: Fix up link down behaviour

Partially reverts 04f92904ea6c41517ff7154910c16ef4c3bc646b
 "userv-ipif: Always request routes from userv, regardless of link quality"

It turns out that this check is necessary to avoid bringing up a route
for a "netlink" stanza in the configuration file which is never used.

In particular, this avoids bringing up a netlink for (a) sites which
are not mentioned in the config file (b) the site on which secnet is

Signed-off-by: Ian Jackson <>
7 years agouserv-ipif: Always request routes from userv, regardless of link quality
Ian Jackson [Thu, 15 Dec 2011 01:11:24 +0000 (01:11 +0000)]
userv-ipif: Always request routes from userv, regardless of link quality

Previously the userv-ipif netlink would not request (from userv) a
route for a site for which the link quality was DOWN.  The link
quality is a dynamic quantity but userv-ipif lacks any machinery for
dynamically adding routes, so this is wrong.

Instead, in userv-ipif, unconditionally add routes for all sites,
regardless of link up status.

In practice this code is run during startup and the only reason a link
might be down at that point, ie LINK_QUALITY_DOWN, is that it does not
have an address configured.  Mobile sites are often in this situation.

Signed-off-by: Ian Jackson <>
7 years agomake-secnet-sites: new "include" keyword
Ian Jackson [Thu, 15 Dec 2011 01:01:38 +0000 (01:01 +0000)]
make-secnet-sites: new "include" keyword

Allow "headers" files and "sites" files to contain "include"

Signed-off-by: Ian Jackson <>
7 years agomake-secnet-sites: refactor to break out new function "pfilepath"
Ian Jackson [Thu, 15 Dec 2011 00:57:51 +0000 (00:57 +0000)]
make-secnet-sites: refactor to break out new function "pfilepath"

No intentional functional change.

Signed-off-by: Ian Jackson <>
7 years agobuild system: More release checklist updates
Ian Jackson [Sun, 11 Dec 2011 14:06:20 +0000 (14:06 +0000)]
build system: More release checklist updates

Need to "make dist" again.

Signed-off-by: Ian Jackson <>
7 years agoUpdate VERSION v0.2.1
Ian Jackson [Sun, 11 Dec 2011 13:20:11 +0000 (13:20 +0000)]

7 years agobuild system: Fix Ian's email address in debian/changelog
Ian Jackson [Sun, 11 Dec 2011 13:15:37 +0000 (13:15 +0000)]
build system: Fix Ian's email address in debian/changelog

Signed-off-by: Ian Jackson <>
7 years agoauthbind: get endianness right (again)
Ian Jackson [Sun, 11 Dec 2011 12:40:37 +0000 (12:40 +0000)]
authbind: get endianness right (again)

It appears that:

 * authbind's documentation authbind-helper(8) describes the
   endianness convention of authbind's helper program incorrectly.
   See Debian #651694.

 * The version of secnet 0.1.16 tagged as such in revision control
   contains a "fix" which was based on the authbind documentation but
   not apparently tested against authbind.  Ie, this part from NEWS:
    4) Change the endianess of the arguments to authbind-helper.
       sprintf("%04X") already translates from machine repesentation to most
       significant octet first so htons reversed it again.

 * The version of secnet 0.1.16 actually in service on chiark had an
   out-of-version-control change to udp.c to make it work with
   chiark's authbind 1.2.0.  The actual code found has been recorded
   on the dead branch "chiark-0.1.16" in the master git repo, but the
   version of udp.c is exactly that from 0.1.15 so it looks like we
   just reverted to the previous udp.c during deployment of 0.1.16.

 * We (re)discovered all this after the release of secnet 0.2.0
   because my attempt to deploy 0.2.0 on chiark was not actually

Therefore, undo the authbind endianness change introduced in secnet
0.1.16.  This is most easily achieved by constructing the arguments to
the helper from the sockaddr rather than the contents of "st".

Thanks are due to Simon Tatham for the bug report.

Signed-off-by: Ian Jackson <>
7 years agoUpdate VERSION v0.2.0
Ian Jackson [Sat, 10 Dec 2011 22:48:05 +0000 (22:48 +0000)]

7 years agobuild system: Further improvements to release checklist
Ian Jackson [Sat, 10 Dec 2011 22:43:25 +0000 (22:43 +0000)]
build system: Further improvements to release checklist

We have to:
 * Update debian/changelog
 * Build a new .deb
 * Copy the files to chiark:~secnet/ in case anyone looks there

Signed-off-by: Ian Jackson <>
7 years agoNew --managed option for use when running under a daemon supervisor.
Richard Kettlewell [Sat, 10 Dec 2011 22:35:47 +0000 (22:35 +0000)]
New --managed option for use when running under a daemon supervisor.

The effect is secnet is told that it is running as a daemon right from
the start, so it knows to follow the logging rules for daemons but not
to fork.

The conflation of daemonization with dropping privilege is also
unpicked by this patch.  Most importantly this ensures that errors
from PHASE_GETRESOURCES operations such as 'route' commands is sent to
the logfile (or syslog).

Signed-off-by: Richard Kettlewell <>
7 years agoMake the list of phases be an enumeration.
Richard Kettlewell [Sat, 10 Dec 2011 22:35:47 +0000 (22:35 +0000)]
Make the list of phases be an enumeration.

Signed-off-by: Richard Kettlewell <>
7 years agoSet group ID and group list.
Richard Kettlewell [Sat, 10 Dec 2011 22:35:47 +0000 (22:35 +0000)]
Set group ID and group list.

More sensible username lookup.

Signed-off-by: Richard Kettlewell <>
7 years agobuild system: Include signing of tarballs in release checklist
Ian Jackson [Sat, 10 Dec 2011 22:22:21 +0000 (22:22 +0000)]
build system: Include signing of tarballs in release checklist

Signed-off-by: Ian Jackson <>
7 years agobuild system: New "make dist" uses git; add release checklist
Ian Jackson [Sat, 10 Dec 2011 21:56:05 +0000 (21:56 +0000)]
build system: New "make dist" uses git; add release checklist

We no longer try to separately track which files should be shipped in
the tarball.  Instead, the tarball contains exactly the same files as
the repo.

Add a release checklist at the bottom of, based on the one
from the stable branch (in 813ff677f7020b12fb1356e977a73dc910e64ce5).

Signed-off-by: Ian Jackson <>
7 years agomd5: correct size arg to memset().
Richard Kettlewell [Sat, 10 Dec 2011 16:13:18 +0000 (16:13 +0000)]
md5: correct size arg to memset().

On realistic targets the effect of the error was that the MD5 context
wasn't fully cleared.

Signed-off-by: Richard Kettlewell <>
7 years agocleanup: build on Ubuntu Lucid
Richard Kettlewell [Sat, 10 Dec 2011 16:09:13 +0000 (16:09 +0000)]
cleanup: build on Ubuntu Lucid

The discard() idiom is chosen because Clang tolerates it.

Signed-off-by: Richard Kettlewell <>
7 years agoAdd a man page.
Richard Kettlewell [Sat, 10 Dec 2011 12:25:43 +0000 (12:25 +0000)]
Add a man page.

As it stands it is purely reference material - no conceptual or
tutorial stuff whatsoever.

Signed-off-by: Richard Kettlewell <>
7 years agoRemove snprintf reimplementation.
Richard Kettlewell [Sat, 10 Dec 2011 12:15:08 +0000 (12:15 +0000)]
Remove snprintf reimplementation.

We don't want to think about the "Frontier Artistic License" and the
requirements it may or may not impose on distribution of secnet.

Signed-off-by: Richard Kettlewell <>
7 years agoConfig file fixes.
Richard Kettlewell [Mon, 11 Jul 2011 19:02:31 +0000 (20:02 +0100)]
Config file fixes.

* Reject integers in excess of 2^32-1 (rather than reducing them mod
* ptree_dump():
  - Remove a magic number.
  - More realistic recursion limit.
* Various bits of type hygeine.

Signed-off-by: Richard Kettlewell <>
7 years agoDetect Fink automatically.
Richard Kettlewell [Mon, 11 Jul 2011 19:02:26 +0000 (20:02 +0100)]
Detect Fink automatically.

The logic is that if Fink is in the path then we should use it.  So
(unlike certain build systems I could mention) if you want to use a
local install of GMP, it should be sufficient to run ./configure
without /sw/bin in the path.

Signed-off-by: Richard Kettlewell <>
7 years agoOSX: fix home directory in setup.mac.
Richard Kettlewell [Sat, 10 Dec 2011 11:56:55 +0000 (11:56 +0000)]
OSX: fix home directory in setup.mac.

Signed-off-by: Richard Kettlewell <>
8 years agoMobile sites: Update test example
Ian Jackson [Wed, 20 Jul 2011 18:54:22 +0000 (19:54 +0100)]
Mobile sites: Update test example

Make the "inside" site in test-example be mobile.

Signed-off-by: Ian Jackson <>
8 years agoMultiple udp ports: Add to test
Ian Jackson [Wed, 20 Jul 2011 18:03:57 +0000 (19:03 +0100)]
Multiple udp ports: Add to test

For testing, add an extra port to one of the sites in test-example.

Signed-off-by: Ian Jackson <>