address comment, add path check.

sonic-net · Feb 27, 2025 · b1fe364 · b1fe364
1 parent adafd2a
commit b1fe364
Show file tree

Hide file tree

Showing 2 changed files with 63 additions and 8 deletions.
diff --git a/gnmi_server/gnoi_system.go b/gnmi_server/gnoi_system.go
@@ -4,6 +4,7 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"path/filepath"
 	"strings"
 	"time"
 
@@ -298,6 +299,27 @@ func (srv *Server) Traceroute(req *syspb.TracerouteRequest, stream syspb.System_
 	return status.Errorf(codes.Unimplemented, "Method system.Traceroute is unimplemented.")
 }
 
+func isInsideFirmwareDir(path string) (bool, error) {
+	// Define the allowed base directory
+	firmwareDir := "/lib/firmware"
+
+	// Get the absolute path of the input
+	absPath, err := filepath.Abs(path)
+	if err != nil {
+		return false, err
+	}
+
+	// Resolve any symlinks in the path
+	resolvedPath, err := filepath.EvalSymlinks(absPath)
+	if err != nil {
+		// If the path doesn't exist, it might be a new file, so just use absPath
+		resolvedPath = absPath
+	}
+
+	// Check if the resolved path is within /lib/firmware/
+	return filepath.HasPrefix(resolvedPath, firmwareDir), nil
+}
+
 func (srv *Server) SetPackage(rs syspb.System_SetPackageServer) error {
 	ctx := rs.Context()
 
@@ -343,6 +365,12 @@ func (srv *Server) SetPackage(rs syspb.System_SetPackageServer) error {
 	// Log the package filename and version
 	log.V(1).Infof("Package filename: %s, version: %s", pkg.Package.Filename, pkg.Package.Version)
 
+	// Only allow putting files in /lib/firmware
+	if ok, err := isInsideFirmwareDir(pkg.Package.Filename); !ok || err != nil {
+		log.Errorf("Invalid filename: %s, error: %v", pkg.Package.Filename, err)
+		return status.Errorf(codes.InvalidArgument, "invalid filename: %s, error: %v", pkg.Package.Filename, err)
+	}
+
 	// Download the package if RemoteDownload is provided
 	if pkg.Package.RemoteDownload != nil {
 		// Validate RemoteDownload

diff --git a/gnmi_server/gnoi_system_test.go b/gnmi_server/gnoi_system_test.go
@@ -48,7 +48,7 @@ func TestSetPackage(t *testing.T) {
 					RemoteDownload: &commonpb.RemoteDownload{
 						Path: "http://example.com/package",
 					},
-					Filename: "package.bin",
+					Filename: "/lib/firmware/package.bin",
 					Version:  "1.0",
 					Activate: false,
 				},
@@ -88,7 +88,7 @@ func TestSetPackage(t *testing.T) {
 		mockServer.EXPECT().Recv().Return(&syspb.SetPackageRequest{
 			Request: &syspb.SetPackageRequest_Package{
 				Package: &syspb.Package{
-					Filename: "package.bin",
+					Filename: "/lib/firmware/package.bin",
 					Version:  "1.0",
 					Activate: true,
 				},
@@ -131,7 +131,7 @@ func TestSetPackage(t *testing.T) {
 					RemoteDownload: &commonpb.RemoteDownload{
 						Path: "http://example.com/package",
 					},
-					Filename: "package.bin",
+					Filename: "/lib/firmware/package.bin",
 					Version:  "1.0",
 					Activate: true,
 				},
@@ -176,7 +176,7 @@ func TestSetPackage(t *testing.T) {
 					RemoteDownload: &commonpb.RemoteDownload{
 						Path: "http://example.com/package",
 					},
-					Filename: "package.bin",
+					Filename: "/lib/firmware/package.bin",
 					Version:  "1.0",
 					Activate: false,
 				},
@@ -209,7 +209,7 @@ func TestSetPackage(t *testing.T) {
 		mockServer.EXPECT().Recv().Return(&syspb.SetPackageRequest{
 			Request: &syspb.SetPackageRequest_Package{
 				Package: &syspb.Package{
-					Filename: "package.bin",
+					Filename: "/lib/firmware/package.bin",
 					Version:  "1.0",
 					Activate: true,
 				},
@@ -265,14 +265,41 @@ func TestSetPackage(t *testing.T) {
 		}
 	})
 
+	t.Run("SetPackageInvalidPath", func(t *testing.T) {
+		mockServer.EXPECT().Recv().Return(&syspb.SetPackageRequest{
+			Request: &syspb.SetPackageRequest_Package{
+				Package: &syspb.Package{
+					RemoteDownload: &commonpb.RemoteDownload{
+						Path: "http://malicious.com/package",
+					},
+					Filename: "/etc/passwd",
+					Version:  "1.0",
+					Activate: true,
+				},
+			},
+		}, nil).Times(1)
+		err := srv.SetPackage(mockServer)
+		if err == nil {
+			t.Fatalf("Expected error but got none")
+		}
+		expectedErrorCode := codes.InvalidArgument
+		st, ok := status.FromError(err)
+		if !ok {
+			t.Fatalf("Expected gRPC status error, but got %v", err)
+		}
+		if st.Code() != expectedErrorCode {
+			t.Errorf("Expected error code '%v', but got '%v'", expectedErrorCode, st.Code())
+		}
+	})
+
 	t.Run("SetPackageMissingVersion", func(t *testing.T) {
 		mockServer.EXPECT().Recv().Return(&syspb.SetPackageRequest{
 			Request: &syspb.SetPackageRequest_Package{
 				Package: &syspb.Package{
 					RemoteDownload: &commonpb.RemoteDownload{
 						Path: "http://example.com/package",
 					},
-					Filename: "package.bin",
+					Filename: "/lib/firmware/package.bin",
 					Activate: true,
 				},
 			},
@@ -296,7 +323,7 @@ func TestSetPackage(t *testing.T) {
 			Request: &syspb.SetPackageRequest_Package{
 				Package: &syspb.Package{
 					RemoteDownload: &commonpb.RemoteDownload{},
-					Filename:       "package.bin",
+					Filename:       "/lib/firmware/package.bin",
 					Version:        "1.0",
 					Activate:       false,
 				},
@@ -344,7 +371,7 @@ func TestSetPackage(t *testing.T) {
 					RemoteDownload: &commonpb.RemoteDownload{
 						Path: "http://example.com/package",
 					},
-					Filename: "package.bin",
+					Filename: "/lib/firmware/package.bin",
 					Version:  "1.0",
 					Activate: false,
 				},