> Date: Tue, 13 Mar 2018 13:49:01 +0100
> From: Peter Hessler <phess...@openbsd.org>
> 
> 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.

I think there is a clear benefit to match what's written in the RFC
since that makes understanding the code easier for people who are new
to it.

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