Skip to content

Conversation

@garethgeorge
Copy link
Owner

No description provided.

if !userExists(username) {
// Create group if it doesn't exist
if !groupExists(groupname) {
if err := createGroup(groupname, int(pgid)); err != nil {

Check failure

Code scanning / CodeQL

Incorrect conversion between integer types High

Incorrect conversion of an unsigned 32-bit integer from
strconv.ParseUint
to a lower bit size type int without an upper bound check.

Copilot Autofix

AI 6 months ago

To fix the problem, we should add explicit bounds checking after parsing the PGID value to ensure it fits within the valid range for a GID and for the int type. The typical valid range for a GID is 0 to math.MaxInt32 (2,147,483,647), since negative GIDs are not valid and the int type may be 32 bits on some systems. We should check that the parsed value is greater than 0 (or at least non-negative, depending on requirements) and less than or equal to math.MaxInt32. If the value is out of bounds, we should return an error. The same check should be applied to PUID for consistency and safety. This change should be made in the setupUserAndGroup function, after parsing puid and pgid but before converting them to int and passing them to createGroup and createUser. We will need to import the math package if it is not already imported.


Suggested changeset 1
cmd/docker-entrypoint/main.go

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/cmd/docker-entrypoint/main.go b/cmd/docker-entrypoint/main.go
--- a/cmd/docker-entrypoint/main.go
+++ b/cmd/docker-entrypoint/main.go
@@ -11,2 +11,3 @@
 	"syscall"
+	"math"
 )
@@ -86,2 +87,5 @@
 	}
+	if puid == 0 || puid > uint64(math.MaxInt32) {
+		return 0, 0, fmt.Errorf("PUID must be between 1 and %d", math.MaxInt32)
+	}
 
@@ -91,2 +95,5 @@
 	}
+	if pgid == 0 || pgid > uint64(math.MaxInt32) {
+		return 0, 0, fmt.Errorf("PGID must be between 1 and %d", math.MaxInt32)
+	}
 
EOF
@@ -11,2 +11,3 @@
"syscall"
"math"
)
@@ -86,2 +87,5 @@
}
if puid == 0 || puid > uint64(math.MaxInt32) {
return 0, 0, fmt.Errorf("PUID must be between 1 and %d", math.MaxInt32)
}

@@ -91,2 +95,5 @@
}
if pgid == 0 || pgid > uint64(math.MaxInt32) {
return 0, 0, fmt.Errorf("PGID must be between 1 and %d", math.MaxInt32)
}

Copilot is powered by AI and may make mistakes. Always verify output.
}

// Create user
if err := createUser(username, groupname, int(puid)); err != nil {

Check failure

Code scanning / CodeQL

Incorrect conversion between integer types High

Incorrect conversion of an unsigned 32-bit integer from
strconv.ParseUint
to a lower bit size type int without an upper bound check.

Copilot Autofix

AI 6 months ago

To fix this issue, we must ensure that the value parsed from the environment variable fits within the range of a signed int before converting it. The best way is to check that the parsed value is less than or equal to math.MaxInt32 (and greater than zero, if negative values are not allowed), and only then perform the conversion. If the value is out of bounds, we should return an error or use a safe default. This check should be added before any conversion of puid or pgid to int in the setupUserAndGroup function. We also need to import the math package to access math.MaxInt32.


Suggested changeset 1
cmd/docker-entrypoint/main.go

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/cmd/docker-entrypoint/main.go b/cmd/docker-entrypoint/main.go
--- a/cmd/docker-entrypoint/main.go
+++ b/cmd/docker-entrypoint/main.go
@@ -11,2 +11,3 @@
 	"syscall"
+	"math"
 )
@@ -86,2 +87,5 @@
 	}
+	if puid > uint64(math.MaxInt32) {
+		return 0, 0, fmt.Errorf("PUID %d is out of range (must be <= %d)", puid, math.MaxInt32)
+	}
 
@@ -91,2 +95,5 @@
 	}
+	if pgid > uint64(math.MaxInt32) {
+		return 0, 0, fmt.Errorf("PGID %d is out of range (must be <= %d)", pgid, math.MaxInt32)
+	}
 
@@ -112,3 +119,3 @@
 
-	return puid, pgid, nil
+	return int(puid), int(pgid), nil
 }
EOF
@@ -11,2 +11,3 @@
"syscall"
"math"
)
@@ -86,2 +87,5 @@
}
if puid > uint64(math.MaxInt32) {
return 0, 0, fmt.Errorf("PUID %d is out of range (must be <= %d)", puid, math.MaxInt32)
}

@@ -91,2 +95,5 @@
}
if pgid > uint64(math.MaxInt32) {
return 0, 0, fmt.Errorf("PGID %d is out of range (must be <= %d)", pgid, math.MaxInt32)
}

@@ -112,3 +119,3 @@

return puid, pgid, nil
return int(puid), int(pgid), nil
}
Copilot is powered by AI and may make mistakes. Always verify output.
@seb26
Copy link

seb26 commented Jan 14, 2026

I peered over this a fair bit this morning seeking a solution for rootless Backrest.

As-is this PR failed for me first on the type mismatch (as flagged by the CodeQL stuff).

Then I patched it to resolve the type issue (retain the user's UID/GID as uint64 and convert when required at last moment).

diff --git a/cmd/docker-entrypoint/main.go b/cmd/docker-entrypoint/main.go
index 11bb216..6684f46 100644
--- a/cmd/docker-entrypoint/main.go
+++ b/cmd/docker-entrypoint/main.go
@@ -77,7 +77,7 @@ func main() {
 }

 // setupUserAndGroup handles PUID/PGID setup
-func setupUserAndGroup() (int, int, error) {
+func setupUserAndGroup() (uint64, uint64, error) {
        puidStr := os.Getenv("PUID")
        pgidStr := os.Getenv("PGID")

@@ -103,14 +103,14 @@ func setupUserAndGroup() (int, int, error) {
        if !userExists(username) {
                // Create group if it doesn't exist
                if !groupExists(groupname) {
-                       if err := createGroup(groupname, int(pgid)); err != nil {
+                       if err := createGroup(groupname, pgid); err != nil {
                                return 0, 0, fmt.Errorf("failed to create group %s: %v", groupname, err)
                        }
                        os.Stderr.WriteString(fmt.Sprintf("Created group %s with GID %d\n", groupname, pgid))
                }

                // Create user
-               if err := createUser(username, groupname, int(puid)); err != nil {
+               if err := createUser(username, groupname, puid); err != nil {
                        return 0, 0, fmt.Errorf("failed to create user %s: %v", username, err)
                }
                os.Stderr.WriteString(fmt.Sprintf("Created user %s with UID %d\n", username, puid))
@@ -132,15 +132,15 @@ func groupExists(groupname string) bool {
 }

 // createGroup creates a group with the specified GID
-func createGroup(groupname string, gid int) error {
-       cmd := exec.Command("groupadd", "-g", strconv.Itoa(gid), groupname)
+func createGroup(groupname string, gid uint64) error {
+       cmd := exec.Command("groupadd", "-g", strconv.FormatUint(gid, 10), groupname)
        cmd.Stderr = os.Stderr
        return cmd.Run()
 }

 // createUser creates a user with the specified UID and group
-func createUser(username, groupname string, uid int) error {
-       cmd := exec.Command("useradd", "-m", "-u", strconv.Itoa(uid), "-g", groupname, username)
+func createUser(username, groupname string, uid uint64) error {
+       cmd := exec.Command("useradd", "-m", "-u", strconv.FormatUint(uid, 10), "-g", groupname, username)
        cmd.Stderr = os.Stderr
        return cmd.Run()
 }

Then the PR failed because useradd was not found in the container - Alpine uses adduser instead.

But taking a step back for a moment and trying to see what this does...

cmd/docker-entrypoint/main.go is a bit convoluted as it stands. I'm not sure I see the justification for golang compared to shell for an entrypoint?

The UID/GID from the user is changed type a bunch of times (as mentioned), and then validated with a check for 0-65536, and then ultimately wrapper go funcs createUser() createGroup() spawn shell processes in order to call groupadd and useradd with string arguments anyway?

A bash entrypoint would be a lot simpler.

And then if user provides invalid UID it will fail with obvious error message without needing to waste effort validating inside go and providing go error message?

And then also one less golang build step for the building of the entrypoint...

Right now an entrypoint needs to:

  • set some defaults for envs
  • read the UID/GID
  • idempotently create the user/group
  • execute backrest

And it does not presently:

  • API link to backrest backend via go module
  • Or other clear reason (to me) where being in golang would be advantageous

A note that alpine and scratch (the supported container bases at the moment) have competing approaches to user creation. Alpine includes adduser and scratch does not, it is rootless and single user by design.

I started focusing only on Alpine so the above suggestion may need to be different for scratch.

A proposed entrypoint would look like this:

#!/bin/sh
set -e

: "${BACKREST_PORT:=0.0.0.0:9898}"
: "${BACKREST_DATA:=/data}"
: "${BACKREST_CONFIG:=/config/config.json}"
: "${XDG_CACHE_HOME:=/cache}"
: "${PUID:=0}"
: "${PGID:=$PUID}"

export BACKREST_PORT BACKREST_DATA BACKREST_CONFIG XDG_CACHE_HOME

echo "Using environment variables:"
echo " BACKREST_PORT=$BACKREST_PORT"
echo " BACKREST_DATA=$BACKREST_DATA"
echo " BACKREST_CONFIG=$BACKREST_CONFIG"
echo " XDG_CACHE_HOME=$XDG_CACHE_HOME"
echo " PUID=$PUID"
echo " PGID=$PGID"

if [ "$PUID" -ne 0 ]; then
    if ! getent group backrest >/dev/null; then
        addgroup -g "$PGID" -S backrest
    fi

    if ! id -u backrest >/dev/null 2>&1; then
        adduser -u "$PUID" -S -G backrest -D -H backrest
    fi

    chown -R backrest:backrest "$BACKREST_DATA" /config "$XDG_CACHE_HOME" /tmp
    exec su-exec backrest "$@"
fi

exec "$@"

And a corresponding Dockerfile section (not shown is my build steps which don't use gorelease):

FROM alpine:3.23
RUN apk --no-cache add \
    bash \
    ca-certificates \
    curl \
    rclone \
    restic \
    tini \
    tzdata \
    su-exec
COPY --from=builder /src/backrest /usr/bin/backrest
RUN mkdir -p /data /config /cache /tmp
COPY ./docker-entrypoint.sh /docker-entrypoint
RUN chmod +x /docker-entrypoint
ENTRYPOINT ["/sbin/tini", "--", "/docker-entrypoint"]
CMD ["/usr/bin/backrest"]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants