Bump netaddr gem for CVE-2019-17383 #2369

FlorianNachtigall · 2022-04-01T12:20:46Z

tldr

netaddr 1.5.0 is vulnerable. Upgrading to netaddr v2 required a lot of changes (see d40d9b7). Let's discuss if this is reasonable and we should finalize the PR or whether other options should be considered (see #Alternatives).

What is this change about?

The currently used version of netaddr is vulnerable:

netaddr 1.5.0 -> CVE-2019-17383

This change updates the gem to the latest version with no known vulnerabilities. bundle update netaddr was used to ensure indirect dependencies are updated as well.

Breaking changes netaddr v1 -> v2

netaddr was completely rewritten from v1 to v2 and the interface changed significantly. Most importantly, NetAddr::CIDR was removed and replaced with NetAddr::IPv4Net & NetAddr::IPv6Net and NetAddr::IPv4 & NetAddr::IPv6, which provide similar functionality.

Our approach

Due to the broad usage of NetAddr::CIDR in the code base the PR introduces many changes, however, we tried to rebuild the old functionality with the new API as much as possible to keep the complexity manageable. We created a wrapper for NetAddr::IPv{4,6}Net -> IpUtil::CIDR and NetAddr::IPv{4,6} -> IpUtil::CIDRIP respectively to maintain the old behavior and differentiate the IP version in one central place.

All netaddr v2 changes have now been incorporated and all bosh-director unit tests are green. Some smaller todos (indicated by TODO NETADDR) and refactorings are still open.
Yet, before we finalize the PR, let's discuss if bumping netaddr to v2 is reasonable considering the size of the change or whether other options should be considered (see #Alternatives).

Open issues with netaddr v2

