Secnet progress

Sat Sep 28 16:27:01 BST 2019

Ian Jackson <ijackson at chiark.greenend.org.uk> writes:

> If you don't ever intend to go through my previous comments then
> please say and I will do it.  I think it will save review work to be
> able to see what I wrote before for the same patch.  Until then I am
> trying to hold off reviewing your latest resubmissions...

I totally intended to go back over your previous comments.  And, indeed,
I have now done this!  Yay!

It turned out to be easier to dig into the list archives (because the
articles had expired from the newsserver :-/).

  https://www.chiark.greenend.org.uk/pipermail/sgo-software-discuss/2017/thread.html

  * The `ECDH, early capabilities, etc.' discussion ended with a
    resolution to make new DH group elements on the wire be plain
    binary.  I did that, though maybe the buffer-protocol work
    (discussed below) will change how that works.

  * [PATCH 02/43] discussion was about archaeology.

  * [PATCH 24/43], [PATCH 25/43] is about the code dump from Catacomb.
    I'm now planning to do that in a mostly automated manner, which
    hopefully will address all of your concerns.

    Hmm.  My new script doesn't currently report a URL for the donor
    repository.  I'm not sure how it would guess one.  I could hardcode
    one, but that doesn't seem very useful.

  * [PATCH 33/43] is on structuring the message codes, of which more
    below.

  * [PATCH 38/43] is about making early capability bits dependent on the
    bit's meaning, rather than just which bit number it is.  I did that,
    and you've taken the patch.  The discussion ends up being about
    capability negotiation, during which it seems that I became rather
    tetchy (so sorry about that), and then realised that we don't need
    any early caps after all.

  * [PATCH 42/43] ended up stuck in discussion about whether capability
    bits needed to be early (spoiler alert: none of them actually need
    to be early).  I believe that the use of the capability system in
    mdw/xdh accords with our final thinking.

So, err, I think I actually did nearl everything except for the import
back in the day, and the import has been totally overhauled.

The missing piece I think is replacing the capability augmentation in
`process_msg3' with a plain assignment, per

  https://www.chiark.greenend.org.uk/pipermail/sgo-software-discuss/2017/000501.html

Something like this?  (Also pushed to mdw/springclean.  Did I get the
style right this time?)


commit 0aae8f7a1935b589258a52c4728a434d87537da5
Author: Mark Wooding <mdw at distorted.org.uk>
Date:   Sat, 28 Sep 2019 16:13:45 +0100

    site: Replace remote's caps after verifying MSG3

    Previously we'd just `or' the new capability bits into
    `st->remote_capabilities' prior to verification, with the rather
    unfortunate result that an adversary could convince us that the remote
    site has features which it doesn't, in fact, implement.  At present, the
    worst effect here is preventing key-exchange from completion, but it's
    imaginable that future capability bits have worse effects.

    Instead, (a) simply replace our idea of the remote site's capabilities
    rather than accumulating a union of all mentioned capabilities, and (b)
    do this /after/ verifying the signature on the message.  (This is safe
    because there's no mention of `st->remote_capabilities' in the
    intervening code in `process_msg3', or in the common message-
    verification code in `process_msg3_msg4'.)

    Signed-off-by: Mark Wooding <mdw at distorted.org.uk>

diff --git a/site.c b/site.c
index 3b8f34d..edf4c50 100644
--- a/site.c
+++ b/site.c
@@ -913,7 +913,6 @@ static bool_t process_msg3(struct site *st, struct buffer_if *msg3,
 	     capab_adv_late, st->remote_capabilities, m.remote_capabilities);
 	return False;
     }
-    st->remote_capabilities|=m.remote_capabilities;

 #define CHOSE_CRYPTO(kind, what) do {					\
     struct kind##_if *iface;						\
@@ -937,6 +936,16 @@ kind##_found:								\
     if (!process_msg3_msg4(st,&m))
 	return False;

+    /* Update our idea of the remote site's capabilities, now that we've
+     * verified that its message was authentic.
+     *
+     * Our previous idea of the remote site's capabilities came from the
+     * unauthenticated MSG1.  We've already checked that this new message
+     * doesn't change any of the bits we relied upon in the past, but it may
+     * also have set additional capability bits.  We simply throw those away
+     * now, and use the authentic capabilities from this MSG3. */
+    st->remote_capabilities=m.remote_capabilities;
+
     /* Terminate their DH public key with a '0' */
     m.pk[m.pklen]=0;
     /* Invent our DH secret key */


> > `mdw/springclean' contains some miscellaneous fixes, a Wireshark
> > dissector, and a reorganization of the capability-bit handling.
> > This is stuff that my previous XDH work built on, but disentangled
> > from that.
>
> Thanks.  After review, I have pulled this into master.  I did have
> some issues with it but I had to decide whether it would be a good
> idea to go around another iteration with you.  I decided it would be
> better to try to do myself whatever needed doing.  Particularly
> because there's some stuff there which I wanted for my pubkey work and
> I wanted to unblock myself.
>
> This resulted in some further commits 5d903ef..ce32dd8 which I think
> will contain one trivial semantic conflict with your xdh branch (which
> will be detected by the compiler).
>
> I wanted to make some specific observations in the hope of making
> our collaboration easier:
>
>  * Thank you very much for the capability-related rework of which
>    I thoroughly approve.
>

>  * "magic.h: Present message labels as an encoding of major
>     and minor numbers."
>    The motivation for this commit is not presented.  This is not
>    really ideal for review of a complex new idea as part of a "spring
>    clean" series.  I nearly decided to drop this patch from my push
>    for this reason but ultimately I chose to take it on faith.

Fair point.  The patch was motivated by the introduction (in the
`mdw/xdh' series) of `MSG3TER', which is kind of like `MSG3BIS' but with
even more things in it.  If I just left the current ad-hoc message-code
dispatch machinery in `site.c' unchanged, I'd end up with several
ever-growing lists of MSG3-like message codes.  My thought was that
making the message-code more structured would let that structure shine
through into the dispatch code.

Hmm.  This probably makes more sense with the `Prepare for adding more
MSG3 variants' patch, which I neglected to contribute.  That change
applied cleanly onto mdw/springclean.

See also

  https://www.chiark.greenend.org.uk/pipermail/sgo-software-discuss/2017/000483.html

and subsequent discussion.

>  * "site.c: Abstract out the various parts of capability handling."
>    suffers slightly from the same problem, although I can guess why
>    something like this might be wanted.

As you no doubt guessed, that's all about preparing to add another thing
which works like transform negotiation but isn't that, without
duplicating a pile of code.  If you're adding public-key types then that
might have wanted a similar mechanism.

>  * "secnet-wireshark.lua: Add a Wireshark dissector."
>    Should something install this somewhere ?

Probably.  But it turns out that this is /really hard/, and breaks all
the time.  My cut at the necessary Autoconf machinery is

  https://git.distorted.org.uk/~mdw/tripe/blob/HEAD:/configure.ac#l218

Alternatively, an individual user can copy or link the thing into
~/.local/lib/wireshark/plugins/ (yes!  I didn't previously know that
~/.local/lib/ was a thing), but that's not something that a build script
should be doing.

>  * This list is in addition to the changes I made myself in
>    5d903ef..ce32dd8, which I'd appreciate it if you looked at.

Err, upstream doesn't seem to have moved.  What's going on?  Oh!  It's
in `Ian's personal secret secnet repo'.

  * %option yynowrap -- Huh.  I didn't know that was a thing.  I'd have
    gone with defining the function rather than using a flex-specific
    option, but either way is fine.

  * style things: Noted for future reference.  Sorry.

  * gitignore: Ah!  Fair point.  I expect that I'll produce more of
    these as time goes on.  Consider it the downside of me testing that
    your out-of-tree build machinery works. :-)

>  * I write commit message titles without trailinng full stops.  This
>    allows one additional character of useful content and IMO makes
>    quoted commit message titles look less odd.  I decided not to
>    rewrite your branch for this tiny bikeshed :-).

Noted.  I shall endeavour to following this convention.

> > `mdw/xdh' contains the rest of my XDH work, and is where I'm currently
> > working.  It starts by refactoring the DH closure interface, then moves
> > onto DH group negotiation, and then adds Bernstein's X25519 and X448.
>
> I have not looked at this at all for the reason I give above.  Please
> let me know whether you think I should review it all de novo, or go
> back to my mails from before, or wait, or what.

Sure.  I think it's probably best if I try to get a tidied up version
ready for you before sunrise on Sunday.

-- [mdw]

