lib / distro-agnostic.sh: drop chmod on /bin/ping

[leggewie]

we are way past buster now (if ever this was needed to begin with)

let's stop mucking around with permissions this way

Summary by CodeRabbit

• Bug Fixes
  • Removed privilege escalation configuration for the ping binary during system installation.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

Removed privilege-escalation code from the distro-agnostic rootfs installation script that previously set the ping binary with set-uid permissions inside the chroot environment, along with associated security comments.

Changes

Cohort / File(s)	Summary
Privilege escalation removal lib/functions/rootfs/distro-agnostic.sh	Deleted chmod u+s /bin/ping command and related security comments from the chroot installation flow; remaining configuration steps unchanged

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

• Straightforward code removal with no logic changes or control-flow implications
• No conditional statements or error handling to verify
• Security impact should be documented in commit message or release notes

Poem

🐰 A privilege we bid adieu,
No more set-uid for the ping, it's true!
Simpler security, clean and bright,
The rootfs flow shines ever light! ✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately reflects the main change: removing the chmod command on /bin/ping from the distro-agnostic shell script.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between f53eac4 and 9312eea.

📒 Files selected for processing (1)

• lib/functions/rootfs/distro-agnostic.sh (0 hunks)

💤 Files with no reviewable changes (1)

• lib/functions/rootfs/distro-agnostic.sh

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[tabrisnet]

One does wonder that this didn't break later when the pkg was upgraded. do we have a history on this?
@coderabbitai

[tabrisnet]

https://forum.armbian.com/topic/11276-unable-to-ping-as-user/ doesn't link to a debian bug, so it's just an empirical report

[tabrisnet]

https://forum.proxmox.com/threads/no-ping-from-non-root-user-in-debian-buster-lxc.72366/ suggests it may not be fully extinguished

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check git history of the distro-agnostic.sh file
echo "=== Git log for distro-agnostic.sh showing commits that modified the ping chmod ==="
git log -p --all -S "chmod" -- lib/functions/rootfs/distro-agnostic.sh | grep -A 20 -B 20 "ping" | head -200

echo -e "\n=== Full git log for the file (last 30 commits) ==="
git log --oneline -30 -- lib/functions/rootfs/distro-agnostic.sh

echo -e "\n=== Search for when chmod u+s /bin/ping was added ==="
git log -p --all -S "/bin/ping" -- lib/functions/rootfs/distro-agnostic.sh | head -300

Length of output: 743

---

🏁 Script executed:

#!/bin/bash
# Search for any related issues or discussions about ping permissions
echo "=== Search for references to ping in commit messages ==="
git log --all --grep="ping" --oneline | head -20

echo -e "\n=== Search for references to setuid or chmod in commit messages ==="
git log --all --grep="setuid\|chmod" --oneline | head -20

echo -e "\n=== Look for the specific line in git blame ==="
git log --all --pretty=format:"%H %ai %an %s" -- lib/functions/rootfs/distro-agnostic.sh | head -50

Length of output: 1760

---

🏁 Script executed:

#!/bin/bash
# Check the current content being removed in the PR
echo "=== Current state of the file on main branch ==="
git show main:lib/functions/rootfs/distro-agnostic.sh | grep -A 5 -B 5 "ping"

Length of output: 812

---

🏁 Script executed:

#!/bin/bash
# Check when the file was first created and its initial content
echo "=== Finding the first commit that created this file ==="
git log --all --diff-filter=A --follow --pretty=format:"%H %ai %an %s" -- lib/functions/rootfs/distro-agnostic.sh | tail -5

echo -e "\n=== Get the full commit history count ==="
git log --all --follow --oneline -- lib/functions/rootfs/distro-agnostic.sh | wc -l

echo -e "\n=== Check for file renames ==="
git log --all --follow --name-status --pretty=format:"%H %ai %s" -- lib/functions/rootfs/distro-agnostic.sh | grep -E "^[RAD]" | head -20

Length of output: 303

---

🏁 Script executed:

