On 2018 Mar 13 (Tue) at 12:41:17 +0100 (+0100), Florian Obser wrote:
:(sending this to tech@ so that more people see this)
:
:semarie@ pointed out on bugs@ (
:https://marc.info/?l=openbsd-bugs&m=152084960013726&w=2 ) that his
:RFC7217 IPv6 address changed after an upgrade. Of course it should not.
:
:The reason for that was a mistake in the original implementation:
:
:In the original implemntation the bits of the sha512 hash and
:the IPv6 address aligned like this:
:
:         511         447   383       127       63          0
:        +-+-[...]-+-+-[...]-+-+-[...]-+-+-[...]-+-+-[...]-+-+
:        |               512 bit sha512 digest               |
:        +-+-[...]-+-+-[...]-+-+-[...]-+-+-[...]-+-+-[...]-+-+
:
: 127     63          0 
:+-+-[...]-+-+-[...]-+-+
:|  IPv6 address       |
:+-+-[...]-+-+-[...]-+-+
:
:
:after phessler's change to support non-64 prefix lenght (rev1.22 of
:engine.c):
:
: 511     447       383       127       63          0
:+-+-[...]-+-+-[...]-+-+-[...]-+-+-[...]-+-+-[...]-+-+
:|               512 bit sha512 digest               |
:+-+-[...]-+-+-[...]-+-+-[...]-+-+-[...]-+-+-[...]-+-+
:
: 127     63        0 
:+-+-[...]-+-+-[...]-+
:|  IPv6 address     |
:+-+-[...]-+-+-[...]-+
:
:So even with a /64 the bits are shifted and you end up with a
:different v6 address. This is what semarie observed.
:
:
:In section 5, page 9 RFC 7217 states:
:
:   2.  The Interface Identifier is finally obtained by taking as many
:       bits from the RID value (computed in the previous step) as
:       necessary, starting from the least significant bit.
:
:So it should have looked like this:
:
: 511     447       383       127       63          0
:+-+-[...]-+-+-[...]-+-+-[...]-+-+-[...]-+-+-[...]-+-+
:|               512 bit sha512 digest               |
:+-+-[...]-+-+-[...]-+-+-[...]-+-+-[...]-+-+-[...]-+-+
:
:                             127       63          0 
:                            +-+-+-[...]-+-+-[...]-+-+
:                            |  IPv6 address         |
:                            +-+-+-[...]-+-+-[...]-+-+
:
:
:I think we should implement what the RFC says. Unfortunately that
:means addresses change once more, but things change in current... I
:will put some wording into current.html. Having this churn now while
:it's a newly introduced feature is better then in two years time if we
:discover that we should really implement what the rfc says.
:
:OK?
:
:If you realy hate this and have a good reason why we should stick with
:the current algorithm speak up now, I'm going to commit this soonish
:to not drag out the address change for too long. Also this needs to
:make 6.3.
:

I don't see the point in mandating which part of the hash we use.  The
RFC allows us to use any algorithm to generate the stable identifier, so
its not expected to be portable to another implementation.  However, if
there is a cryptographic reason to prefer the least significant bits
instead of the most significatnt bits, I'm completely in support.  But as
I understand it, random data is random, *shrug*.

That being said, while I don't like shifting this around I'm not opposed
to it.

If you want to commit it, OK.


:diff --git sbin/slaacd/engine.c sbin/slaacd/engine.c
:index f473e3d0b80..e41a7c31751 100644
:--- sbin/slaacd/engine.c
:+++ sbin/slaacd/engine.c
:@@ -1239,6 +1239,8 @@ gen_addr(struct slaacd_iface *iface, struct radv_prefix 
*prefix, struct
:       int dad_counter = 0; /* XXX not used */
:       u_int8_t digest[SHA512_DIGEST_LENGTH];
: 
:+      memset(&iid, 0, sizeof(iid));
:+
:       /* from in6_ifadd() in nd6_rtr.c */
:       /* XXX from in6.h, guarded by #ifdef _KERNEL   XXX nonstandard */
: #define s6_addr32 __u6_addr.__u6_addr32
:@@ -1275,7 +1277,8 @@ gen_addr(struct slaacd_iface *iface, struct radv_prefix 
*prefix, struct
:                   sizeof(addr_proposal->soiikey));
:               SHA512Final(digest, &ctx);
: 
:-              memcpy(&iid.s6_addr, digest, sizeof(iid.s6_addr));
:+              memcpy(&iid.s6_addr, digest + (sizeof(digest) -
:+                  sizeof(iid.s6_addr)), sizeof(iid.s6_addr));
:       } else {
:               /* This is safe, because we have a 64 prefix len */
:               memcpy(&iid.s6_addr, &iface->ll_address.sin6_addr,
:diff --git sys/netinet6/in6_ifattach.c sys/netinet6/in6_ifattach.c
:index 0aa10fad94b..e2a4ab1dd92 100644
:--- sys/netinet6/in6_ifattach.c
:+++ sys/netinet6/in6_ifattach.c
:@@ -244,7 +244,7 @@ in6_get_soii_ifid(struct ifnet *ifp, struct in6_addr *in6)
:       SHA512Update(&ctx, ip6_soiikey, sizeof(ip6_soiikey));
:       SHA512Final(digest, &ctx);
: 
:-      bcopy(digest, &in6->s6_addr[8], 8);
:+      bcopy(digest + (sizeof(digest) - 8), &in6->s6_addr[8], 8);
: 
:       return 0;
: }


-- 
The revolution will not be televised.

Reply via email to