fix(security): address CodeQL alerts for integer overflow etc.#132
fix(security): address CodeQL alerts for integer overflow etc.#132
Conversation
…jection - fileDirectory: cast uid/gid through uint32 before int to prevent overflow on 32-bit platforms (CodeQL #48-51) - pki: add IsValidSproutID checks to AcceptNKey, DeleteNKey, DenyNKey, UnacceptNKey, RejectNKey for consistent path validation (CodeQL #35-40, #47) - pki: document intentional InsecureSkipVerify in TLS bootstrap (CodeQL #34)
| notes = append(notes, cook.Snprintf("would chown %s to %s", name, d.user)) | ||
| } else { | ||
| chownErr := os.Chown(name, int(uid), -1) | ||
| chownErr := os.Chown(name, int(uint32(uid)), -1) |
Check failure
Code scanning / CodeQL
Incorrect conversion between integer types High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 days ago
In general, to fix this kind of issue you should avoid converting a parsed integer to a smaller or differently signed type without checking bounds, or avoid the intermediate type (int) entirely and use a type whose size and sign you control (int32, int64, etc.). For UID/GID operations in Go, os.Chown accepts int arguments, but system UID/GID values are non‑negative and typically within 32 bits, so using a consistent signed 32‑bit or 64‑bit representation is sufficient.
The cleanest fix here is to avoid parsing as uint and then converting through int. Instead, parse the UID as a signed 32‑bit integer using strconv.ParseInt with bit size 32, and pass that int32 (or its int equivalent) to os.Chown. This both expresses the intended range and avoids unsigned‑to‑signed confusion. Specifically:
- Change line 84 to use
strconv.ParseInt(userL.Uid, 10, 32)returning a signedint64. - Immediately convert that to
int32, and then consistently use either:int32and cast tointat the call site (int(uid32)), or- keep it as
intonce, derived fromint32(uid := int(uid32)).
Because the bitSize argument to ParseInt is 32, any parsed value is guaranteed to be in the int32 range; if the string encodes something outside that range, ParseInt will return an error, which you already handle. This removes the path CodeQL is complaining about and keeps behavior equivalent for all valid UIDs.
Below I parse into a signed 32‑bit value and then pass it directly as int to os.Chown, minimizing changes while eliminating the unsigned‑to‑signed cast pattern.
| @@ -81,17 +81,18 @@ | ||
| Succeeded: false, Failed: true, Notes: notes, | ||
| }, lookupErr | ||
| } | ||
| uid, parseErr := strconv.ParseUint(userL.Uid, 10, 32) | ||
| uid64, parseErr := strconv.ParseInt(userL.Uid, 10, 32) | ||
| if parseErr != nil { | ||
| return cook.Result{ | ||
| Succeeded: false, Failed: true, Notes: notes, | ||
| }, parseErr | ||
| } | ||
| uid := int32(uid64) | ||
|
|
||
| if test { | ||
| notes = append(notes, cook.Snprintf("would chown %s to %s", name, d.user)) | ||
| } else { | ||
| chownErr := os.Chown(name, int(uint32(uid)), -1) | ||
| chownErr := os.Chown(name, int(uid), -1) | ||
| if chownErr != nil { | ||
| return cook.Result{ | ||
| Succeeded: false, Failed: true, Notes: notes, | ||
| @@ -106,7 +98,7 @@ | ||
| return nil | ||
| } | ||
| notes = append(notes, cook.Snprintf("chown %s to %s", name, val)) | ||
| return os.Chown(path, int(uint32(uid)), -1) | ||
| return os.Chown(path, int(uid), -1) | ||
| }) | ||
| if walkErr != nil { | ||
| return cook.Result{ |
| } | ||
| notes = append(notes, cook.Snprintf("chown %s to %s", name, val)) | ||
| return os.Chown(path, int(uid), -1) | ||
| return os.Chown(path, int(uint32(uid)), -1) |
Check failure
Code scanning / CodeQL
Incorrect conversion between integer types High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 days ago
In general, to fix this kind of issue you should avoid parsing into a wider or unsigned integer type and then casting down to a smaller or signed type without bounds checks. Instead, either parse directly into the exact bit size and signedness you need, or enforce explicit upper/lower bounds before converting.
Here, os.Chown expects an int UID, and the code currently parses into uint64 with a 32‑bit limit (ParseUint(..., 10, 32)), then casts via uint32 to int. The cleanest fix without changing behaviour is to parse the UID as a signed 32‑bit integer using strconv.ParseInt(..., 10, 32) and then convert that directly to int for os.Chown. This matches the typical domain of UIDs (non‑negative values that fit in 32 bits) and removes the unsigned‑to‑signed narrowing conversion that CodeQL flags.
Concretely in internal/ingredients/file/fileDirectory.go:
- Change the UID parsing line (84) from
strconv.ParseUint(..., 10, 32)tostrconv.ParseInt(..., 10, 32)and rename the type accordingly (uidbecomes anint64constrained to 32 bits). - Change the two call sites where
uidis used (lines 94 and 109) fromint(uint32(uid))toint(uid), which is safe on 32‑bit/64‑bit platforms given the 32‑bit bound inParseInt. - No new imports are needed;
strconvis already imported. No changes to other logic or parameters are required.
| @@ -81,7 +81,7 @@ | ||
| Succeeded: false, Failed: true, Notes: notes, | ||
| }, lookupErr | ||
| } | ||
| uid, parseErr := strconv.ParseUint(userL.Uid, 10, 32) | ||
| uid, parseErr := strconv.ParseInt(userL.Uid, 10, 32) | ||
| if parseErr != nil { | ||
| return cook.Result{ | ||
| Succeeded: false, Failed: true, Notes: notes, | ||
| @@ -91,7 +91,7 @@ | ||
| if test { | ||
| notes = append(notes, cook.Snprintf("would chown %s to %s", name, d.user)) | ||
| } else { | ||
| chownErr := os.Chown(name, int(uint32(uid)), -1) | ||
| chownErr := os.Chown(name, int(uid), -1) | ||
| if chownErr != nil { | ||
| return cook.Result{ | ||
| Succeeded: false, Failed: true, Notes: notes, | ||
| @@ -106,7 +106,7 @@ | ||
| return nil | ||
| } | ||
| notes = append(notes, cook.Snprintf("chown %s to %s", name, val)) | ||
| return os.Chown(path, int(uint32(uid)), -1) | ||
| return os.Chown(path, int(uid), -1) | ||
| }) | ||
| if walkErr != nil { | ||
| return cook.Result{ |
| notes = append(notes, cook.Snprintf("would chown %s to %s", name, d.group)) | ||
| } else { | ||
| chownErr := os.Chown(name, -1, int(gid)) | ||
| chownErr := os.Chown(name, -1, int(uint32(gid))) |
Check failure
Code scanning / CodeQL
Incorrect conversion between integer types High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 days ago
In general, to fix this type of issue you must ensure that the integer value you parsed is within the valid range of the destination type before casting. Here, you already parse with a 32‑bit limit (ParseUint(..., 32)), so you now just need to ensure the resulting value fits into a signed int on the target architecture, and then cast directly to int without unnecessary intermediate unsigned casts.
The best minimal fix, without changing existing functionality, is:
- After
gid, parseErr := strconv.ParseUint(group.Gid, 10, 32), add a bounds check that ensuresgiddoes not exceedmath.MaxInt(the maximum value of typeinton the current platform). - If the value is out of range, return an error instead of calling
os.Chown. - Replace
int(uint32(gid))andint(uint32(gid))in bothos.Chowncalls withint(gid), relying on the previous bounds check to guarantee safety.
This only requires importing the math package in internal/ingredients/file/fileDirectory.go and adding a few lines near the existing parsing logic.
Concretely:
-
Edit
internal/ingredients/file/fileDirectory.goto add"math"to the import list. -
After the existing
ParseUintcall and error check, insert:if gid > uint64(math.MaxInt) { return cook.Result{ Succeeded: false, Failed: true, Notes: notes, }, fmt.Errorf("group ID %s overflows int", group.Gid) }
-
Change
os.Chown(name, -1, int(uint32(gid)))toos.Chown(name, -1, int(gid)). -
Change
os.Chown(path, -1, int(uint32(gid)))in theWalkDircallback toos.Chown(path, -1, int(gid)).
This preserves behavior for all in-range group IDs and fails cleanly with an error if an out-of-range ID is encountered.
| @@ -4,6 +4,7 @@ | ||
| "context" | ||
| "fmt" | ||
| "io/fs" | ||
| "math" | ||
| "os" | ||
| "os/user" | ||
| "path/filepath" | ||
| @@ -132,10 +133,15 @@ | ||
| Succeeded: false, Failed: true, Notes: notes, | ||
| }, parseErr | ||
| } | ||
| if gid > uint64(math.MaxInt) { | ||
| return cook.Result{ | ||
| Succeeded: false, Failed: true, Notes: notes, | ||
| }, fmt.Errorf("group ID %s overflows int", group.Gid) | ||
| } | ||
| if test { | ||
| notes = append(notes, cook.Snprintf("would chown %s to %s", name, d.group)) | ||
| } else { | ||
| chownErr := os.Chown(name, -1, int(uint32(gid))) | ||
| chownErr := os.Chown(name, -1, int(gid)) | ||
| if chownErr != nil { | ||
| return cook.Result{ | ||
| Succeeded: false, Failed: true, Notes: notes, | ||
| @@ -150,7 +153,7 @@ | ||
| return nil | ||
| } | ||
| notes = append(notes, cook.Snprintf("chown %s to %s", name, val)) | ||
| return os.Chown(path, -1, int(uint32(gid))) | ||
| return os.Chown(path, -1, int(gid)) | ||
| }) | ||
| if walkErr != nil { | ||
| return cook.Result{ |
| } | ||
| notes = append(notes, cook.Snprintf("chown %s to %s", name, val)) | ||
| return os.Chown(path, -1, int(gid)) | ||
| return os.Chown(path, -1, int(uint32(gid))) |
Check failure
Code scanning / CodeQL
Incorrect conversion between integer types High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 days ago
In general, to fix this class of issues you must avoid converting from a wider integer type to a narrower one without checking that the value lies within the target type’s bounds. For parsed values, either (a) parse directly to the right bit size for the target type or (b) ensure the parsed value is within the target type’s min/max range before converting.
In this specific case, gid is the result of strconv.ParseUint(group.Gid, 10, 32), so it is guaranteed to be within the range of an unsigned 32‑bit integer. It is then converted to int (potentially narrower) for os.Chown. The most precise, behavior‑preserving fix is:
- After parsing
gid, compute anintGIDvariable by:- Ensuring
giddoes not exceedmath.MaxInt(the maximum forinton the running platform). - Casting directly from
uint64toint(no need to go viauint32).
- Ensuring
- Use this checked
intGIDin all subsequentos.Chowncalls in this block, instead of repeatedly convertinggidinline.
This requires importing math and adding a small bounds check. Functionality is unchanged for all valid group IDs that fit in int; for too‑large IDs the function will now return an error instead of silently wrapping.
Concretely:
- In
internal/ingredients/file/fileDirectory.go:- Extend the import block to include
"math". - Immediately after verifying
parseErr == nilforgid, add code to:- Compare
gidtouint64(math.MaxInt)and return an error if it is larger. - Set
intGID := int(gid).
- Compare
- Replace both
os.Chown(..., int(uint32(gid)))calls (lines 138 and 153) withos.Chown(..., intGID).
- Extend the import block to include
| @@ -4,6 +4,7 @@ | ||
| "context" | ||
| "fmt" | ||
| "io/fs" | ||
| "math" | ||
| "os" | ||
| "os/user" | ||
| "path/filepath" | ||
| @@ -132,10 +133,16 @@ | ||
| Succeeded: false, Failed: true, Notes: notes, | ||
| }, parseErr | ||
| } | ||
| if gid > uint64(math.MaxInt) { | ||
| return cook.Result{ | ||
| Succeeded: false, Failed: true, Notes: notes, | ||
| }, fmt.Errorf("group GID %q overflows int", group.Gid) | ||
| } | ||
| intGID := int(gid) | ||
| if test { | ||
| notes = append(notes, cook.Snprintf("would chown %s to %s", name, d.group)) | ||
| } else { | ||
| chownErr := os.Chown(name, -1, int(uint32(gid))) | ||
| chownErr := os.Chown(name, -1, intGID) | ||
| if chownErr != nil { | ||
| return cook.Result{ | ||
| Succeeded: false, Failed: true, Notes: notes, | ||
| @@ -150,7 +154,7 @@ | ||
| return nil | ||
| } | ||
| notes = append(notes, cook.Snprintf("chown %s to %s", name, val)) | ||
| return os.Chown(path, -1, int(uint32(gid))) | ||
| return os.Chown(path, -1, intGID) | ||
| }) | ||
| if walkErr != nil { | ||
| return cook.Result{ |
No description provided.