Handling of /64 and larger IPv6 subnets (see IPv6 CIDR prefixes dspinhirne/netaddr-rb#28 for more details)
Ambiguity between IPv4 and IPv6 addresses when storing them as integers in the database (see IPv6 ambiguity issues #2370; applies to netaddr v1 & v2)

Alternatives

The potential fix for CVE-2019-17383 was backported to 1.5.3, however, the version has not yet been released on rubygems.org. We could ask for it to be made available.
Adopting Ruby's default IPAddr gem which provides similar functionality and is potentially better maintained.

What tests have you run against this PR?

bosh-director unit tests (all succeeding besides two unrelated NATS cert tests which fail due to changed OpenSSL error messages)

How should this change be described in bosh release notes?

upgrade netaddr gem to 2.0.5

Does this PR introduce a breaking change?

no

Tag your pair, your PM, and/or team!

@friegger @cloudfoundry/cf-bosh-europe

src/bosh-director/lib/bosh/director/ip_util.rb

mvach · 2022-04-04T07:16:54Z

src/bosh-director/spec/unit/deployment_plan/network_planner/reservation_reconciler_spec.rb

@@ -255,7 +255,7 @@ module Bosh::Director::DeploymentPlan

            expect(existing_plans[0].reservation.network.name).to eq('my-network-2')
            expect(existing_plans[0].reservation.instance_model).to eq(instance_model)
-            expect(ip_to_netaddr(existing_plans[0].reservation.ip)).to eq('192.168.1.2')
+            expect(ip_to_netaddr(existing_plans[0].reservation.ip).to_s).to eq('192.168.1.2')


How about using the format_ip method in all these expects with ip_to_netaddr(..).to_s

Good idea, I changed all of them.

src/bosh-director/lib/bosh/director/deployment_plan/manual_network_subnet.rb

mvach · 2022-04-05T06:39:36Z

src/bosh-director/lib/bosh/director/deployment_plan/stages/create_network.rb

              raise NetworkReservedIpOutOfRange,
-                    "Reserved IP '#{format_ip(ip)}' is out of " \
+                    "Reserved IP '#{format_ip(ip_obj)}' is out of " \


I would use the raw ip from json here. Otherwise an exception could raise for. {"ip" : {...}} and you don't get the expected exception.
"Reserved IP '#{ip}' is out of "

each_ip iterates over the json and supplies the integer representation of each ip. To have a human-readable error message the ip has to be converted back to its string representation. Therefore, either format_ip(ip) or CIDRIP.parse(ip).to_s are required.

rkoster

Looks good overall, just a few TODO's that should be removed/addressed

rkoster · 2022-04-20T15:27:00Z

src/bosh-director/spec/unit/deployment_plan/manual_network_subnet_spec.rb

-      expect(subnet.netmask).to eq('ffff:ffff:ffff:ffff:0000:0000:0000:0000')
-      expect(subnet.gateway).to eq('fdab:d85c:118d:8a46:0000:0000:0000:0001')
+      expect(subnet.range.network.to_s).to eq('fdab:d85c:118d:8a46::')
+      # subnet.range.ip.size == 2**64  # TODO NETADDR: bug? / what was tested here?


That CIDR Prefix is parsed into a range size. Should probably be updated to network.prefix_len

rkoster · 2022-04-20T15:42:41Z

I do like the idea of moving to IPAddr, but since most of the work for bumping netaddr seems to have been done already, I would try and get this merged first.

jpalermo · 2022-04-21T06:04:24Z

I'm not opposed to merging it, but I feel like it would be easier if we just move to IPAddr. Either change is probably going to have some rough patches and pipeline breakages. Feels like less work to just fix things once, rather than bumping the netaddr gem, fixing things, then replacing netaddr and fixing things again.

I was looking over the use cases and it seems like IPAddr should work for everything we're using netaddr for. This way we have one fewer external dependency too.

I'm happy to work on an IPAddr PR, but also happy to defer to the folks who put this PR together if they want to do the work.

Again, also happy to merge it and work on IPAddr later if that's what the consensus is.

FlorianNachtigall · 2022-04-21T09:15:49Z

Thanks for the feedback. I'm happy with any approach. Though, the work already put into bumping netaddr shouldn't influence the decision and if we believe IPAddr is the better way forward, I have no hard feelings about discarding all changes.

friegger · 2022-04-21T09:21:07Z

Also asked for a netaddr 1.5.3 RubyGems release: dspinhirne/netaddr-rb#29

FlorianNachtigall · 2022-04-21T09:22:40Z

In terms of fixing the CVE quickly, what are your thoughts on bumping to netaddr 1.5.3 until we have migrated to IPAddr?
(blocked by dspinhirne/netaddr-rb#29)

rkoster · 2022-04-21T14:49:48Z

We could also install netaddr 1.5.3 from github (we also do this for eventmachine here), and then migrate to IPAddr.

Consume netaddr release from github since it hasn't been published on rubygems yet. For more context see: - dspinhirne/netaddr-rb#29 - #2369 Co-authored-by: Shilpa Chandrashekara <[email protected]>

FlorianNachtigall · 2022-05-05T15:05:19Z

Closing in favor of #2373

Consume netaddr release from github since it hasn't been published on rubygems yet. For more context see: - dspinhirne/netaddr-rb#29 - #2369 Co-authored-by: Shilpa Chandrashekara <[email protected]>

mvach reviewed Apr 4, 2022

View reviewed changes

src/bosh-director/lib/bosh/director/ip_util.rb Outdated Show resolved Hide resolved

mvach reviewed Apr 4, 2022

View reviewed changes

mvach reviewed Apr 5, 2022

View reviewed changes

src/bosh-director/lib/bosh/director/deployment_plan/manual_network_subnet.rb Show resolved Hide resolved

mvach reviewed Apr 5, 2022

View reviewed changes

FlorianNachtigall force-pushed the bump-gems branch 6 times, most recently from 56acdd8 to 44f1975 Compare April 6, 2022 14:37

FlorianNachtigall changed the title ~~Bump json & netaddr gem~~ Bump netaddr gem Apr 7, 2022

FlorianNachtigall added 3 commits April 7, 2022 16:37

Bump netaddr gem due to CVE-2019-17383

985f91e

Incorporate netaddr v2 changes

d40d9b7

Workaround for IPv6Net /64 anomaly in netaddr gem

3414bce

FlorianNachtigall force-pushed the bump-gems branch from 44f1975 to 3414bce Compare April 7, 2022 14:40

FlorianNachtigall changed the title ~~Bump netaddr gem~~ Bump netaddr gem for CVE-2019-17383 Apr 7, 2022

FlorianNachtigall marked this pull request as ready for review April 7, 2022 14:45

rkoster requested review from a team, jpalermo and bgandon and removed request for a team April 14, 2022 15:14

rkoster assigned bgandon and jpalermo Apr 14, 2022

rkoster requested changes Apr 20, 2022

View reviewed changes

bgandon removed their request for review April 21, 2022 06:26

bgandon removed their assignment Apr 21, 2022

FlorianNachtigall mentioned this pull request May 5, 2022

Bump netaddr gem to 1.5.3 to fix CVE-2019-17383 #2373

Merged

FlorianNachtigall closed this May 5, 2022

aramprice deleted the bump-gems branch November 22, 2024 19:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bump netaddr gem for CVE-2019-17383 #2369

Bump netaddr gem for CVE-2019-17383 #2369

FlorianNachtigall commented Apr 1, 2022 •

edited

Loading

mvach Apr 4, 2022

FlorianNachtigall Apr 6, 2022

mvach Apr 5, 2022

FlorianNachtigall Apr 6, 2022

rkoster left a comment

rkoster Apr 20, 2022

rkoster commented Apr 20, 2022

jpalermo commented Apr 21, 2022

FlorianNachtigall commented Apr 21, 2022

friegger commented Apr 21, 2022

FlorianNachtigall commented Apr 21, 2022

rkoster commented Apr 21, 2022

FlorianNachtigall commented May 5, 2022

Bump netaddr gem for CVE-2019-17383 #2369

Bump netaddr gem for CVE-2019-17383 #2369

Conversation

FlorianNachtigall commented Apr 1, 2022 • edited Loading

tldr

What is this change about?

Breaking changes netaddr v1 -> v2

Our approach

Open issues with netaddr v2

Alternatives

What tests have you run against this PR?

How should this change be described in bosh release notes?

Does this PR introduce a breaking change?

Tag your pair, your PM, and/or team!

mvach Apr 4, 2022

Choose a reason for hiding this comment

FlorianNachtigall Apr 6, 2022

Choose a reason for hiding this comment

mvach Apr 5, 2022

Choose a reason for hiding this comment

FlorianNachtigall Apr 6, 2022

Choose a reason for hiding this comment

rkoster left a comment

Choose a reason for hiding this comment

rkoster Apr 20, 2022

Choose a reason for hiding this comment

rkoster commented Apr 20, 2022

jpalermo commented Apr 21, 2022

FlorianNachtigall commented Apr 21, 2022

friegger commented Apr 21, 2022

FlorianNachtigall commented Apr 21, 2022

rkoster commented Apr 21, 2022

FlorianNachtigall commented May 5, 2022

FlorianNachtigall commented Apr 1, 2022 •

edited

Loading