Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some IPv4 and IPv4-mapped IPv6 properties don't match #122792

Open
sethmlarson opened this issue Aug 7, 2024 · 4 comments
Open

Some IPv4 and IPv4-mapped IPv6 properties don't match #122792

sethmlarson opened this issue Aug 7, 2024 · 4 comments
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error type-security A security issue

Comments

@sethmlarson
Copy link
Contributor

sethmlarson commented Aug 7, 2024

Bug report

Bug description:

The following properties on an IPv6Address don't match their IPv4Address counterparts when using an IPv6-mapped IPv4 address (ie ::ffff:<ipv4>):

  • is_multicast
  • is_reserved
  • is_link_local
  • is_global
  • is_unspecified

Proposed fix is to make all properties use their IPv4Address values for IPv4-mapped addresses.

CPython versions tested on:

CPython main branch

Operating systems tested on:

No response

Linked PRs

@sethmlarson sethmlarson added type-bug An unexpected behavior, bug, or error type-security A security issue stdlib Python modules in the Lib dir labels Aug 7, 2024
gpshead pushed a commit that referenced this issue Sep 7, 2024
…Pv4 (GH-122793)

Make IPv4-mapped IPv6 address properties consistent with IPv4.
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Sep 7, 2024
…with IPv4 (pythonGH-122793)

Make IPv4-mapped IPv6 address properties consistent with IPv4.
(cherry picked from commit 76a1c5d)

Co-authored-by: Seth Michael Larson <seth@python.org>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Sep 7, 2024
…with IPv4 (pythonGH-122793)

Make IPv4-mapped IPv6 address properties consistent with IPv4.
(cherry picked from commit 76a1c5d)

Co-authored-by: Seth Michael Larson <seth@python.org>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Sep 7, 2024
…with IPv4 (pythonGH-122793)

Make IPv4-mapped IPv6 address properties consistent with IPv4.
(cherry picked from commit 76a1c5d)

Co-authored-by: Seth Michael Larson <seth@python.org>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Sep 7, 2024
…with IPv4 (pythonGH-122793)

Make IPv4-mapped IPv6 address properties consistent with IPv4.
(cherry picked from commit 76a1c5d)

Co-authored-by: Seth Michael Larson <seth@python.org>
gpshead pushed a commit that referenced this issue Sep 7, 2024
… with IPv4 (GH-122793) (GH-123814)

gh-122792: Make IPv4-mapped IPv6 address properties consistent with IPv4 (GH-122793)

Make IPv4-mapped IPv6 address properties consistent with IPv4.
(cherry picked from commit 76a1c5d)

Co-authored-by: Seth Michael Larson <seth@python.org>
@jstasiak
Copy link
Contributor

jstasiak commented Sep 7, 2024

Hey, I learned about this issue just today and, as a user of this API, I'd like to express some opposition to it.

My reasons, in no particular order:

  1. The proposed change makes the API more unpredictable and confusing (somewhat subjective) by "unwrapping" the IPv4-mapped addresses.
  2. This change makes the properties inconsistent with the already documented behavior of the properties (and the PR, gh-122792: Make IPv4-mapped IPv6 address properties consistent with IPv4 #122793, doesn't attempt to change the documentation).
  3. The new behavior of the properties breaks backwards compatibility as code that already depends on the existing behavior (which I'll claim there are good reasons for) will now produce incorrect results. Breaking compatibility is fine if the old behavior is a legitimate mistake and didn't make much sense etc. but I don't see this here.
  4. The proposed change makes the API more limiting. Previously, as the user of this API, I was free to decide what to do in case of IPv4-mapped IPv6 addresses – I could unwrap them or not before checking is_link_local, for example. With this change I'm left with no API at all to do the latter.

cc @gpshead

@gpshead
Copy link
Member

gpshead commented Sep 7, 2024

I think we're lacking the motivating reason behind the change, @sethmlarson can explain more.

Agreed with (2), we need a docs update. But the merged change does not make it inconsistent, the ipaddress docs today are sparse and don't specify the exact behavior when these are used on IPv6Address. To me this change matches what I would assume they would do based on the existing terse docs. https://docs.python.org/3/library/ipaddress.html#ipaddress.IPv6Address.is_multicast

Regarding (3) / (4) do you have practical examples of actual code in use that does not want the new behavior? (got links?) That'd help reason about where this falls into breaking change vs bug fix categories.

Some general background: We've gotten security reports over the years about the ipaddress module in various places because people write code using it for security purposes and wind up surprised when it doesn't behave in the manner their specific application needed and thus maybe they did something undesirable with network traffic as a result.

@sethmlarson
Copy link
Contributor Author

@jstasiak Thanks for the question, indeed the primary motivation for this change is security, specifically folks using IP address filtering before establishing a connection. Since an IPv4-mapped IPv6 address gets established as an IPv4 address I figured these properties should match the mapped IPv4 address and not the IPv6 address that's mostly being used for framing. If you have better guidance about this case, please share!

@sethmlarson
Copy link
Contributor Author

@jstasiak Do you have an opinion on the above comment? Otherwise we will continue back-porting this change, we want to make sure we're not breaking any legitimate use-cases unintentionally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error type-security A security issue
Projects
None yet
Development

No branches or pull requests

3 participants