Skip to content

Detect weird local ips - #34160

Merged
PVince81 merged 7 commits into
masterfrom
fix/detect-weird-local-ips
Sep 22, 2022
Merged

Detect weird local ips#34160
PVince81 merged 7 commits into
masterfrom
fix/detect-weird-local-ips

Conversation

@come-nc

@come-nccome-nc commented Sep 20, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

Use new dependency to detect weird syntaxes

@come-nc
Harden tests for local IP detection in URLs
c083f77
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
Add missing urldecode and idn_to_utf8 calls to local address checker
a907b74
The call to idn_to_utf8 call is actually to apply normalization
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
Add mlocati/ip-lib dependency
6b876bb
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
Use new dependency to normalize IPs
7ac688a
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
Fix tests for nested v4 in v6
31117fa
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nccome-nc added the 3. to review Waiting for reviews label Sep 20, 2022
@come-nccome-nc added this to the Nextcloud 25 milestone Sep 20, 2022
@come-nccome-nc self-assigned this Sep 20, 2022
@come-nc
come-nc requested review from a team, ChristophWurst, blizzz, icewind1991, nickvergessen and skjnldsv and removed request for a teamSeptember 20, 2022 10:48
github-advanced-security[bot]
github-advanced-securityAI found potential problems Sep 20, 2022
View reviewed changes
Comment threadlib/private/Http/Client/DnsPinMiddleware.php Fixed
Comment threadlib/private/Http/Client/LocalAddressChecker.php Fixed
@kesselb

kesselb commented Sep 20, 2022

Copy link
Copy Markdown
Contributor

Thanks for taking care 👍

In theory we can drop the iputils (symfony http-foundation) again and only use the ip-lib package: https://github.com/mlocati/ip-lib#check-if-an-address-is-contained-in-a-range

This could also help with #33567

@blizzzblizzz mentioned this pull request Sep 20, 2022
Closed
@come-nc

come-nc commented Sep 20, 2022

Copy link
Copy Markdown
ContributorAuthor

Thanks for taking care +1

In theory we can drop the iputils (symfony http-foundation) again and only use the ip-lib package: https://github.com/mlocati/ip-lib#check-if-an-address-is-contained-in-a-range

This could also help with #33567

Hum but I trust the range detection in symfony more, especially it correctly tests ipv6 in ipv4 ranges, which ip-lib does not.
But as our code translates the IP to v4 if it maps, maybe it is not a problem.

@kesselb

kesselb commented Sep 20, 2022

Copy link
Copy Markdown
Contributor

Thanks for taking care +1
In theory we can drop the iputils (symfony http-foundation) again and only use the ip-lib package: https://github.com/mlocati/ip-lib#check-if-an-address-is-contained-in-a-range
This could also help with #33567

Hum but I trust the range detection in symfony more, especially it correctly tests ipv6 in ipv4 ranges, which ip-lib does not. But as our code translates the IP to v4 if it maps, maybe it is not a problem.

Good point 👍

@come-nc

come-nc commented Sep 20, 2022

Copy link
Copy Markdown
ContributorAuthor

@kesselb I can make a PR to use ip-lib instead of iputils but I’d like to do so in a separate PR once this one is merged.

@nickvergessen

nickvergessen commented Sep 20, 2022

Copy link
Copy Markdown
Member

3rdparty merged

@kesselb

kesselb commented Sep 20, 2022

Copy link
Copy Markdown
Contributor

@kesselb I can make a PR to use ip-lib instead of iputils but I’d like to do so in a separate PR once this one is merged.

Sorry I was just thinking loud. The current way is fine and we can combine both libraries.

@come-nc
Update 3rdparty to master
b3b98a3
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc

come-nc commented Sep 20, 2022

Copy link
Copy Markdown
ContributorAuthor

/backport to stable24

@come-nc

come-nc commented Sep 20, 2022

Copy link
Copy Markdown
ContributorAuthor

/backport to stable23

@nickvergessen

nickvergessen commented Sep 21, 2022

Copy link
Copy Markdown
Member

/backport to stable22

@come-nc
Fix idn_to_utf8 stub signature
7532859
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@blizzzblizzz mentioned this pull request Sep 22, 2022
Merged
2 tasks
@PVince81

PVince81 commented Sep 22, 2022

Copy link
Copy Markdown
Member

/backport to stable25

PVince81
PVince81 approved these changes Sep 22, 2022
View reviewed changes

@PVince81PVince81 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

@PVince81
PVince81 merged commit 2f15e83 into masterSep 22, 2022
@PVince81
PVince81 deleted the fix/detect-weird-local-ips branch September 22, 2022 09:38
@blizzzblizzz modified the milestones: Nextcloud 25, Nextcloud 26Sep 22, 2022
@blizzz

blizzz commented Sep 22, 2022

Copy link
Copy Markdown
Member

/backport to stable25

@come-nccome-nc mentioned this pull request Sep 22, 2022
Merged
This was referenced Nov 14, 2022
Merged
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nickvergessennickvergessennickvergessen approved these changes
@PVince81PVince81PVince81 approved these changes
@ChristophWurstChristophWurstAwaiting requested review from ChristophWurst
@icewind1991icewind1991Awaiting requested review from icewind1991
@blizzzblizzzAwaiting requested review from blizzz
@skjnldsvskjnldsvAwaiting requested review from skjnldsv
@CarlSchwanCarlSchwanAwaiting requested review from CarlSchwan

Assignees

@come-nccome-nc

Labels

3. to reviewWaiting for reviews

Projects

None yet

Milestone

Nextcloud 26

Development

Successfully merging this pull request may close these issues.

6 participants

@come-nc@kesselb@nickvergessen@PVince81@blizzz@github-advanced-security