| Message ID | 20220112131204.800307-3-Jason@zx2c4.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | remove remaining users of SHA-1 | expand |
| Context | Check | Description |
|---|---|---|
| bpf/vmtest-bpf-PR | fail | merge-conflict |
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply, async |
For the record, I've been able to simplify this even more in my remove-sha1 branch: https://git.zx2c4.com/linux-dev/log/?h=remove-sha1 . We no longer need the packed struct and we handle that secret a bit better too. If this patchset moves onto a non-RFC v2, that'll be part of it.
"Jason A. Donenfeld" <Jason@zx2c4.com> writes: > BLAKE2s is faster and more secure. SHA-1 has been broken for a long time > now. This also removes some code complexity, and lets us potentially > remove sha1 from lib, which would further reduce vmlinux size. So this one is a bit less obvious than the BPF case: the "stable address generation" is supposed to result in generating addresses that are, well, stable. The documentation for the stable_secret sysctl implies that this should be for the lifetime of the system: It is recommended to generate this secret during installation of a system and keep it stable after that. However, if we make this change, systems setting a stable_secret and using addr_gen_mode 2 or 3 will come up with a completely different address after a kernel upgrade. Which would be bad for any operator expecting to be able to find their machine again after a reboot, especially if it is accessed remotely. I haven't ever used this feature myself, though, or seen it in use. So I don't know if this is purely a theoretical concern, or if the stable_address feature is actually used in this way in practice. If it is, I guess the switch would have to be opt-in, which kinda defeats the purpose, no (i.e., we'd have to keep the SHA1 code around)? Adding some of the people involved in the original work on stable address generation in the hope that they can shed some light on the real-world uses for this feature. -Toke > Cc: Geert Uytterhoeven <geert@linux-m68k.org> > Cc: Herbert Xu <herbert@gondor.apana.org.au> > Cc: Ard Biesheuvel <ardb@kernel.org> > Cc: Jean-Philippe Aumasson <jeanphilippe.aumasson@gmail.com> > Cc: linux-crypto@vger.kernel.org > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> > --- > net/ipv6/addrconf.c | 31 +++++++++---------------------- > 1 file changed, 9 insertions(+), 22 deletions(-) > > diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c > index 3445f8017430..f5cb534aa261 100644 > --- a/net/ipv6/addrconf.c > +++ b/net/ipv6/addrconf.c > @@ -61,7 +61,7 @@ > #include <linux/delay.h> > #include <linux/notifier.h> > #include <linux/string.h> > -#include <linux/hash.h> > +#include <crypto/blake2s.h> > > #include <net/net_namespace.h> > #include <net/sock.h> > @@ -3225,25 +3225,16 @@ static int ipv6_generate_stable_address(struct in6_addr *address, > const struct inet6_dev *idev) > { > static DEFINE_SPINLOCK(lock); > - static __u32 digest[SHA1_DIGEST_WORDS]; > - static __u32 workspace[SHA1_WORKSPACE_WORDS]; > - > - static union { > - char __data[SHA1_BLOCK_SIZE]; > - struct { > - struct in6_addr secret; > - __be32 prefix[2]; > - unsigned char hwaddr[MAX_ADDR_LEN]; > - u8 dad_count; > - } __packed; > - } data; > - > + struct { > + struct in6_addr secret; > + __be32 prefix[2]; > + unsigned char hwaddr[MAX_ADDR_LEN]; > + u8 dad_count; > + } __packed data; > struct in6_addr secret; > struct in6_addr temp; > struct net *net = dev_net(idev->dev); > > - BUILD_BUG_ON(sizeof(data.__data) != sizeof(data)); > - > if (idev->cnf.stable_secret.initialized) > secret = idev->cnf.stable_secret.secret; > else if (net->ipv6.devconf_dflt->stable_secret.initialized) > @@ -3254,20 +3245,16 @@ static int ipv6_generate_stable_address(struct in6_addr *address, > retry: > spin_lock_bh(&lock); > > - sha1_init(digest); > memset(&data, 0, sizeof(data)); > - memset(workspace, 0, sizeof(workspace)); > memcpy(data.hwaddr, idev->dev->perm_addr, idev->dev->addr_len); > data.prefix[0] = address->s6_addr32[0]; > data.prefix[1] = address->s6_addr32[1]; > data.secret = secret; > data.dad_count = dad_count; > > - sha1_transform(digest, data.__data, workspace); > - > temp = *address; > - temp.s6_addr32[2] = (__force __be32)digest[0]; > - temp.s6_addr32[3] = (__force __be32)digest[1]; > + blake2s((u8 *)&temp.s6_addr32[2], (u8 *)&data, NULL, > + sizeof(temp.s6_addr32[2]) * 2, sizeof(data), 0); > > spin_unlock_bh(&lock); > > -- > 2.34.1
Hi Toke, On 1/13/22, Toke Høiland-Jørgensen <toke@redhat.com> wrote: > However, if we make this change, systems setting a stable_secret and > using addr_gen_mode 2 or 3 will come up with a completely different > address after a kernel upgrade. Which would be bad for any operator > expecting to be able to find their machine again after a reboot, > especially if it is accessed remotely. > > I haven't ever used this feature myself, though, or seen it in use. So I > don't know if this is purely a theoretical concern, or if the > stable_address feature is actually used in this way in practice. If it > is, I guess the switch would have to be opt-in, which kinda defeats the > purpose, no (i.e., we'd have to keep the SHA1 code around I'm not even so sure that's true. That was my worry at first, but actually, looking at this more closely, DAD means that the address can be changed anyway - a byte counter is hashed in - so there's no gurantee there. There's also the other aspect that open coding sha1_transform like this and prepending it with the secret (rather than a better construction) isn't so great... Take a look at the latest version of this in my branch to see a really nice simplification and security improvement: https://git.zx2c4.com/linux-dev/log/?h=remove-sha1 Jason
Hello, On 13.01.22 00:31, Jason A. Donenfeld wrote: > On 1/13/22, Toke Høiland-Jørgensen <toke@redhat.com> wrote: >> However, if we make this change, systems setting a stable_secret and >> using addr_gen_mode 2 or 3 will come up with a completely different >> address after a kernel upgrade. Which would be bad for any operator >> expecting to be able to find their machine again after a reboot, >> especially if it is accessed remotely. >> >> I haven't ever used this feature myself, though, or seen it in use. So I >> don't know if this is purely a theoretical concern, or if the >> stable_address feature is actually used in this way in practice. If it >> is, I guess the switch would have to be opt-in, which kinda defeats the >> purpose, no (i.e., we'd have to keep the SHA1 code around Yes, it is hard to tell if such a change would have real world impact due to not knowing its actual usage in the field - but I would avoid such a change. The reason for this standard is to have stable addresses across reboots. The standard is widely used but most servers or desktops might get their stable privacy addresses being generated by user space network management systems (NetworkManager/networkd) nowadays. I would guess it could be used in embedded installations. The impact of this change could be annoying though: users could suddenly lose connectivity due to e.g. changes to the default gateway after an upgrade. > I'm not even so sure that's true. That was my worry at first, but > actually, looking at this more closely, DAD means that the address can > be changed anyway - a byte counter is hashed in - so there's no > gurantee there. The duplicate address detection counter is a way to merely provide basic network connectivity in case of duplicate addresses on the network (maybe some kind misconfiguration or L2 attack). Such detected addresses would show up in the kernel log and an administrator should investigate and clean up the situation. Afterwards bringing the interface down and up again should revert the interface to its initial (dad_counter == 0) address. > There's also the other aspect that open coding sha1_transform like > this and prepending it with the secret (rather than a better > construction) isn't so great... Take a look at the latest version of > this in my branch to see a really nice simplification and security > improvement: > > https://git.zx2c4.com/linux-dev/log/?h=remove-sha1 All in all, I consider the hash produced here as being part of uAPI unfortunately and thus cannot be changed. It is unfortunate that it can't easily be improved (I assume a separate mode for this is not reasonable). The patches definitely look like a nice cleanup. Would this be the only user of sha_transform left? Bye, Hannes
On Thu, 13 Jan 2022 at 12:15, Hannes Frederic Sowa <hannes@stressinduktion.org> wrote: > > Hello, > > On 13.01.22 00:31, Jason A. Donenfeld wrote: > > On 1/13/22, Toke Høiland-Jørgensen <toke@redhat.com> wrote: > >> However, if we make this change, systems setting a stable_secret and > >> using addr_gen_mode 2 or 3 will come up with a completely different > >> address after a kernel upgrade. Which would be bad for any operator > >> expecting to be able to find their machine again after a reboot, > >> especially if it is accessed remotely. > >> > >> I haven't ever used this feature myself, though, or seen it in use. So I > >> don't know if this is purely a theoretical concern, or if the > >> stable_address feature is actually used in this way in practice. If it > >> is, I guess the switch would have to be opt-in, which kinda defeats the > >> purpose, no (i.e., we'd have to keep the SHA1 code around > > Yes, it is hard to tell if such a change would have real world impact > due to not knowing its actual usage in the field - but I would avoid > such a change. The reason for this standard is to have stable addresses > across reboots. The standard is widely used but most servers or desktops > might get their stable privacy addresses being generated by user space > network management systems (NetworkManager/networkd) nowadays. I would > guess it could be used in embedded installations. > > The impact of this change could be annoying though: users could suddenly > lose connectivity due to e.g. changes to the default gateway after an > upgrade. > > > I'm not even so sure that's true. That was my worry at first, but > > actually, looking at this more closely, DAD means that the address can > > be changed anyway - a byte counter is hashed in - so there's no > > gurantee there. > > The duplicate address detection counter is a way to merely provide basic > network connectivity in case of duplicate addresses on the network > (maybe some kind misconfiguration or L2 attack). Such detected addresses > would show up in the kernel log and an administrator should investigate > and clean up the situation. Afterwards bringing the interface down and > up again should revert the interface to its initial (dad_counter == 0) > address. > > > There's also the other aspect that open coding sha1_transform like > > this and prepending it with the secret (rather than a better > > construction) isn't so great... Take a look at the latest version of > > this in my branch to see a really nice simplification and security > > improvement: > > > > https://git.zx2c4.com/linux-dev/log/?h=remove-sha1 > > All in all, I consider the hash produced here as being part of uAPI > unfortunately and thus cannot be changed. It is unfortunate that it > can't easily be improved (I assume a separate mode for this is not > reasonable). The patches definitely look like a nice cleanup. > > Would this be the only user of sha_transform left? > The question is not whether but when we can/will change this. SHA-1 is broken and should be removed at *some* point, so unless the feature itself is going to be obsolete, its implementation will need to switch to a PRF that fulfils the requirements in RFC7217 once SHA-1 ceases to do so. And I should also point out that the current implementation does not even use SHA-1 correctly, as it omits the finalization step. This may or may not matter in practice, but it deviates from crypto best practices, as well as from RFC7217 I already pointed out to Jason (in private) that the PRF does not need to be based on a cryptographic hash, so as far as I can tell, siphash would be a suitable candidate here as well, and I already switched the TCP fastopen code to that in the past. But SHA-1 definitely has to go.
On 1/13/22, Ard Biesheuvel <ardb@kernel.org> wrote: > > The question is not whether but when we can/will change this. > > SHA-1 is broken and should be removed at *some* point, so unless the > feature itself is going to be obsolete, its implementation will need > to switch to a PRF that fulfils the requirements in RFC7217 once SHA-1 > ceases to do so. > > And I should also point out that the current implementation does not > even use SHA-1 correctly, as it omits the finalization step. This may > or may not matter in practice, but it deviates from crypto best > practices, as well as from RFC7217 > > I already pointed out to Jason (in private) that the PRF does not need > to be based on a cryptographic hash, so as far as I can tell, siphash > would be a suitable candidate here as well, and I already switched the > TCP fastopen code to that in the past. But SHA-1 definitely has to go. > Correction: this should be a cryptographically secure. That's part of the point of moving away from SHA-1 of course. But fortunately, siphash *is* considered to be cryptographically secure. Whether you want blake2s's keyed mode or siphash doesn't really matter to me. I thought the former's API mapped a bit neater here.
On Thu, 13 Jan 2022 at 13:22, Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > On 1/13/22, Ard Biesheuvel <ardb@kernel.org> wrote: > > > > The question is not whether but when we can/will change this. > > > > SHA-1 is broken and should be removed at *some* point, so unless the > > feature itself is going to be obsolete, its implementation will need > > to switch to a PRF that fulfils the requirements in RFC7217 once SHA-1 > > ceases to do so. > > > > And I should also point out that the current implementation does not > > even use SHA-1 correctly, as it omits the finalization step. This may > > or may not matter in practice, but it deviates from crypto best > > practices, as well as from RFC7217 > > > > I already pointed out to Jason (in private) that the PRF does not need > > to be based on a cryptographic hash, so as far as I can tell, siphash > > would be a suitable candidate here as well, and I already switched the > > TCP fastopen code to that in the past. But SHA-1 definitely has to go. > > > > Correction: this should be a cryptographically secure. Of course. I said it does not need to be based on a cryptographic *hash*. > That's part of > the point of moving away from SHA-1 of course. But fortunately, > siphash *is* > considered to be cryptographically secure. Whether you want blake2s's > keyed mode or siphash doesn't really matter to me. I thought the > former's API mapped a bit neater here. Fair enough. This is not on a hot path anyway, so it doesn't really matter performance wise.
Ard Biesheuvel <ardb@kernel.org> writes: > On Thu, 13 Jan 2022 at 12:15, Hannes Frederic Sowa > <hannes@stressinduktion.org> wrote: >> >> Hello, >> >> On 13.01.22 00:31, Jason A. Donenfeld wrote: >> > On 1/13/22, Toke Høiland-Jørgensen <toke@redhat.com> wrote: >> >> However, if we make this change, systems setting a stable_secret and >> >> using addr_gen_mode 2 or 3 will come up with a completely different >> >> address after a kernel upgrade. Which would be bad for any operator >> >> expecting to be able to find their machine again after a reboot, >> >> especially if it is accessed remotely. >> >> >> >> I haven't ever used this feature myself, though, or seen it in use. So I >> >> don't know if this is purely a theoretical concern, or if the >> >> stable_address feature is actually used in this way in practice. If it >> >> is, I guess the switch would have to be opt-in, which kinda defeats the >> >> purpose, no (i.e., we'd have to keep the SHA1 code around >> >> Yes, it is hard to tell if such a change would have real world impact >> due to not knowing its actual usage in the field - but I would avoid >> such a change. The reason for this standard is to have stable addresses >> across reboots. The standard is widely used but most servers or desktops >> might get their stable privacy addresses being generated by user space >> network management systems (NetworkManager/networkd) nowadays. I would >> guess it could be used in embedded installations. >> >> The impact of this change could be annoying though: users could suddenly >> lose connectivity due to e.g. changes to the default gateway after an >> upgrade. >> >> > I'm not even so sure that's true. That was my worry at first, but >> > actually, looking at this more closely, DAD means that the address can >> > be changed anyway - a byte counter is hashed in - so there's no >> > gurantee there. >> >> The duplicate address detection counter is a way to merely provide basic >> network connectivity in case of duplicate addresses on the network >> (maybe some kind misconfiguration or L2 attack). Such detected addresses >> would show up in the kernel log and an administrator should investigate >> and clean up the situation. Afterwards bringing the interface down and >> up again should revert the interface to its initial (dad_counter == 0) >> address. >> >> > There's also the other aspect that open coding sha1_transform like >> > this and prepending it with the secret (rather than a better >> > construction) isn't so great... Take a look at the latest version of >> > this in my branch to see a really nice simplification and security >> > improvement: >> > >> > https://git.zx2c4.com/linux-dev/log/?h=remove-sha1 >> >> All in all, I consider the hash produced here as being part of uAPI >> unfortunately and thus cannot be changed. It is unfortunate that it >> can't easily be improved (I assume a separate mode for this is not >> reasonable). The patches definitely look like a nice cleanup. >> >> Would this be the only user of sha_transform left? >> > > The question is not whether but when we can/will change this. > > SHA-1 is broken and should be removed at *some* point, so unless the > feature itself is going to be obsolete, its implementation will need > to switch to a PRF that fulfils the requirements in RFC7217 once SHA-1 > ceases to do so. > > And I should also point out that the current implementation does not > even use SHA-1 correctly, as it omits the finalization step. This may > or may not matter in practice, but it deviates from crypto best > practices, as well as from RFC7217 Right, but that implies we need to work on a transition mechanism. For newly deployed systems changing the hash is obviously fine, it's the "reboot and you have a new address" problem. We could introduce new values to the addr_gen_mode? I.e. values of 4 and 5 would be equivalent to 2 and 3 (respectively), but with the new hashing algorithm? And then document that 2 and 3 are considered deprecated to be removed at some point in the future... -Toke
On Thu, 13 Jan 2022 at 14:30, Toke Høiland-Jørgensen <toke@redhat.com> wrote: > > Ard Biesheuvel <ardb@kernel.org> writes: > > > On Thu, 13 Jan 2022 at 12:15, Hannes Frederic Sowa > > <hannes@stressinduktion.org> wrote: > >> > >> Hello, > >> > >> On 13.01.22 00:31, Jason A. Donenfeld wrote: > >> > On 1/13/22, Toke Høiland-Jørgensen <toke@redhat.com> wrote: > >> >> However, if we make this change, systems setting a stable_secret and > >> >> using addr_gen_mode 2 or 3 will come up with a completely different > >> >> address after a kernel upgrade. Which would be bad for any operator > >> >> expecting to be able to find their machine again after a reboot, > >> >> especially if it is accessed remotely. > >> >> > >> >> I haven't ever used this feature myself, though, or seen it in use. So I > >> >> don't know if this is purely a theoretical concern, or if the > >> >> stable_address feature is actually used in this way in practice. If it > >> >> is, I guess the switch would have to be opt-in, which kinda defeats the > >> >> purpose, no (i.e., we'd have to keep the SHA1 code around > >> > >> Yes, it is hard to tell if such a change would have real world impact > >> due to not knowing its actual usage in the field - but I would avoid > >> such a change. The reason for this standard is to have stable addresses > >> across reboots. The standard is widely used but most servers or desktops > >> might get their stable privacy addresses being generated by user space > >> network management systems (NetworkManager/networkd) nowadays. I would > >> guess it could be used in embedded installations. > >> > >> The impact of this change could be annoying though: users could suddenly > >> lose connectivity due to e.g. changes to the default gateway after an > >> upgrade. > >> > >> > I'm not even so sure that's true. That was my worry at first, but > >> > actually, looking at this more closely, DAD means that the address can > >> > be changed anyway - a byte counter is hashed in - so there's no > >> > gurantee there. > >> > >> The duplicate address detection counter is a way to merely provide basic > >> network connectivity in case of duplicate addresses on the network > >> (maybe some kind misconfiguration or L2 attack). Such detected addresses > >> would show up in the kernel log and an administrator should investigate > >> and clean up the situation. Afterwards bringing the interface down and > >> up again should revert the interface to its initial (dad_counter == 0) > >> address. > >> > >> > There's also the other aspect that open coding sha1_transform like > >> > this and prepending it with the secret (rather than a better > >> > construction) isn't so great... Take a look at the latest version of > >> > this in my branch to see a really nice simplification and security > >> > improvement: > >> > > >> > https://git.zx2c4.com/linux-dev/log/?h=remove-sha1 > >> > >> All in all, I consider the hash produced here as being part of uAPI > >> unfortunately and thus cannot be changed. It is unfortunate that it > >> can't easily be improved (I assume a separate mode for this is not > >> reasonable). The patches definitely look like a nice cleanup. > >> > >> Would this be the only user of sha_transform left? > >> > > > > The question is not whether but when we can/will change this. > > > > SHA-1 is broken and should be removed at *some* point, so unless the > > feature itself is going to be obsolete, its implementation will need > > to switch to a PRF that fulfils the requirements in RFC7217 once SHA-1 > > ceases to do so. > > > > And I should also point out that the current implementation does not > > even use SHA-1 correctly, as it omits the finalization step. This may > > or may not matter in practice, but it deviates from crypto best > > practices, as well as from RFC7217 > > Right, but that implies we need to work on a transition mechanism. For > newly deployed systems changing the hash is obviously fine, it's the > "reboot and you have a new address" problem. > > We could introduce new values to the addr_gen_mode? I.e. values of 4 and > 5 would be equivalent to 2 and 3 (respectively), but with the new > hashing algorithm? And then document that 2 and 3 are considered > deprecated to be removed at some point in the future... > I guess that for the time being, we could use assignments of stable_secret by user space as a hint that we should switch to the old scheme. We'd also need a knob to opt into the new scheme in that case, and maybe print a warning otherwise? That would at least give us a path forward where we can rip it out /some/ point in the future.
Hi Toke, On Thu, Jan 13, 2022 at 2:30 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > Right, but that implies we need to work on a transition mechanism. For > newly deployed systems changing the hash is obviously fine, it's the > "reboot and you have a new address" problem. > > We could introduce new values to the addr_gen_mode? I.e. values of 4 and > 5 would be equivalent to 2 and 3 (respectively), but with the new > hashing algorithm? And then document that 2 and 3 are considered > deprecated to be removed at some point in the future... Right, so this is exactly the flow of conversation I anticipated. "Let's change it!" "No, we can't." "Okay, let's add a knob." The knob I was thinking about, though, was actually a compile-time one CONFIG_NET_OBSOLETE_INSECURE_ADDRCONF_HASH, which itself is a `depends on CONFIG_OLD_N_CRUSTY` or something. This way we could gate the inclusion of sha1.c/sha1.o on that at compile time, and shave down vmlinux a bit, which would make Geert happy. Then, at some point down the road, we can talk about removing CONFIG_NET_OBSOLETE_INSECURE_ADDRCONF_HASH too. Jason
On Thu, 13 Jan 2022 at 14:46, Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > Hi Toke, > > On Thu, Jan 13, 2022 at 2:30 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > > Right, but that implies we need to work on a transition mechanism. For > > newly deployed systems changing the hash is obviously fine, it's the > > "reboot and you have a new address" problem. > > > > We could introduce new values to the addr_gen_mode? I.e. values of 4 and > > 5 would be equivalent to 2 and 3 (respectively), but with the new > > hashing algorithm? And then document that 2 and 3 are considered > > deprecated to be removed at some point in the future... > > Right, so this is exactly the flow of conversation I anticipated. > "Let's change it!" "No, we can't." "Okay, let's add a knob." > > The knob I was thinking about, though, was actually a compile-time one > CONFIG_NET_OBSOLETE_INSECURE_ADDRCONF_HASH, which itself is a `depends > on CONFIG_OLD_N_CRUSTY` or something. This way we could gate the > inclusion of sha1.c/sha1.o on that at compile time, and shave down > vmlinux a bit, which would make Geert happy. > > Then, at some point down the road, we can talk about removing > CONFIG_NET_OBSOLETE_INSECURE_ADDRCONF_HASH too. > What is the point of having CONFIG_OLD_N_CRUSTY if all distros are going to enable it indefinitely?
On Thu, Jan 13, 2022 at 2:50 PM Ard Biesheuvel <ardb@kernel.org> wrote: > > Then, at some point down the road, we can talk about removing > > CONFIG_NET_OBSOLETE_INSECURE_ADDRCONF_HASH too. > > > > What is the point of having CONFIG_OLD_N_CRUSTY if all distros are > going to enable it indefinitely? I think there's probably some combination of CONFIG_NET_OBSOLETE_INSECURE_ADDRCONF_HASH and CONFIG_OLD_N_CRUSTY and maybe even a CONFIG_GOD_MURDERS_KITTENS that might be sufficiently disincentivizing? Or this ties into other general ideas on a gradual obsolescence->removal flow for things.
"Jason A. Donenfeld" <Jason@zx2c4.com> writes: > On Thu, Jan 13, 2022 at 2:50 PM Ard Biesheuvel <ardb@kernel.org> wrote: >> > Then, at some point down the road, we can talk about removing >> > CONFIG_NET_OBSOLETE_INSECURE_ADDRCONF_HASH too. >> > >> >> What is the point of having CONFIG_OLD_N_CRUSTY if all distros are >> going to enable it indefinitely? > > I think there's probably some combination of > CONFIG_NET_OBSOLETE_INSECURE_ADDRCONF_HASH and CONFIG_OLD_N_CRUSTY and > maybe even a CONFIG_GOD_MURDERS_KITTENS that might be sufficiently > disincentivizing? Or this ties into other general ideas on a gradual > obsolescence->removal flow for things. Making it a compile-time switch doesn't really solve anything, though. It'll need to be a runtime switch for people to be able to opt-in to the new behaviour; otherwise there would still be a flag day when distributions switch on the new config option. I don't think there's any reason to offload this decision on distributions either: there's clearly a "best option" here, absent any backwards compatibility concerns. So it's on us to design a proper transition mechanism. Defaulting to SHA1 when stable_secret is set, as Ard suggested, sounds like a reasonable default; then we only need a single new value for addr_gen_mode to opt-in to using blake2s even when setting the stable_secret. -Toke
Hi Hannes, On Thu, Jan 13, 2022 at 12:15 PM Hannes Frederic Sowa <hannes@stressinduktion.org> wrote: > > I'm not even so sure that's true. That was my worry at first, but > > actually, looking at this more closely, DAD means that the address can > > be changed anyway - a byte counter is hashed in - so there's no > > guarantee there. > > The duplicate address detection counter is a way to merely provide basic > network connectivity in case of duplicate addresses on the network > (maybe some kind misconfiguration or L2 attack). Such detected addresses > would show up in the kernel log and an administrator should investigate > and clean up the situation. I don't mean to belabor a point where I'm likely wrong anyway, but this DAD business has kept me thinking... Attacker is hanging out on the network sending DAD responses, forcing those counters to increment, and thus making SHA1(stuff || counter) result in a different IPv6 address than usual. Outcomes: 1) The administrator cannot handle this, did not understand the semantics of this address generation feature, and will now have a broken network; 2) The administrator knows what he's doing, and will be able to handle a different IPv6 address coming up. Do we really care about case (1)? That sounds like emacs spacebar heating https://xkcd.com/1172/. And case (2) seems like something that would tolerate us changing the hash function. > Afterwards bringing the interface down and > up again should revert the interface to its initial (dad_counter == 0) > address. Except the attacker is still on the network, and the administrator can't figure it out because the mac addresses keep changing and it's arriving from seemingly random switches! Plot twist: the attack is being conducted from an implant in the switch firmware. There are a lot of creative different takes on the same basic scenario. The point is - the administrator really _can't_ rely on the address always being the same, because it's simply out of his control. Given that the admin already *must* be prepared for the address to change, doesn't that give us some leeway to change the algorithm used between kernels? Or to put it differently, are there _actually_ braindead deployments out there that truly rely on the address never ever changing, and should we be going out of our way to support what is arguably a misreading and misdeployment of the feature? (Feel free to smack this line of argumentation down if you disagree. I just thought it should be a bit more thoroughly explored.) Jason
"Jason A. Donenfeld" <Jason@zx2c4.com> writes: > Hi Hannes, > > On Thu, Jan 13, 2022 at 12:15 PM Hannes Frederic Sowa > <hannes@stressinduktion.org> wrote: >> > I'm not even so sure that's true. That was my worry at first, but >> > actually, looking at this more closely, DAD means that the address can >> > be changed anyway - a byte counter is hashed in - so there's no >> > guarantee there. >> >> The duplicate address detection counter is a way to merely provide basic >> network connectivity in case of duplicate addresses on the network >> (maybe some kind misconfiguration or L2 attack). Such detected addresses >> would show up in the kernel log and an administrator should investigate >> and clean up the situation. > > I don't mean to belabor a point where I'm likely wrong anyway, but > this DAD business has kept me thinking... > > Attacker is hanging out on the network sending DAD responses, forcing > those counters to increment, and thus making SHA1(stuff || counter) > result in a different IPv6 address than usual. Outcomes: > 1) The administrator cannot handle this, did not understand the > semantics of this address generation feature, and will now have a > broken network; > 2) The administrator knows what he's doing, and will be able to handle > a different IPv6 address coming up. > > Do we really care about case (1)? That sounds like emacs spacebar > heating https://xkcd.com/1172/. And case (2) seems like something that > would tolerate us changing the hash function. Privacy addresses mostly address identification outside of the local network (because on the local network you can see the MAC address), so I don't think it's unreasonable for someone to enable this and not have a procedure in place to deal with DAD causing the address to change. For instance, they could manage their network in a way that they won't happen (or just turn off DAD entirely on the affected boxes). >> Afterwards bringing the interface down and >> up again should revert the interface to its initial (dad_counter == 0) >> address. > > Except the attacker is still on the network, and the administrator > can't figure it out because the mac addresses keep changing and it's > arriving from seemingly random switches! Plot twist: the attack is > being conducted from an implant in the switch firmware. There are a > lot of creative different takes on the same basic scenario. The point > is - the administrator really _can't_ rely on the address always being > the same, because it's simply out of his control. > > Given that the admin already *must* be prepared for the address to > change, doesn't that give us some leeway to change the algorithm used > between kernels? > > Or to put it differently, are there _actually_ braindead deployments > out there that truly rely on the address never ever changing, and > should we be going out of our way to support what is arguably a > misreading and misdeployment of the feature? > > (Feel free to smack this line of argumentation down if you disagree. I > just thought it should be a bit more thoroughly explored.) I kinda get where you're coming from, but most systems are not actively under attack, and those will still "break" if this is just changed. Which is one of those "a kernel upgrade broke my system" type of events that we want to avoid because it makes people vary of upgrading, so they'll keep running old kernels way past their expiry dates. -Toke
Hello, On Fri, Jan 14, 2022, at 17:07, Jason A. Donenfeld wrote: > On Thu, Jan 13, 2022 at 12:15 PM Hannes Frederic Sowa > <hannes@stressinduktion.org> wrote: >> > I'm not even so sure that's true. That was my worry at first, but >> > actually, looking at this more closely, DAD means that the address can >> > be changed anyway - a byte counter is hashed in - so there's no >> > guarantee there. >> >> The duplicate address detection counter is a way to merely provide basic >> network connectivity in case of duplicate addresses on the network >> (maybe some kind misconfiguration or L2 attack). Such detected addresses >> would show up in the kernel log and an administrator should investigate >> and clean up the situation. > > I don't mean to belabor a point where I'm likely wrong anyway, but > this DAD business has kept me thinking... > > Attacker is hanging out on the network sending DAD responses, forcing > those counters to increment, and thus making SHA1(stuff || counter) > result in a different IPv6 address than usual. Outcomes: > 1) The administrator cannot handle this, did not understand the > semantics of this address generation feature, and will now have a > broken network; > 2) The administrator knows what he's doing, and will be able to handle > a different IPv6 address coming up. > > Do we really care about case (1)? That sounds like emacs spacebar > heating https://xkcd.com/1172/. And case (2) seems like something that > would tolerate us changing the hash function. Taking a step back, there is the base case where we don't have duplicate addresses on the network nor an attack is on-going. We would break those setups with that patch. And those are the ones that matter most. In particular those stable-random addresses are being used in router advertisements for announcing the next-hop/default gateway on the broadcast domain. During my time in IPv6 land I have seen lots of setups where those automatic advertisements got converted into static configuration for the sake of getting hands on a cool looking IPv6 address on another host (I did that as well ;) ). In particular, in the last example, you might not only have one administrator at hand to handle the issue, but probably multiple roles are involved (host admin and network admin maybe from different organizations - how annoying - but that's a worst case scenario). Furthermore most L2 attacks nowadays are stopped by smarter switches or wifi access points(?) anyway with per-port MAC learning and other hardening features. Obviously this only happens in more managed environments but probably already also at smaller home networks nowadays. Datacenters probably already limit access to the Layer 2 raw network in such a way that this attack is probably not possible either. Same for IoT stuff where you probably have a point-to-point IPv6 connection anyway. The worst case scenario is someone upgrading their kernel during a trip away from home, rebooting, and losing access to their system. If we experience just one of those cases we have violated Linux strict uAPI rules (in my opinion). Thus, yes, we care about both, (1) and (2) cases. I don't think we can argue our way out of this by stating that there are no guarantees anyway, as much as I would like to change the hash function as well. As much as I know about the problems with SHA1 and would like to see it removed from the kernel as well, I fear that in this case it seems hard to do. I would propose putting sha1 into a compilation unit and overwrite the compiler flags to optimize the function optimized for size and maybe add another mode or knob to switch the hashing algorithm if necessary. >> Afterwards bringing the interface down and >> up again should revert the interface to its initial (dad_counter == 0) >> address. > > Except the attacker is still on the network, and the administrator > can't figure it out because the mac addresses keep changing and it's > arriving from seemingly random switches! Plot twist: the attack is > being conducted from an implant in the switch firmware. There are a > lot of creative different takes on the same basic scenario. The point > is - the administrator really _can't_ rely on the address always being > the same, because it's simply out of his control. This is a very pessimistic scenario bordering a nightmare. I hope the new hashing algorithm will protect them. ;) > Given that the admin already *must* be prepared for the address to > change, doesn't that give us some leeway to change the algorithm used > between kernels? > > Or to put it differently, are there _actually_ braindead deployments > out there that truly rely on the address never ever changing, and > should we be going out of our way to support what is arguably a > misreading and misdeployment of the feature? Given the example above, users might hardcode this generated IP address as a default gateway in their configs on other hosts. This is actually a very common thing to do. > (Feel free to smack this line of argumentation down if you disagree. I > just thought it should be a bit more thoroughly explored.) I haven't investigated recent research into breakage of SHA1, I mostly remember the chosen-image and collision attacks against it. Given the particular usage of SHA1 in this case, do you think switching the hashing function increases security? I am asking because of the desire to decrease the instruction size of the kernel, but adding a switch will actually increase the size in the foreseeable future (and I agree with Toke that offloading this decision to distributions is probably not fair). Maybe at some point the networking subsystem will adapt a generic knob like LD_ASSUME_KERNEL? ;) Bye, Hannes
Hi Hannes, On Fri, Jan 14, 2022 at 6:44 PM Hannes Frederic Sowa <hannes@stressinduktion.org> wrote: > I don't think we can argue our way out of this by stating that there are > no guarantees anyway, as much as I would like to change the hash > function as well. Shucks. Alright then. > As much as I know about the problems with SHA1 and would like to see it > removed from the kernel as well, I fear that in this case it seems hard > to do. I would propose putting sha1 into a compilation unit and > overwrite the compiler flags to optimize the function optimized for size > and maybe add another mode or knob to switch the hashing algorithm if > necessary. Already on it! :) https://lore.kernel.org/linux-crypto/20220114154247.99773-3-Jason@zx2c4.com/ > I haven't investigated recent research into breakage of SHA1, I mostly > remember the chosen-image and collision attacks against it. Given the > particular usage of SHA1 in this case, do you think switching the > hashing function increases security? Considering we're only using 64-bits of SHA-1 output, I don't think the SHA-1 collision attacks give you that much here. And it seems like there are other network-level security concerns with the whole scheme anyway. So it might not be the largest of matters. However... > I am asking because of the desire > to decrease the instruction size of the kernel Indeed this is what I was hoping for. Jason
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c index 3445f8017430..f5cb534aa261 100644 --- a/net/ipv6/addrconf.c +++ b/net/ipv6/addrconf.c @@ -61,7 +61,7 @@ #include <linux/delay.h> #include <linux/notifier.h> #include <linux/string.h> -#include <linux/hash.h> +#include <crypto/blake2s.h> #include <net/net_namespace.h> #include <net/sock.h> @@ -3225,25 +3225,16 @@ static int ipv6_generate_stable_address(struct in6_addr *address, const struct inet6_dev *idev) { static DEFINE_SPINLOCK(lock); - static __u32 digest[SHA1_DIGEST_WORDS]; - static __u32 workspace[SHA1_WORKSPACE_WORDS]; - - static union { - char __data[SHA1_BLOCK_SIZE]; - struct { - struct in6_addr secret; - __be32 prefix[2]; - unsigned char hwaddr[MAX_ADDR_LEN]; - u8 dad_count; - } __packed; - } data; - + struct { + struct in6_addr secret; + __be32 prefix[2]; + unsigned char hwaddr[MAX_ADDR_LEN]; + u8 dad_count; + } __packed data; struct in6_addr secret; struct in6_addr temp; struct net *net = dev_net(idev->dev); - BUILD_BUG_ON(sizeof(data.__data) != sizeof(data)); - if (idev->cnf.stable_secret.initialized) secret = idev->cnf.stable_secret.secret; else if (net->ipv6.devconf_dflt->stable_secret.initialized) @@ -3254,20 +3245,16 @@ static int ipv6_generate_stable_address(struct in6_addr *address, retry: spin_lock_bh(&lock); - sha1_init(digest); memset(&data, 0, sizeof(data)); - memset(workspace, 0, sizeof(workspace)); memcpy(data.hwaddr, idev->dev->perm_addr, idev->dev->addr_len); data.prefix[0] = address->s6_addr32[0]; data.prefix[1] = address->s6_addr32[1]; data.secret = secret; data.dad_count = dad_count; - sha1_transform(digest, data.__data, workspace); - temp = *address; - temp.s6_addr32[2] = (__force __be32)digest[0]; - temp.s6_addr32[3] = (__force __be32)digest[1]; + blake2s((u8 *)&temp.s6_addr32[2], (u8 *)&data, NULL, + sizeof(temp.s6_addr32[2]) * 2, sizeof(data), 0); spin_unlock_bh(&lock);
BLAKE2s is faster and more secure. SHA-1 has been broken for a long time now. This also removes some code complexity, and lets us potentially remove sha1 from lib, which would further reduce vmlinux size. Cc: Geert Uytterhoeven <geert@linux-m68k.org> Cc: Herbert Xu <herbert@gondor.apana.org.au> Cc: Ard Biesheuvel <ardb@kernel.org> Cc: Jean-Philippe Aumasson <jeanphilippe.aumasson@gmail.com> Cc: linux-crypto@vger.kernel.org Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> --- net/ipv6/addrconf.c | 31 +++++++++---------------------- 1 file changed, 9 insertions(+), 22 deletions(-)