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

Adds support for Unique Local Addresses #2856

Closed
wants to merge 1 commit into from

Conversation

n-thumann
Copy link

As suggested in #399, this PR adds support for detecting Unique Local Addresses.

@codecov
Copy link

codecov bot commented Oct 7, 2020

Codecov Report

Merging #2856 into master will increase coverage by 0.03%.
The diff coverage is 93.33%.

@@            Coverage Diff             @@
##           master    #2856      +/-   ##
==========================================
+ Coverage   88.15%   88.19%   +0.03%     
==========================================
  Files         254      254              
  Lines       54240    54246       +6     
==========================================
+ Hits        47817    47841      +24     
+ Misses       6423     6405      -18     
Impacted Files Coverage Δ
scapy/utils6.py 87.13% <92.85%> (-0.09%) ⬇️
scapy/data.py 91.79% <100.00%> (+0.02%) ⬆️
scapy/arch/windows/__init__.py 68.50% <0.00%> (ø)
scapy/layers/inet.py 74.58% <0.00%> (+0.15%) ⬆️
scapy/layers/http.py 87.88% <0.00%> (+0.31%) ⬆️
scapy/themes.py 80.91% <0.00%> (+0.70%) ⬆️
scapy/arch/windows/native.py 80.00% <0.00%> (+1.81%) ⬆️
scapy/autorun.py 78.08% <0.00%> (+4.10%) ⬆️
scapy/pton_ntop.py 97.26% <0.00%> (+8.21%) ⬆️

@gpotter2 gpotter2 requested a review from guedou October 8, 2020 06:57
@n-thumann n-thumann marked this pull request as ready for review October 9, 2020 07:34
gpotter2
gpotter2 previously approved these changes Oct 9, 2020
Copy link
Member

@gpotter2 gpotter2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll let @guedou review it but it looks fine to me ! Thanks for the PR

@gpotter2 gpotter2 dismissed their stale review October 9, 2020 09:32

Bad move

Copy link
Member

@guedou guedou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this really difficult PR! I think that only in6_getAddrType() should be changed.

@@ -783,12 +779,14 @@ def in6_getscope(addr):
"""
Returns the scope of the address.
"""
if in6_isgladdr(addr) or in6_isuladdr(addr):
if in6_isgladdr(addr):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to https://tools.ietf.org/html/rfc4193#section-3.3 the scope is global. This function should not be modified.

addrType = (IPV6_ADDR_UNICAST | IPV6_ADDR_GLOBAL)
if naddr[:2] == b' \x02': # Mark 6to4 @
if in6_isaddr6to4(addr): # Mark 6to4
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please replace @ with address instead of removing it.

@@ -164,29 +167,27 @@ def rfc3484_cmp(source_a, source_b):
return candidate_set[0]


# Think before modify it : for instance, FE::1 does exist and is unicast
# there are many others like that.
# TODO : integrate Unique Local Addresses
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not remove the other comments.

addrType = (IPV6_ADDR_UNICAST | IPV6_ADDR_LINKLOCAL)
elif in6_isuladdr(addr):
addrType = (IPV6_ADDR_UNICAST | IPV6_ADDR_UNIQUE_LOCAL)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should also be IPV6_ADDR_GLOBAL

@@ -59,12 +60,14 @@ def cset_sort(x, y):
return -res

cset = []
if in6_isgladdr(addr) or in6_isuladdr(addr):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not convinced that this change is correct. Did you test it on Linux with unique local addresses? It seems that the Linux kernel does not specify IPV6_ADDR_UNIQUE_LOCAL. Therefore, this patch will likely break address selection.

@gpotter2 gpotter2 added this to the 2.5.0 milestone Mar 29, 2021
@guedou
Copy link
Member

guedou commented Sep 12, 2021

I am closing this PR as we did not receive an answer, and that I doubt that it works correctly.

@guedou guedou closed this Sep 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants