ipaddr 1.2.4 breaks URL blocker allowlist

The following discussion from !98777 (merged) should be addressed:

  • @mkaeppler started a discussion: (+18 comments)

    Tests are failing due to what I believe is the ipaddr version bump.

    This validator is failing:

          def validate_local_network(addrs_info)
            return unless addrs_info.any? { |addr| addr.ipv4_private? || addr.ipv6_sitelocal? || addr.ipv6_unique_local? }
    
            raise BlockedUrlError, "Requests to the local network are not allowed"
          end

    https://gitlab.com/gitlab-org/gitlab/-/blob/7359d23f4e078479969c872924150219c6f1665f/lib/gitlab/url_blocker.rb#L249

    For the following test and inputs:

    Gitlab::UrlBlocker#blocked_url? when allow_local_network is false when local domain/IP is allowed with IPs in allowlist behaves like allows local requests does not block urls from private networks
         Failure/Error: expect(described_class).not_to be_blocked_url("http://#{ip}", **url_blocker_attributes)
           expected `Gitlab::UrlBlocker.blocked_url?("http://[::ffff:ac10:20]", {:allow_local_network=>false, :allow_localhost=>false})` to be falsey, got true
         Shared Example Group: "allows local requests" called from ./spec/lib/gitlab/url_blocker_spec.rb:548

    Input IP: [#<Addrinfo: 172.16.0.32 TCP (::ffff:ac10:20)>]

    This is an IP from this list in the test data: https://gitlab.com/gitlab-org/gitlab/-/blob/7359d23f4e078479969c872924150219c6f1665f/spec/lib/gitlab/url_blocker_spec.rb#L405

    But we stub out Addrinfo here: https://gitlab.com/gitlab-org/gitlab/-/blob/7359d23f4e078479969c872924150219c6f1665f/spec/lib/gitlab/url_blocker_spec.rb#L760

    This looks fishy since we stub all inputs to return true for ipv4_local, but when I run this locally in a console I see:

    [9] pry(main)> Addrinfo.ip('::ffff:ac10:20').ipv4_private?
    => false

    So we are stubbing out Addrinfo in an unexpected way?

This was discovered while working on Ruby 3 support. Quick summary of what happened:

  • openssl depends on the ipaddr gem
  • This gem used to be bundled with the Ruby 2.7 distribution
  • Ruby 3 has unbundled this gem i.e. it is now tracked via Gemfile.lock.
  • The latest available version (1.2.4) contains changes compared to what Ruby 2.7 used to ship: https://github.com/ruby/ipaddr/compare/v1.2.2...v1.2.4
  • Specifically, IPAddr.include? has seen changes upstream that changes the equality relation between IPv4 and IPv6 addresses: !98777 (comment 1117740605) (arguably this is just a bug fix but it leads to behavioral changes).
  • This in turn means that the semantics of IP allowlists defined via outbound_local_requests_whitelist have changed and causes some of our unit tests to fail too.

For example, if previously the allowlist would contain the following entry:

::ffff:169.254.168.100

Then if the input IP was 169.254.168.100 or even ::ffff:169.254.168.100 (note that we convert IPv4-mapped IPv6 addresses to IPv4 before probing into the allowlist), the IP would be allowed because due to the aforementioned bug, the ipaddr library allowed matching an IPv4 address (source) against an IPv6 address (allowlist).

With the bug fixed in 1.2.4, this check will now fail i.e. return false. Admins would have to change or extend the allowlist to actually include an IPv4 address entry or mask such as:

169.254.0.0/16

We should assess how exactly this affects customers before pulling in the newer version and whether customers might have to review their allowlists prior to us relying on the latest version of ipaddr.

Assignee Loading
Time tracking Loading