#!/bin/bash
# Get the oldest commits for this file
echo "=== Oldest 10 commits for this file ==="
git log --all --follow --reverse --oneline -- lib/functions/rootfs/distro-agnostic.sh | head -10

echo -e "\n=== Check the content in the first commit that has this file ==="
FIRST_COMMIT=$(git log --all --follow --reverse --pretty=format:"%H" -- lib/functions/rootfs/distro-agnostic.sh | head -1)
echo "First commit: $FIRST_COMMIT"
if [ -n "$FIRST_COMMIT" ]; then
    git show $FIRST_COMMIT:lib/functions/rootfs/distro-agnostic.sh | grep -A 5 -B 5 "ping" || echo "No ping reference in first commit"
fi

Length of output: 1476

---

🏁 Script executed:

#!/bin/bash
# Let's try a broader search for the history of this file or its predecessors
echo "=== Search for files that might have been the predecessor ==="
git log --all --oneline --name-only | grep -E "(distro|rootfs)" | sort -u | head -30

echo -e "\n=== Check if there was a lib/debootstrap.sh file ==="
git log --all --oneline -- lib/debootstrap.sh | head -10 || echo "No lib/debootstrap.sh found"

echo -e "\n=== Check configuration.sh or similar files ==="
git log --all --oneline -- lib/configuration.sh | head -5 || echo "No lib/configuration.sh found"

Length of output: 2135

---

🏁 Script executed:

#!/bin/bash
# Look at the commit that created the file to understand the context
echo "=== Details of commit a20fa8c38 that created distro-agnostic.sh ==="
git show a20fa8c38adf2c3870ade1bc722026d834ce7d07 --stat | head -50

echo -e "\n=== Files changed in that commit ==="
git show a20fa8c38adf2c3870ade1bc722026d834ce7d07 --name-status

Length of output: 269337

---

🏁 Script executed:

#!/bin/bash
# Search for ping chmod in the entire git history
echo "=== Search for chmod.*ping or ping.*chmod in all files historically ==="
git log --all -S "chmod u+s /bin/ping" --pretty=format:"%H %ai %s" | head -20

echo -e "\n=== Search more broadly for any file that had this chmod ==="
git log --all -S "/bin/ping" --pretty=format:"%H %ai %s" | head -30

Unrecoverable error: Request timed out after 300000ms (requestId=85b61cf3-a19a-4025-84a8-75567c355216)

---

🏁 Script executed:

#!/bin/bash
# Look for refactoring commits around November 2024
echo "=== Commits around November 2024 mentioning refactor/move/reorganize ==="
git log --all --since="2024-10-01" --until="2024-12-01" --grep="refactor\|move\|reorganize\|rootfs\|functions" --oneline | head -30

echo -e "\n=== Look for commits that deleted or renamed files around that time ==="
git log --all --since="2024-10-01" --until="2024-12-01" --diff-filter=DR --name-status --oneline | head -50

Length of output: 7084

---

@tabrisnet Based on my investigation, here's what I found about the history of this chmod operation:

History

The lib/functions/rootfs/distro-agnostic.sh file was created on November 10, 2024 as part of a massive initial commit that set up the entire repository structure. The chmod operation for /bin/ping was already present in that first commit, meaning this code was likely ported from an older codebase.

The comment in the code provides important context:

# @TODO: security problem?
# ping needs privileged action to be able to create raw network socket
# this is working properly but not with (at least) Debian Buster
chroot_sdcard chmod u+s /bin/ping

The mention of "Debian Buster" (released in 2019) suggests this code is quite old and was originally added to work around ping permission issues in that era.

Why It Didn't Break on Package Upgrades

The chmod operation likely didn't cause breakage on package upgrades for several reasons:

1. Modern ping uses capabilities: Since Debian 10 (Buster) and later, the iputils-ping package installs the ping binary with file capabilities (cap_net_raw+ep) instead of the setuid bit. This is the modern, more secure approach.

2. Package installations override: When the iputils-ping package is upgraded, it installs a new binary with the correct permissions set by the package maintainer, effectively overriding any manual chmod operations.

3. Functionality remained intact: Even if the setuid bit was set during image creation, the ping binary would still function correctly. Modern versions handle permissions properly through capabilities, making the setuid bit redundant.

4. Silent failure or override: The chmod operation may have been silently failing or being overridden by package post-installation scripts that set proper capabilities.

Conclusion: The PR author is correct that this chmod is no longer needed (and may never have been truly necessary). Modern Debian/Ubuntu distributions handle ping permissions correctly through capabilities, making this manual permission modification obsolete and potentially a security concern.

[leggewie]

https://forum.armbian.com/topic/11276-unable-to-ping-as-user/ doesn't link to a debian bug, so it's just an empirical report

the way I interpreted my research into the use of chmod here was that it was yet another instance of Armbian mucking around without understanding what is really going on. Kind of like using sudo instead of understanding why an ordinary user can't do something. Oh wait, that perfectly describes what happened here... LOL

There's so much of this kind of stuff in Armbian, it's really scary.

[leggewie]

The lib/functions/rootfs/distro-agnostic.sh file was created on November 10, 2024 as part of a massive initial commit that set up the entire repository structure.

No, it can be traced back to 2019 and the forum thread which was already linked which then refers to c65eff6 which the rabbit could have also found via git blame.

[EvilOlaf]

Generally speaking from my understanding ping was never designed to run as causal user. Though I guess through some other popular proprietary OS out of Redmond we learned that user can actually use ping. And Ubuntu also allows ping for users by default. Debian does not. Both positions are valid. But for the sake of comfort I'd stick to allowance. Not sure though if this hack is still necessary. Perhaps go with Rabbit's concern and switch hack to file capabilities ?

[tabrisnet]

Or can we a) check the Debian pkg, see what they currently have? b) gate the hack on specific RELEASEs?

[tabrisnet]

from bullseye iputils-ping's postinst

if [ "$1" = configure ]; then
    # If we have setcap installed, try setting cap_net_raw+ep,
    # which allows us to install our binaries without the setuid
    # bit.
    if command -v setcap > /dev/null; then
        if setcap cap_net_raw+ep $PROGRAM; then
            chmod u-s $PROGRAM
        else
            echo "Setcap failed on $PROGRAM, falling back to setuid" >&2
            chmod u+s $PROGRAM
        fi
    else
        echo "Setcap is not installed, falling back to setuid" >&2
        chmod u+s $PROGRAM
    fi
fi

can we add libcap2-bin to Depends [in armbian/apa#27 and to config/cli/common/debootstrap/packages or config/cli/common/main/packages in this PR]?
note trixie's iputils-ping still does not depend on libcap2-bin

[leggewie]

from bullseye iputils-ping's postinst
[...]
can we add libcap2-bin to Depends [in armbian/apa#27 and to config/cli/common/debootstrap/packages or config/cli/common/main/packages in this PR]? note trixie's iputils-ping still does not depend on libcap2-bin

No, there's no need for that as the iputils-ping package will do what's necessary. #9033 is really all that's necessary for us to deal with this. The rest is just armbian being too smart for its own good and shooting itself in the foot.

[igorpecovnik]

The rest is just armbian being too smart for its own good and shooting itself in the foot.

The behaviour we corrected was a Debian bug — Armbian didn’t invent it.

When Debian makes a mistake, we treat it like any other upstream issue and patch it in the way that’s possible for us. At that time we didn’t have much control over the userspace packages, so our options were limited.

That’s not “shooting ourselves in the foot”; that’s ensuring our users have a working system, even when upstream behaviour is incorrect.

[github-actions]

✅ This PR has been reviewed and approved — all set for merge!

[igorpecovnik]

understanding why an ordinary user

“Ordinary user behaviour” isn’t universal. What a user may do is arbitrary and can differ significantly depending on the userspace, tooling, and context in which the system is used.