Skip to content

Commit f6ee62d

Browse files
elvin03elvin03
authored
Merge pull request #381 from elvin03/main
Add fix for codeql security issue
2 parents 577993b + 5820ac0 commit f6ee62d

6 files changed

Lines changed: 122 additions & 32 deletions

File tree

‎cmd/os-image-composer/build.go‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,9 @@ post:
126126
if buildErr == nil {
127127
log.Info("image build completed successfully")
128128
} else {
129-
log.Errorf("image build failed: %v", buildErr)
129+
// Avoid logging the full error chain to prevent potential leakage of sensitive data.
130+
// Log only the error type/category to aid debugging without exposing sensitive details.
131+
log.Errorf("image build failed (error type: %T)", buildErr)
130132
}
131133

132134
return buildErr

‎internal/config/merge.go‎

Lines changed: 48 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -137,13 +137,14 @@ func MergeConfigurations(userTemplate, defaultTemplate *ImageTemplate) (*ImageTe
137137
// Validate immutability configuration and fix if needed
138138
validateAndFixImmutabilityConfig(&mergedTemplate)
139139

140-
// Debug mode: Pretty print the merged template
140+
// Debug mode: Pretty print the merged template with sensitive data redacted
141141
if IsDebugMode() {
142-
pretty, err := json.MarshalIndent(mergedTemplate, "", " ")
142+
redactedTemplate := redactSensitiveData(&mergedTemplate)
143+
pretty, err := json.MarshalIndent(redactedTemplate, "", " ")
143144
if err != nil {
144145
log.Warnf("Failed to pretty print merged template: %v", err)
145146
} else {
146-
log.Debugf("Merged Template:\n%s", string(pretty))
147+
log.Debugf("Merged Template (sensitive data redacted):\n%s", string(pretty))
147148
}
148149
}
149150

@@ -153,6 +154,50 @@ func MergeConfigurations(userTemplate, defaultTemplate *ImageTemplate) (*ImageTe
153154
return &mergedTemplate, nil
154155
}
155156

157+
// redactSensitiveData creates a copy of the template with sensitive data redacted for safe logging.
158+
// This prevents passwords, keys, and other sensitive information from appearing in logs.
159+
func redactSensitiveData(template *ImageTemplate) *ImageTemplate {
160+
// Create a deep copy
161+
redacted := *template
162+
redacted.SystemConfig = redactSensitiveSystemConfig(template.SystemConfig)
163+
return &redacted
164+
}
165+
166+
// redactSensitiveSystemConfig creates a copy of SystemConfig with sensitive fields redacted
167+
func redactSensitiveSystemConfig(config SystemConfig) SystemConfig {
168+
redacted := config
169+
170+
// Redact user passwords and sensitive user data
171+
if len(config.Users) > 0 {
172+
redacted.Users = make([]UserConfig, len(config.Users))
173+
for i, user := range config.Users {
174+
redactedUser := user
175+
// Redact password if present
176+
if user.Password != "" {
177+
redactedUser.Password = "[REDACTED]"
178+
}
179+
// Redact hash algorithm to prevent revealing password security details
180+
if user.HashAlgo != "" {
181+
redactedUser.HashAlgo = "[REDACTED]"
182+
}
183+
redacted.Users[i] = redactedUser
184+
}
185+
}
186+
187+
// Redact secure boot keys/certificates (sensitive file paths that could reveal security setup)
188+
if config.Immutability.SecureBootDBKey != "" {
189+
redacted.Immutability.SecureBootDBKey = "[REDACTED]"
190+
}
191+
if config.Immutability.SecureBootDBCrt != "" {
192+
redacted.Immutability.SecureBootDBCrt = "[REDACTED]"
193+
}
194+
if config.Immutability.SecureBootDBCer != "" {
195+
redacted.Immutability.SecureBootDBCer = "[REDACTED]"
196+
}
197+
198+
return redacted
199+
}
200+
156201
// mergeSystemConfig merges a single system configuration
157202
func mergeSystemConfig(defaultConfig, userConfig SystemConfig) SystemConfig {
158203
merged := defaultConfig // Start with default

‎internal/image/imageos/imageos.go‎

Lines changed: 35 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1351,21 +1351,31 @@ func verifyUserCreated(installRoot, username string) error {
13511351

13521352
// Check if user exists in passwd file
13531353
passwdCmd := fmt.Sprintf("grep '^%s:' /etc/passwd", username)
1354-
output, err := shell.ExecCmd(passwdCmd, true, installRoot, nil)
1354+
// output, err := shell.ExecCmd(passwdCmd, true, installRoot, nil)
1355+
_, err := shell.ExecCmd(passwdCmd, true, installRoot, nil)
13551356
if err != nil {
1356-
log.Errorf("User %s not found in passwd file: %v", username, err)
1357-
return fmt.Errorf("user %s not found in passwd file: %w", username, err)
1357+
// log.Errorf("User %s not found in passwd file: %v", username, err)
1358+
// return fmt.Errorf("user %s not found in passwd file: %w", username, err)
1359+
// Do not log command output or sensitive file contents
1360+
log.Errorf("User %s not found in passwd file", username)
1361+
return fmt.Errorf("user %s not found in passwd file", username)
13581362
}
1359-
log.Debugf("User in passwd: %s", strings.TrimSpace(output))
1363+
// log.Debugf("User in passwd: %s", strings.TrimSpace(output))
1364+
// User was found in passwd; avoid logging the line content to prevent leaking sensitive data
13601365

13611366
// Check if user has password in shadow file
13621367
shadowCmd := fmt.Sprintf("grep '^%s:' /etc/shadow", username)
1363-
output, err = shell.ExecCmd(shadowCmd, true, installRoot, nil)
1368+
// output, err = shell.ExecCmd(shadowCmd, true, installRoot, nil)
1369+
_, err = shell.ExecCmd(shadowCmd, true, installRoot, nil)
13641370
if err != nil {
1365-
log.Errorf("User %s not found in shadow file: %v", username, err)
1366-
return fmt.Errorf("user %s not found in shadow file: %w", username, err)
1371+
// log.Errorf("User %s not found in shadow file: %v", username, err)
1372+
// return fmt.Errorf("user %s not found in shadow file: %w", username, err)
1373+
// Do not log command output or sensitive file contents
1374+
log.Errorf("User %s not found in shadow file", username)
1375+
return fmt.Errorf("user %s not found in shadow file", username)
13671376
}
1368-
log.Debugf("User in shadow: %s", strings.TrimSpace(output))
1377+
// log.Debugf("User in shadow: %s", strings.TrimSpace(output))
1378+
// User was found in shadow; avoid logging the line content to prevent leaking sensitive data
13691379

13701380
return nil
13711381
}
@@ -1508,8 +1518,10 @@ func setUserPassword(installRoot string, user config.UserConfig) error {
15081518
// Password is already hashed, use usermod to set it directly
15091519
usermodCmd := fmt.Sprintf("usermod -p '%s' %s", user.Password, user.Name)
15101520
if _, err := shell.ExecCmd(usermodCmd, true, installRoot, nil); err != nil {
1511-
log.Errorf("Failed to set hashed password for user %s: %v", user.Name, err)
1512-
return fmt.Errorf("failed to set hashed password for user %s: %w", user.Name, err)
1521+
// log.Errorf("Failed to set hashed password for user %s: %v", user.Name, err)
1522+
// return fmt.Errorf("failed to set hashed password for user %s: %w", user.Name, err)
1523+
log.Errorf("Failed to set hashed password for user %s", user.Name)
1524+
return fmt.Errorf("failed to set hashed password for user %s", user.Name)
15131525
}
15141526
} else {
15151527
// Password is plaintext, need to hash it first
@@ -1520,17 +1532,21 @@ func setUserPassword(installRoot string, user config.UserConfig) error {
15201532

15211533
usermodCmd := fmt.Sprintf("usermod -p '%s' %s", hashedPassword, user.Name)
15221534
if _, err := shell.ExecCmd(usermodCmd, true, installRoot, nil); err != nil {
1523-
log.Errorf("Failed to set hashed password for user %s: %v", user.Name, err)
1524-
return fmt.Errorf("failed to set hashed password for user %s: %w", user.Name, err)
1535+
// log.Errorf("Failed to set hashed password for user %s: %v", user.Name, err)
1536+
// return fmt.Errorf("failed to set hashed password for user %s: %w", user.Name, err)
1537+
log.Errorf("Failed to set password for user %s", user.Name)
1538+
return fmt.Errorf("failed to set password for user %s", user.Name)
15251539
}
15261540
}
15271541
} else {
15281542
// No hash algorithm specified, use interactive passwd command (legacy behavior)
15291543
passwdInput := fmt.Sprintf("%s\n%s\n", user.Password, user.Password)
15301544
passwdCmd := fmt.Sprintf("passwd %s", user.Name)
15311545
if _, err := shell.ExecCmdWithInput(passwdInput, passwdCmd, true, installRoot, nil); err != nil {
1532-
log.Errorf("Failed to set password for user %s: %v", user.Name, err)
1533-
return fmt.Errorf("failed to set password for user %s: %w", user.Name, err)
1546+
// log.Errorf("Failed to set password for user %s: %v", user.Name, err)
1547+
// return fmt.Errorf("failed to set password for user %s: %w", user.Name, err)
1548+
log.Errorf("Failed to set password for user %s", user.Name)
1549+
return fmt.Errorf("failed to set password for user %s", user.Name)
15341550
}
15351551
}
15361552

@@ -1562,7 +1578,8 @@ func hashPassword(password, hashAlgo, installRoot string) (string, error) {
15621578
log.Debugf("Hashing password with algorithm %s", hashAlgo)
15631579
output, err := shell.ExecCmd(cmd, true, installRoot, nil)
15641580
if err != nil {
1565-
log.Errorf("Failed to hash password with algorithm %s: %v", hashAlgo, err)
1581+
// log.Errorf("Failed to hash password with algorithm %s: %v", hashAlgo, err)
1582+
log.Errorf("Failed to hash password with algorithm %s", hashAlgo)
15661583
return "", fmt.Errorf("failed to hash password with algorithm %s: %w", hashAlgo, err)
15671584
}
15681585

@@ -1591,7 +1608,9 @@ func configUserStartupScript(installRoot string, user config.UserConfig) error {
15911608
passwdFile := filepath.Join(installRoot, "etc", "passwd")
15921609

15931610
if err := file.ReplaceRegexInFile(findPattern, replacePattern, passwdFile); err != nil {
1594-
log.Errorf("Failed to update user %s startup command: %v", user.Name, err)
1611+
// log.Errorf("Failed to update user %s startup command: %v", user.Name, err)
1612+
// Log only high-level context to avoid leaking potentially sensitive details from the underlying error.
1613+
log.Errorf("Failed to update startup command for user %s", user.Name)
15951614
return fmt.Errorf("failed to update user %s startup command: %w", user.Name, err)
15961615
}
15971616
return nil

‎internal/image/imageos/imageos_test.go‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1737,8 +1737,8 @@ func TestInstallImagePkgs(t *testing.T) {
17371737
pkgType string
17381738
expected string
17391739
}{
1740-
{"RPM packages", "rpm", "rpm"},
1741-
{"DEB packages", "deb", "repo config file"},
1740+
{"RPM packages", "rpm", "RPM database"},
1741+
{"DEB packages", "deb", "local repo"},
17421742
}
17431743

17441744
for _, tt := range tests {
@@ -1758,6 +1758,11 @@ func TestInstallImagePkgs(t *testing.T) {
17581758
if err := os.MkdirAll(configDir, 0755); err != nil {
17591759
t.Fatalf("Failed to create config directory: %v", err)
17601760
}
1761+
// Create the local.list file that the method expects
1762+
localListPath := filepath.Join(configDir, "local.list")
1763+
if err := os.WriteFile(localListPath, []byte("deb file:///repo bookworm main"), 0644); err != nil {
1764+
t.Fatalf("Failed to create local.list file: %v", err)
1765+
}
17611766
defer os.RemoveAll("/tmp/config")
17621767
}
17631768

‎internal/utils/shell/shell.go‎

Lines changed: 20 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -456,7 +456,9 @@ func GetFullCmdStr(cmdStr string, sudo bool, chrootPath string, envVal []string)
456456

457457
fullCmdStr = "sudo " + envValStr + "chroot " + chrootPath + " " + fullPathCmdStr
458458
chrootDir := filepath.Base(chrootPath)
459-
log.Debugf("Chroot " + chrootDir + " Exec: [" + fullPathCmdStr + "]")
459+
// log.Debugf("Chroot " + chrootDir + " Exec: [" + fullPathCmdStr + "]")
460+
// Avoid logging full command string to prevent leaking sensitive data.
461+
log.Debugf("Chroot %s Exec: [command executed]", chrootDir)
460462

461463
} else {
462464
if sudo {
@@ -467,10 +469,14 @@ func GetFullCmdStr(cmdStr string, sudo bool, chrootPath string, envVal []string)
467469
}
468470

469471
fullCmdStr = "sudo " + envValStr + fullPathCmdStr
470-
log.Debugf("Exec: [sudo " + fullPathCmdStr + "]")
472+
// log.Debugf("Exec: [sudo " + fullPathCmdStr + "]")
473+
// Avoid logging full command string to prevent leaking sensitive data.
474+
log.Debugf("Exec with sudo: [command executed]")
471475
} else {
472476
fullCmdStr = fullPathCmdStr
473-
log.Debugf("Exec: [" + fullPathCmdStr + "]")
477+
// log.Debugf("Exec: [" + fullPathCmdStr + "]")
478+
// Avoid logging full command string to prevent leaking sensitive data.
479+
log.Debugf("Exec without sudo: [command executed]")
474480
}
475481
}
476482

@@ -490,13 +496,19 @@ func (d *DefaultExecutor) ExecCmd(cmdStr string, sudo bool, chrootPath string, e
490496

491497
if err != nil {
492498
if outputStr != "" {
493-
return outputStr, fmt.Errorf("failed to exec %s: output %s, err %w", fullCmdStr, outputStr, err)
499+
// return outputStr, fmt.Errorf("failed to exec %s: output %s, err %w", fullCmdStr, outputStr, err)
500+
// Do not include the full command string in the error to avoid leaking sensitive data.
501+
return outputStr, fmt.Errorf("failed to execute command with output: %w", err)
494502
} else {
495-
return outputStr, fmt.Errorf("failed to exec %s: %w", fullCmdStr, err)
503+
// return outputStr, fmt.Errorf("failed to exec %s: %w", fullCmdStr, err)
504+
// Do not include the full command string in the error to avoid leaking sensitive data.
505+
return outputStr, fmt.Errorf("failed to execute command: %w", err)
496506
}
497507
} else {
498508
if outputStr != "" {
499-
log.Debugf(outputStr)
509+
// log.Debugf(outputStr)
510+
// Avoid logging raw command output to prevent leaking sensitive data.
511+
log.Debugf("Command executed successfully with non-empty output")
500512
}
501513
return outputStr, nil
502514
}
@@ -599,7 +611,8 @@ func (d *DefaultExecutor) ExecCmdWithInput(inputStr string, cmdStr string, sudo
599611
if outputStr != "" {
600612
log.Infof(outputStr)
601613
}
602-
return outputStr, fmt.Errorf("failed to exec %s with input %s: %w", fullCmdStr, inputStr, err)
614+
// return outputStr, fmt.Errorf("failed to exec %s with input %s: %w", fullCmdStr, inputStr, err)
615+
return outputStr, fmt.Errorf("failed to exec command: %w", err)
603616
} else {
604617
if outputStr != "" {
605618
log.Debugf(outputStr)

‎internal/utils/shell/shell_mock.go‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -31,14 +31,20 @@ func getFullCmdStr(cmdStr string, sudo bool, chrootPath string, envVal []string)
3131
if chrootPath != HostPath {
3232
fullCmdStr = "sudo " + envValStr + "chroot " + chrootPath + " " + cmdStr
3333
chrootDir := filepath.Base(chrootPath)
34-
log.Debugf("MockExecutor: Chroot " + chrootDir + " Exec: [" + cmdStr + "]")
34+
// log.Debugf("MockExecutor: Chroot " + chrootDir + " Exec: [" + cmdStr + "]")
35+
// Avoid logging full command string to prevent leaking sensitive data such as passwords.
36+
log.Debugf("MockExecutor: Chroot %s Exec", chrootDir)
3537
} else {
3638
if sudo {
3739
fullCmdStr = "sudo " + envValStr + cmdStr
38-
log.Debugf("MockExecutor: Host Exec with sudo: [" + cmdStr + "]")
40+
// log.Debugf("MockExecutor: Host Exec with sudo: [" + cmdStr + "]")
41+
// Avoid logging full command string to prevent leaking sensitive data such as passwords.
42+
log.Debugf("MockExecutor: Host Exec with sudo")
3943
} else {
4044
fullCmdStr = envValStr + cmdStr
41-
log.Debugf("MockExecutor: Host Exec: [" + cmdStr + "]")
45+
// log.Debugf("MockExecutor: Host Exec: [" + cmdStr + "]")
46+
// Avoid logging full command string to prevent leaking sensitive data such as passwords.
47+
log.Debugf("MockExecutor: Host Exec without sudo")
4248
}
4349
}
4450
return fullCmdStr, nil

0 commit comments

Comments
 (0)