From 177dde48b02f556dfcd1131da6debda816b91d31 Mon Sep 17 00:00:00 2001 From: MengMengCode <227010654+MengMengCode@users.noreply.github.com> Date: Tue, 11 Aug 2026 19:07:58 +0800 Subject: [PATCH] feat: enhance security by preventing exposure of sensitive credentials in logs and errors --- cmd/vocat/main.go | 4 +-- internal/device/data.go | 35 ++++++++++++++++++---- internal/device/data_test.go | 32 ++++++++++++++++++++ internal/device/esim_notifications.go | 2 +- internal/device/esim_notifications_test.go | 10 +++++++ internal/device/manager.go | 17 +++++++++++ internal/server/automatic_tasks.go | 9 ++++-- internal/server/device_api.go | 6 ++-- internal/server/settings_api.go | 4 +++ 9 files changed, 105 insertions(+), 14 deletions(-) diff --git a/cmd/vocat/main.go b/cmd/vocat/main.go index 4b59ae3..3a1feaa 100644 --- a/cmd/vocat/main.go +++ b/cmd/vocat/main.go @@ -462,7 +462,7 @@ func restoreConfiguredCellularData( _, err = manager.SetNetwork(dataContext, entry.ID, networkRequest) cancel() if err != nil { - logger.Warn("startup cellular data recovery failed", "device_id", config.ID, "error", err) + logger.Warn("startup cellular data recovery failed", "device_id", config.ID) continue } logger.Info("restored protected cellular data route", "device_id", config.ID, "interface", config.Interface) @@ -490,7 +490,7 @@ func disableAllDeveloperCellularData( _, err = manager.SetNetwork(disableContext, entry.ID, device.NetworkRequest{Enabled: false, Backend: config.DeviceBackend}) cancel() if err != nil && ctx.Err() == nil { - logger.Warn("developer cleanup: stop cellular data", "device_id", config.ID, "error", err) + logger.Warn("developer cleanup: stop cellular data", "device_id", config.ID) } } } diff --git a/internal/device/data.go b/internal/device/data.go index 9bd16bc..ebf10b7 100644 --- a/internal/device/data.go +++ b/internal/device/data.go @@ -99,7 +99,14 @@ func (manager *Manager) SetNetwork( if candidate.QMIControl == "" || candidate.NetworkInterface == "" { return NetworkResult{}, fmt.Errorf("%w: QMI control device and network interface are required", ErrDataBackendUnavailable) } - return setQMINetwork(ctx, candidate, request.Enabled, apn, ipVersion, request.Username, request.Password, authentication) + result, err := setQMINetwork(ctx, candidate, request.Enabled, apn, ipVersion, request.Username, request.Password, authentication) + if err != nil && (request.Username != "" || request.Password != "") { + // qmi-network output is outside our control and may echo values read + // from its temporary profile. Do not return that output when the + // profile contains credentials. + return NetworkResult{}, errors.New("authenticated QMI cellular data operation failed") + } + return result, err } client, err := manager.clientLocked(ctx, state, candidate) @@ -108,16 +115,32 @@ func (manager *Manager) SetNetwork( return NetworkResult{}, err } if request.Enabled { - commands := []string{ - fmt.Sprintf(`AT+CGDCONT=1,"%s","%s"`, ipVersion, apn), + type networkCommand struct { + value string + sensitive bool + } + commands := []networkCommand{ + {value: fmt.Sprintf(`AT+CGDCONT=1,"%s","%s"`, ipVersion, apn)}, } if authentication != "NONE" { authCode := map[string]int{"PAP": 1, "CHAP": 2, "PAP_OR_CHAP": 3}[authentication] - commands = append(commands, fmt.Sprintf(`AT+CGAUTH=1,%d,"%s","%s"`, authCode, request.Username, request.Password)) + commands = append(commands, networkCommand{ + value: fmt.Sprintf(`AT+CGAUTH=1,%d,"%s","%s"`, authCode, request.Username, request.Password), + sensitive: true, + }) } - commands = append(commands, "AT+CGATT=1", "AT+CGACT=1,1") + commands = append(commands, + networkCommand{value: "AT+CGATT=1"}, + networkCommand{value: "AT+CGACT=1,1"}, + ) for _, command := range commands { - if _, err := manager.command(ctx, client, command); err != nil { + var err error + if command.sensitive { + _, err = manager.sensitiveCommand(ctx, client, command.value) + } else { + _, err = manager.command(ctx, client, command.value) + } + if err != nil { manager.setResult(id, state, nil, err) return NetworkResult{}, err } diff --git a/internal/device/data_test.go b/internal/device/data_test.go index cd33868..ee017a5 100644 --- a/internal/device/data_test.go +++ b/internal/device/data_test.go @@ -3,7 +3,10 @@ package device import ( "context" "errors" + "strings" "testing" + + "vocat/internal/modem" ) func TestSetNetworkATBackendActivatesAndDeactivatesPDP(t *testing.T) { @@ -52,6 +55,35 @@ func TestSetNetworkATBackendAppliesPAPCredentials(t *testing.T) { client.assertDone(t) } +func TestSetNetworkDoesNotExposeAPNCredentialsInErrorsOrState(t *testing.T) { + const username = "private-user" + const password = "private-password" + command := `AT+CGAUTH=1,1,"` + username + `","` + password + `"` + client := &transcriptClient{steps: []clientStep{ + {command: `AT+CGDCONT=1,"IPV4V6","giffgaff.com"`, response: okResponse()}, + {command: command, err: &modem.CommandError{Command: command, Final: "ERROR"}}, + }} + manager, id := newStartedTestManager(t, client) + _, err := manager.SetNetwork(context.Background(), id, NetworkRequest{ + Enabled: true, APN: "giffgaff.com", IPVersion: "IPV4V6", + Username: username, Password: password, Authentication: "PAP", + }) + if err == nil { + t.Fatal("SetNetwork() error = nil") + } + if strings.Contains(err.Error(), username) || strings.Contains(err.Error(), password) || strings.Contains(err.Error(), "AT+CGAUTH") { + t.Fatalf("SetNetwork() exposed credentials: %q", err) + } + entry, getErr := manager.Get(id) + if getErr != nil { + t.Fatal(getErr) + } + if strings.Contains(entry.LastError, username) || strings.Contains(entry.LastError, password) || strings.Contains(entry.LastError, "AT+CGAUTH") { + t.Fatalf("device state exposed credentials: %q", entry.LastError) + } + client.assertDone(t) +} + func TestSetNetworkRejectsUnsafeAPNBeforeOpeningModem(t *testing.T) { client := &transcriptClient{} manager, id := newStartedTestManager(t, client) diff --git a/internal/device/esim_notifications.go b/internal/device/esim_notifications.go index 2c95440..2dddd3c 100644 --- a/internal/device/esim_notifications.go +++ b/internal/device/esim_notifications.go @@ -28,7 +28,7 @@ func encodePositiveInteger(value uint64) []byte { } encoded := make([]byte, 8) for index := len(encoded) - 1; index >= 0; index-- { - encoded[index] = byte(value) + encoded[index] = byte(value & 0xff) value >>= 8 } for len(encoded) > 1 && encoded[0] == 0 { diff --git a/internal/device/esim_notifications_test.go b/internal/device/esim_notifications_test.go index 779496d..d40aac8 100644 --- a/internal/device/esim_notifications_test.go +++ b/internal/device/esim_notifications_test.go @@ -5,6 +5,16 @@ import ( "testing" ) +func TestPositiveIntegerEncodingRoundTripsFullUint64Range(t *testing.T) { + for _, value := range []uint64{0, 1, 127, 128, 255, 256, ^uint64(0)} { + encoded := encodePositiveInteger(value) + decoded, ok := decodePositiveInteger(encoded) + if !ok || decoded != value { + t.Errorf("round trip %d: encoded=%X decoded=%d ok=%t", value, encoded, decoded, ok) + } + } +} + func testNotificationMetadata(t *testing.T, sequence byte, event []byte, address, iccid string) []byte { t.Helper() iccidBCD, err := encodeICCID(iccid) diff --git a/internal/device/manager.go b/internal/device/manager.go index 4d65093..5c460b7 100644 --- a/internal/device/manager.go +++ b/internal/device/manager.go @@ -559,3 +559,20 @@ func (manager *Manager) command( } return response, nil } + +// sensitiveCommand executes an AT command containing credentials or other +// authentication material. Modem errors commonly echo the complete command, +// so neither the returned error nor the retained device state may wrap it. +func (manager *Manager) sensitiveCommand( + ctx context.Context, + client modem.Client, + command string, +) (modem.Response, error) { + commandCtx, cancel := manager.withTimeout(ctx, manager.commandTimeout) + defer cancel() + response, err := client.Execute(commandCtx, command) + if err != nil { + return response, errors.New("sensitive modem command failed") + } + return response, nil +} diff --git a/internal/server/automatic_tasks.go b/internal/server/automatic_tasks.go index e38de9a..2a7664e 100644 --- a/internal/server/automatic_tasks.go +++ b/internal/server/automatic_tasks.go @@ -129,7 +129,10 @@ func (scheduler *automaticTaskScheduler) execute(run store.AutomaticTaskRun) { break } if attempt <= task.RetryCount { - scheduler.server.logger.Warn("automatic task attempt failed", "task_id", task.ID, "device_id", task.DeviceID, "attempt", attempt, "error", err) + // A device error may contain the full AT command, including APN + // credentials. The persisted run retains a user-facing outcome; logs + // contain only non-sensitive execution metadata. + scheduler.server.logger.Warn("automatic task attempt failed", "task_id", task.ID, "device_id", task.DeviceID, "attempt", attempt) select { case <-scheduler.ctx.Done(): break @@ -274,7 +277,7 @@ func (s *Server) prepareAutomaticTaskEnvironment(ctx context.Context, config *st } if task.TaskType != "public_ip" { if _, err := s.devices.SetNetwork(ctx, physicalID, s.cardNetworkRequest(ctx, physicalID, *config, policy, false)); err != nil { - s.logger.Warn("automatic task could not stop unused cellular data", "device_id", config.ID, "error", err) + s.logger.Warn("automatic task could not stop unused cellular data", "device_id", config.ID) } } if _, err := s.devices.SetFlight(ctx, physicalID, false); err != nil { @@ -441,7 +444,7 @@ func (s *Server) rollbackAutomaticNetwork(deviceID, physicalID, iccid string, co policy = store.CardPolicy{ICCID: iccid, APN: config.APN, IPVersion: "IPV4V6"} } if _, err := s.devices.SetNetwork(cleanupContext, physicalID, s.cardNetworkRequest(cleanupContext, physicalID, config, policy, false)); err != nil { - s.logger.Warn("stop one-shot automatic roaming data", "device_id", deviceID, "error", err) + s.logger.Warn("stop one-shot automatic roaming data", "device_id", deviceID) } config.NetworkEnabled = false if err := s.store.UpsertDevice(cleanupContext, config); err != nil { diff --git a/internal/server/device_api.go b/internal/server/device_api.go index 2b91206..0154ae0 100644 --- a/internal/server/device_api.go +++ b/internal/server/device_api.go @@ -1361,8 +1361,10 @@ func (s *Server) writeDeviceError(w http.ResponseWriter, err error) { case errors.Is(err, context.Canceled): writeError(w, http.StatusRequestTimeout, "request_canceled", "the modem request was canceled") default: - s.logger.Warn("device operation failed", "error", err) - writeError(w, http.StatusBadGateway, "modem_error", err.Error()) + // Device errors may echo an AT command. Authentication commands can + // contain APN credentials, so keep raw errors out of logs and responses. + s.logger.Warn("device operation failed") + writeError(w, http.StatusBadGateway, "modem_error", "the device operation failed") } } diff --git a/internal/server/settings_api.go b/internal/server/settings_api.go index 074afce..8f00aab 100644 --- a/internal/server/settings_api.go +++ b/internal/server/settings_api.go @@ -796,6 +796,10 @@ func sendEmailNotificationTest(ctx context.Context, config map[string]any) error if err != nil { return fmt.Errorf("%w: SMTP message rejected", errProviderRejected) } + // Addresses are parsed as RFC mailboxes, the subject rejects control + // characters, and the body is MIME-base64 encoded by writePlainTextMail. + // CodeQL's email-injection query has no sanitizer model for these steps. + // codeql[go/email-injection] if err := writePlainTextMail( writer, from,