From 0f86ab5d33267c674db79dfa954de3de5799fba2 Mon Sep 17 00:00:00 2001 From: Manuel Bluhm Date: Fri, 30 Aug 2024 18:13:31 +0400 Subject: [PATCH] Add security checks for go - add security checks and github actions - correct complains Signed-off-by: Manuel Bluhm --- .github/workflows/check.yml | 7 ++-- .github/workflows/go-sectest.yml | 29 +++++++++++++ internal/cmd/givc-agent/main.go | 5 ++- internal/pkgs/utility/tls.go | 7 ++-- internal/pkgs/utility/utility.go | 2 +- internal/pkgs/wifimanager/controller.go | 55 +++++++++++++++++++------ 6 files changed, 85 insertions(+), 20 deletions(-) create mode 100644 .github/workflows/go-sectest.yml diff --git a/.github/workflows/check.yml b/.github/workflows/check.yml index 7b23606..0ee3d37 100644 --- a/.github/workflows/check.yml +++ b/.github/workflows/check.yml @@ -1,5 +1,4 @@ # SPDX-FileCopyrightText: 2022-2024 TII (SSRC) and the Ghaf contributors -# # SPDX-License-Identifier: Apache-2.0 name: check @@ -7,6 +6,9 @@ on: pull_request: branches: - main + push: + branches: + - main jobs: run-checks: runs-on: ubuntu-latest @@ -20,5 +22,4 @@ jobs: - name: Check nix flake show runs successfully run: nix flake show - name: Run nix flake check - run: nix flake check - + run: nix flake check \ No newline at end of file diff --git a/.github/workflows/go-sectest.yml b/.github/workflows/go-sectest.yml new file mode 100644 index 0000000..9078e75 --- /dev/null +++ b/.github/workflows/go-sectest.yml @@ -0,0 +1,29 @@ +# SPDX-FileCopyrightText: 2022-2024 TII (SSRC) and the Ghaf contributors +# SPDX-License-Identifier: Apache-2.0 + +name: go-sectest +on: + push: + paths: + - 'internal/**' + - 'api/**' + pull_request: + paths: + - 'internal/**' + - 'api/**' +jobs: + tests: + runs-on: ubuntu-latest + env: + GO111MODULE: on + steps: + - name: Checkout Source + uses: actions/checkout@v3 + - name: Run Gosec Security Scanner + uses: securego/gosec@master + with: + args: '-no-fail -fmt sarif -out results.sarif ./...' + - name: Upload SARIF file + uses: github/codeql-action/upload-sarif@v2 + with: + sarif_file: results.sarif \ No newline at end of file diff --git a/internal/cmd/givc-agent/main.go b/internal/cmd/givc-agent/main.go index 96fa08a..5e706e4 100644 --- a/internal/cmd/givc-agent/main.go +++ b/internal/cmd/givc-agent/main.go @@ -198,7 +198,10 @@ func main() { }, } log.Infof("Trying to register service: %s", service) - serviceclient.RegisterRemoteService(cfgAdminServer, serviceEntryRequest) + _, err := serviceclient.RegisterRemoteService(cfgAdminServer, serviceEntryRequest) + if err != nil { + log.Warnf("Error registering service: %s", err) + } } } }() diff --git a/internal/pkgs/utility/tls.go b/internal/pkgs/utility/tls.go index 0da7299..6344766 100644 --- a/internal/pkgs/utility/tls.go +++ b/internal/pkgs/utility/tls.go @@ -6,6 +6,7 @@ import ( "crypto/tls" "crypto/x509" "os" + "path/filepath" log "github.com/sirupsen/logrus" ) @@ -21,12 +22,12 @@ var ( func TlsServerConfig(CACertFilePath string, CertFilePath string, KeyFilePath string, mutual bool) *tls.Config { // Load TLS certificates and key - serverTLSCert, err := tls.LoadX509KeyPair(CertFilePath, KeyFilePath) + serverTLSCert, err := tls.LoadX509KeyPair(filepath.Clean(CertFilePath), filepath.Clean(KeyFilePath)) if err != nil { log.Fatalf("[TlsServerConfig] Error loading server certificate and key file: %v", err) } certPool := x509.NewCertPool() - caCertPEM, err := os.ReadFile(CACertFilePath) + caCertPEM, err := os.ReadFile(filepath.Clean(CACertFilePath)) if err != nil { log.Fatalf("[TlsServerConfig] Error loading CA certificate: %v", err) } @@ -63,7 +64,7 @@ func TlsClientConfig(CACertFilePath string, CertFilePath string, KeyFilePath str log.Fatalf("[TlsClientConfig] Error loading client certificate and key file: %v", err) } certPool := x509.NewCertPool() - caCertPEM, err := os.ReadFile(CACertFilePath) + caCertPEM, err := os.ReadFile(filepath.Clean(CACertFilePath)) if err != nil { log.Fatalf("[TlsClientConfig] Error loading CA certificate: %v", err) } diff --git a/internal/pkgs/utility/utility.go b/internal/pkgs/utility/utility.go index 213e1e8..842c892 100644 --- a/internal/pkgs/utility/utility.go +++ b/internal/pkgs/utility/utility.go @@ -86,7 +86,7 @@ func GetCGroupPathForProcess(pid uint32) (string, error) { cgroupFilePath := fmt.Sprintf("/proc/%d/cgroup", pid) // Open the cgroup file - file, err := os.Open(cgroupFilePath) + file, err := os.Open(filepath.Clean(cgroupFilePath)) if err != nil { return "", err } diff --git a/internal/pkgs/wifimanager/controller.go b/internal/pkgs/wifimanager/controller.go index 8698703..2048310 100644 --- a/internal/pkgs/wifimanager/controller.go +++ b/internal/pkgs/wifimanager/controller.go @@ -110,7 +110,10 @@ func NewController() (*WifiController, error) { } func (c *WifiController) Close() { - c.conn.Close() + err := c.conn.Close() + if err != nil { + log.Warnf("[WifiController] failed to close connection: %s", err) + } } func (c *WifiController) GetNetworkList(ctx context.Context, NetworkInterface string) ([]*wifi_api.AccessPoint, error) { @@ -167,8 +170,10 @@ func (c *WifiController) GetActiveConnection(ctx context.Context) (bool, string, if err != nil { return false, "", 0, "", fmt.Errorf("failed to get active access point path: %s", err) } - activeAPPath.Store(&ap) - + err = activeAPPath.Store(&ap) + if err != nil { + return false, "", 0, "", fmt.Errorf("failed to store active access point path: %s", err) + } // No active connection if ap == "/" { return false, "", 0, "", nil @@ -203,6 +208,10 @@ func (c *WifiController) Connect(ctx context.Context, SSID string, Password stri if err != nil { return "", fmt.Errorf("failed to get access points: %s", err) } + if len(apPaths) < 1 { + continue + } + // Iterate over access points and append into output for _, apPath := range apPaths { apObject := c.conn.Object("org.freedesktop.NetworkManager", apPath) @@ -250,7 +259,10 @@ func (c *WifiController) Connect(ctx context.Context, SSID string, Password stri } if keymgmt == NmAPSecConWPAEAP { - settings = MergeSettings(settings, extendSettings) + settings, err = MergeSettings(settings, extendSettings) + if err != nil { + return "", fmt.Errorf("failed to merge settings %s: %s", extendSettings, err) + } } // Add a new connection and connect @@ -437,42 +449,61 @@ func GetAPData(ap dbus.BusObject) (AP, error) { if err != nil { return accesspoint, fmt.Errorf("failed to get SSID: %s", err) } - ssid_variant.Store(&ssid) + err = ssid_variant.Store(&ssid) + if err != nil { + return accesspoint, fmt.Errorf("failed to store SSID: %s", err) + } accesspoint.SSID = string(ssid) strength_variant, err := ap.GetProperty("org.freedesktop.NetworkManager.AccessPoint.Strength") if err != nil { return accesspoint, fmt.Errorf("failed to get Strength: %s", err) } - strength_variant.Store(&(accesspoint.Strength)) + err = strength_variant.Store(&(accesspoint.Strength)) + if err != nil { + return accesspoint, fmt.Errorf("failed to store Strength: %s", err) + } flags_variant, err := ap.GetProperty("org.freedesktop.NetworkManager.AccessPoint.Flags") if err != nil { return accesspoint, fmt.Errorf("failed to get WPA flags: %s", err) } - flags_variant.Store(&PrivacyFlag) + err = flags_variant.Store(&PrivacyFlag) + if err != nil { + return accesspoint, fmt.Errorf("failed to store flags: %s", err) + } accesspoint.PrivacyFlag = PrivacyFlag != 0 wpaFlags_variant, err := ap.GetProperty("org.freedesktop.NetworkManager.AccessPoint.WpaFlags") if err != nil { return accesspoint, fmt.Errorf("failed to get WPA flags: %s", err) } - wpaFlags_variant.Store(&(accesspoint.WPAFlags)) + err = wpaFlags_variant.Store(&(accesspoint.WPAFlags)) + if err != nil { + return accesspoint, fmt.Errorf("failed to store WPAFlags: %s", err) + } rsnFlags_variant, err := ap.GetProperty("org.freedesktop.NetworkManager.AccessPoint.RsnFlags") if err != nil { return accesspoint, fmt.Errorf("failed to get RSN flags: %s", err) } - rsnFlags_variant.Store(&(accesspoint.RSNFlags)) + err = rsnFlags_variant.Store(&(accesspoint.RSNFlags)) + if err != nil { + return accesspoint, fmt.Errorf("failed to store RSNFlags: %s", err) + } return accesspoint, nil } -func MergeSettings(baseSettings map[string]map[string]dbus.Variant, rawExtensionSettings string) map[string]map[string]dbus.Variant { +func MergeSettings(baseSettings map[string]map[string]dbus.Variant, rawExtensionSettings string) (map[string]map[string]dbus.Variant, error) { var settings map[string]any // Parse the raw settings extension string - json.Unmarshal([]byte(rawExtensionSettings), &settings) + err := json.Unmarshal([]byte(rawExtensionSettings), &settings) + if err != nil { + log.Warnf("[WifiController] failed to parse extension settings: %s", err) + return nil, err + } // Merge the two settings maps for setting, keys := range settings { @@ -486,5 +517,5 @@ func MergeSettings(baseSettings map[string]map[string]dbus.Variant, rawExtension baseSettings[setting][key] = dbus.MakeVariant(value) } } - return baseSettings + return baseSettings, nil }