Re: correctly calculate RFC7217 based IPv6 address
> Date: Tue, 13 Mar 2018 13:49:01 +0100
> From: Peter Hessler
>
> 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 630
> :+-+-[...]-+-+-[...]-+
> :| 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));
> : SHA512Fina
Re: correctly calculate RFC7217 based IPv6 address
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 630
:+-+-[...]-+-+-[...]-+
:| 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.
correctly calculate RFC7217 based IPv6 address
(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 630
+-+-[...]-+-+-[...]-+
| 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.
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;
}
--
I'm not entirely sure you are real.
