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

(#2037807) Allow unprivileged users to send ICMP echo requests #256

Merged
merged 8 commits into from Feb 7, 2022
Merged

(#2037807) Allow unprivileged users to send ICMP echo requests #256

merged 8 commits into from Feb 7, 2022

Conversation

jamacku
Copy link
Member

@jamacku jamacku commented Jan 21, 2022

Upstream PR systemd/systemd#13191

Follow-up to #246

Resolves: #2037807

@jamacku jamacku added this to the RHEL-8.6 milestone Jan 21, 2022
@systemd-rhel-bot systemd-rhel-bot added pr/needs-ci Formerly needs-ci pr/needs-review Formerly needs-review labels Jan 21, 2022
@systemd-rhel-bot systemd-rhel-bot changed the title Allow unprivileged users to send ICMP echo requests (#2037807) Allow unprivileged users to send ICMP echo requests Jan 21, 2022
@mrc0mmand
Copy link
Member

CI seems to be happy, I gave systemd-sysctl a brief check with ASan+UBsan (since the original backport caused a memory corruption) and that's sorted out as well. Also, ping is now available to unprivileged users again.

Old version:

# sysctl -w net.ipv4.ping_group_range="1 0"
net.ipv4.ping_group_range = 1 0
# sudo -u test ping localhost
ping: socket: Operation not permitted
# /usr/lib/systemd/systemd-sysctl 
Couldn't write '0 2147483647' to '-net/ipv4/ping_group_range', ignoring: No such file or directory
# sysctl net.ipv4.ping_group_range
net.ipv4.ping_group_range = 1	0
# sudo -u test ping localhost
ping: socket: Operation not permitted

New version:

# sysctl -w net.ipv4.ping_group_range="1 0"
net.ipv4.ping_group_range = 1 0
# sudo -u test ping localhost
ping: socket: Operation not permitted
# /usr/lib/systemd/systemd-sysctl
# sysctl net.ipv4.ping_group_range
net.ipv4.ping_group_range = 0	2147483647
# sudo -u test ping localhost
PING localhost(localhost (::1)) 56 data bytes
64 bytes from localhost (::1): icmp_seq=1 ttl=64 time=0.046 ms
...

@mrc0mmand
Copy link
Member

mrc0mmand commented Jan 21, 2022

And just for completeness, the "ignore" mechanic seems to work as well:

# echo "fs.protected_symlinks = 9999" >>/usr/lib/sysctl.d/50-default.conf
# /usr/lib/systemd/systemd-sysctl; echo $?
Couldn't write '9999' to 'fs/protected_symlinks': Invalid argument
1

# echo "-fs.protected_symlinks = 9999" >>/usr/lib/sysctl.d/50-default.conf
# /usr/lib/systemd/systemd-sysctl; echo $?
Couldn't write '9999' to 'fs/protected_symlinks', ignoring: Invalid argument
0

@systemd-rhel-bot systemd-rhel-bot removed the pr/needs-ci Formerly needs-ci label Jan 21, 2022
@jamacku jamacku requested a review from dtardon January 21, 2022 13:58
src/sysctl/sysctl.c Show resolved Hide resolved
@systemd-rhel-bot systemd-rhel-bot removed the pr/needs-review Formerly needs-review label Jan 24, 2022
yuwata and others added 7 commits January 24, 2022 13:06
(cherry picked from commit d1005d1)

Resolves: #2037807
If they are set, then they are called in hashmap_clear() or
hashmap_free().

(cherry picked from commit 59a5cda)

Resolves: #2037807
(cherry picked from commit e30f9c9)

Resolves: #2037807
(cherry picked from commit 25073e5)

Resolves: #2037807
(cherry picked from commit 98233ee)

Resolves: #2037807
(cherry picked from commit e08be64)

Related: #2037807
(cherry picked from commit dec02d6)

Resolves: #2037807
@jamacku jamacku requested a review from dtardon January 24, 2022 12:09
@systemd-rhel-bot systemd-rhel-bot added pr/needs-ci Formerly needs-ci and removed pr/needs-ci Formerly needs-ci labels Jan 24, 2022
Copy link
Member

@msekletar msekletar left a comment

Choose a reason for hiding this comment

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

LGTM but before I approve...

@jamacku @mrc0mmand please triple-check there were no further follow-up bugfixes for these reworks. Hashmaps are in critical path and we must be 100% sure it is correct.

@mrc0mmand
Copy link
Member

mrc0mmand commented Jan 25, 2022

LGTM but before I approve...

@jamacku @mrc0mmand please triple-check there were no further follow-up bugfixes for these reworks. Hashmaps are in critical path and we must be 100% sure it is correct.

I went through the git log of each modified file:

  • path_compare_func() is not used anywhere and could be safely dropped (was done in 6906ac9), but that shouldn't matter
  • there's a memory leak in the path cache rebuilding... path, which is unrelated to this PR, but should be fixed nonetheless in the future (https://github.com/systemd/systemd/pull/15954/commits)
  • there's c380b84 (and follow-up ca32371) which look like a good-to-have commits, so I pulled them in as well
    • nevermind, that would require a couple of other commits to be pulled in as well, not worth it imo
  • db99904 is definitely something we should have; backported

That should be, hopefully, it.

@systemd-rhel-bot systemd-rhel-bot added the pr/needs-ci Formerly needs-ci label Jan 25, 2022
Fixes #14801.

(cherry picked from commit db99904)

Resolves: #2037807
@systemd-rhel-bot systemd-rhel-bot removed the pr/needs-ci Formerly needs-ci label Jan 25, 2022
@mrc0mmand
Copy link
Member

We should merge this soon, since more and more uses are hitting the issue - @msekletar PTAL.

Copy link
Member

@msekletar msekletar left a comment

Choose a reason for hiding this comment

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

LGTM

@systemd-rhel-bot systemd-rhel-bot merged commit b30c37b into redhat-plumbers:master Feb 7, 2022
@jamacku jamacku deleted the bz2037807-unprivileged-users-cant-ping branch February 7, 2022 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants