Skip to content

Commit 6154210

Browse files
dhananjay-ngYashwantGohokar
dhananjay-ng
authored and
YashwantGohokar
committed
Added input validation on lnet label to fixing fortify command execution vuln.
1 parent 4697c0b commit 6154210

File tree

3 files changed

+65
-44
lines changed

3 files changed

+65
-44
lines changed

‎pkg/csi-util/lustre_lnet_helper.go‎

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@ import (
1212
)
1313

1414
const (
15-
CHROOT_BASH_COMMAND = "chroot-bash"
1615
LOAD_LNET_KERNEL_MODULE_COMMAND = "modprobe lnet"
1716
CONFIGURE_LNET_KERNEL_SERVICE_COMMAND = "lnetctl lnet configure"
1817
SHOW_CONFIGURED_LNET = "lnetctl net show --net %s"
@@ -66,9 +65,12 @@ func ValidateLustreVolumeId(lusterVolumeId string) (bool, string) {
6665
return false, lnetLabel
6766
}
6867
lnetLabel = parts[1]
68+
if !isValidShellInput(lnetLabel) {
69+
return false, ""
70+
}
6971
}
7072
//last part in volume handle which is fsname should start with "/"
71-
if !strings.HasPrefix(splits[len(splits)-1], "/") {
73+
if !strings.HasPrefix(splits[len(splits)-1], "/") || !isValidShellInput(splits[len(splits)-1]) {
7274
return false, lnetLabel
7375
}
7476
return true, lnetLabel
@@ -329,7 +331,8 @@ func (ls *LnetService) IsLnetActive(logger *zap.SugaredLogger, lnetLabel string)
329331
}
330332

331333
func (olc *OCILnetConfigurator) ExecuteCommandOnWorkerNode(args ...string) (string, error) {
332-
command := exec.Command(CHROOT_BASH_COMMAND, args...)
334+
335+
command := exec.Command("chroot-bash", args...)
333336

334337
output, err := command.CombinedOutput()
335338

@@ -365,15 +368,15 @@ func (ls *LnetService) ApplyLustreParameters(logger *zap.SugaredLogger, lustrePa
365368
return nil
366369
}
367370

368-
func isValidLustreParam(param string) bool {
371+
func isValidShellInput(input string) bool {
369372
// Check for no spaces
370-
if strings.Contains(param, " ") {
373+
if strings.Contains(input, " ") {
371374
return false
372375
}
373376
// List of forbidden characters
374-
forbiddenChars := []string{";", "&", "|", "<", ">", "(", ")", "`", "'", "\""}
377+
forbiddenChars := []string{";", "&", "|", "<", ">", "(", ")", "`", "'", "\"","$","!"}
375378
for _, char := range forbiddenChars {
376-
if strings.Contains(param, char) {
379+
if strings.Contains(input, char) {
377380
return false
378381
}
379382
}
@@ -397,7 +400,7 @@ func ValidateLustreParameters(logger *zap.SugaredLogger, lustreParamsJson strin
397400
for _, param := range lustreParams {
398401
for key, value := range param {
399402
logger.Infof("Validating lustre param %s=%s", key, fmt.Sprintf("%v", value))
400-
if !isValidLustreParam(key) || !isValidLustreParam(fmt.Sprintf("%v", value)) {
403+
if !isValidShellInput(key) || !isValidShellInput(fmt.Sprintf("%v", value)) {
401404
invalidParams = append(invalidParams, fmt.Sprintf("%v=%v",key, value))
402405
}
403406
}

‎pkg/csi-util/lustre_lnet_helper_test.go‎

Lines changed: 48 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -18,19 +18,22 @@ func TestValidateLustreVolumeId(t *testing.T) {
1818
}{
1919
// Valid cases
2020
{"192.168.227.11@tcp1:192.168.227.12@tcp1:/demo", true, "tcp1"},
21-
{"192.168.227.11@tcp1:/demo", true,"tcp1"},
21+
{"192.168.227.11@tcp1:/demo", true, "tcp1"},
22+
{"192.168.227.11@tcp1 & rm -rf /:192.168.227.12@tcp1:/demo", false, ""},
23+
{"192.168.227.11@tcp1:192.168.227.12@tcp1:/demo & rm -rf", false, "tcp1"},
24+
{"192.168.227.11@tcp1:/demo", true, "tcp1"},
2225
// Invalid cases
23-
{"192.168.227.11@tcp1:192.168.227.12@tcp1", false,"tcp1"}, // No fsname provided
24-
{"192.168.227.11@tcp1:192.168.227.12@tcp1:demo", false,"tcp1"}, // fsname not starting with "/"
25-
{"invalidip@tcp1:192.168.227.12@tcp1:/demo", false,""}, // Invalid IP address
26-
{"192.168.227.11@tcp1:invalidip@tcp1:/demo", false,"tcp1"}, // Invalid IP address
27-
{"192.168.227.11@:192.168.227.12@:tcp1/demo", false, ""}, // No Lnet label provided
28-
{"192.168.227.11@tcp1:192.168.227.12:/demo", false, "tcp1"}, // No Lnet label provided in 2nd MGS NID
26+
{"192.168.227.11@tcp1:192.168.227.12@tcp1", false, "tcp1"}, // No fsname provided
27+
{"192.168.227.11@tcp1:192.168.227.12@tcp1:demo", false, "tcp1"}, // fsname not starting with "/"
28+
{"invalidip@tcp1:192.168.227.12@tcp1:/demo", false, ""}, // Invalid IP address
29+
{"192.168.227.11@tcp1:invalidip@tcp1:/demo", false, "tcp1"}, // Invalid IP address
30+
{"192.168.227.11@:192.168.227.12@:tcp1/demo", false, ""}, // No Lnet label provided
31+
{"192.168.227.11@tcp1:192.168.227.12:/demo", false, "tcp1"}, // No Lnet label provided in 2nd MGS NID
2932
// Empty input
30-
{"", false,""},
33+
{"", false, ""},
3134

3235
// Single IP
33-
{"192.168.227.11", false,""}, // Missing ":" in the input
36+
{"192.168.227.11", false, ""}, // Missing ":" in the input
3437
}
3538

3639
for _, test := range tests {
@@ -53,7 +56,6 @@ type FakeConfigurator struct {
5356
ExecuteCommandOnWorkerNodeFunc func(args ...string) (string, error)
5457
}
5558

56-
5759
func (f *FakeConfigurator) GetNetInterfacesInSubnet(subnetCIDR string) ([]NetInterface, error) {
5860
return f.GetNetInterfacesInSubnetFunc(subnetCIDR)
5961
}
@@ -128,9 +130,13 @@ func TestLnetService_SetupLnet_TableDriven(t *testing.T) {
128130
// These functions are not used in this case.
129131
IsLustreClientPackagesInstalledFunc: func(logger *zap.SugaredLogger) bool { return true },
130132
ExecuteCommandOnWorkerNodeFunc: func(args ...string) (string, error) { return "ok", nil },
131-
GetLnetInfoByLnetLabelFunc: func(lnetLabel string) (NetInfo, error) { return NetInfo{}, nil },
132-
ConfigureLnetFunc: func(logger *zap.SugaredLogger, ifaces []NetInterface, lnetLabel string, netInfo NetInfo) error { return nil },
133-
VerifyLnetConfigurationFunc: func(logger *zap.SugaredLogger, ifaces []NetInterface, lnetLabel string, netInfo NetInfo, err error) error { return nil },
133+
GetLnetInfoByLnetLabelFunc: func(lnetLabel string) (NetInfo, error) { return NetInfo{}, nil },
134+
ConfigureLnetFunc: func(logger *zap.SugaredLogger, ifaces []NetInterface, lnetLabel string, netInfo NetInfo) error {
135+
return nil
136+
},
137+
VerifyLnetConfigurationFunc: func(logger *zap.SugaredLogger, ifaces []NetInterface, lnetLabel string, netInfo NetInfo, err error) error {
138+
return nil
139+
},
134140
},
135141
expectedErrSubstr: "No VNIC identified on worker node to configure lnet.",
136142
},
@@ -179,9 +185,13 @@ func TestLnetService_SetupLnet_TableDriven(t *testing.T) {
179185
}
180186
return "ok", nil
181187
},
182-
GetLnetInfoByLnetLabelFunc: func(lnetLabel string) (NetInfo, error) { return NetInfo{}, nil },
183-
ConfigureLnetFunc: func(logger *zap.SugaredLogger, ifaces []NetInterface, lnetLabel string, netInfo NetInfo) error { return nil },
184-
VerifyLnetConfigurationFunc: func(logger *zap.SugaredLogger, ifaces []NetInterface, lnetLabel string, netInfo NetInfo, err error) error { return nil },
188+
GetLnetInfoByLnetLabelFunc: func(lnetLabel string) (NetInfo, error) { return NetInfo{}, nil },
189+
ConfigureLnetFunc: func(logger *zap.SugaredLogger, ifaces []NetInterface, lnetLabel string, netInfo NetInfo) error {
190+
return nil
191+
},
192+
VerifyLnetConfigurationFunc: func(logger *zap.SugaredLogger, ifaces []NetInterface, lnetLabel string, netInfo NetInfo, err error) error {
193+
return nil
194+
},
185195
},
186196
expectedErrSubstr: "Failed to load lnet kernel module with error : loading lnet module failed. Please make sure that lustre client packages are installed on worker nodes.",
187197
},
@@ -200,9 +210,13 @@ func TestLnetService_SetupLnet_TableDriven(t *testing.T) {
200210
}
201211
return "ok", nil
202212
},
203-
GetLnetInfoByLnetLabelFunc: func(lnetLabel string) (NetInfo, error) { return NetInfo{}, nil },
204-
ConfigureLnetFunc: func(logger *zap.SugaredLogger, ifaces []NetInterface, lnetLabel string, netInfo NetInfo) error { return nil },
205-
VerifyLnetConfigurationFunc: func(logger *zap.SugaredLogger, ifaces []NetInterface, lnetLabel string, netInfo NetInfo, err error) error { return nil },
213+
GetLnetInfoByLnetLabelFunc: func(lnetLabel string) (NetInfo, error) { return NetInfo{}, nil },
214+
ConfigureLnetFunc: func(logger *zap.SugaredLogger, ifaces []NetInterface, lnetLabel string, netInfo NetInfo) error {
215+
return nil
216+
},
217+
VerifyLnetConfigurationFunc: func(logger *zap.SugaredLogger, ifaces []NetInterface, lnetLabel string, netInfo NetInfo, err error) error {
218+
return nil
219+
},
206220
},
207221
expectedErrSubstr: "Failed to configure lnet kernel service with error : lnet service configuration failed. Please make sure that lustre client packages are installed on worker nodes.",
208222
},
@@ -289,9 +303,9 @@ func TestLnetService_SetupLnet_TableDriven(t *testing.T) {
289303
func TestLnetService_IsLnetActive_TableDriven(t *testing.T) {
290304
logger := zap.NewExample().Sugar()
291305
tests := []struct {
292-
name string
293-
lnetLabel string
294-
fakeGetInfo func(lnetLabel string) (NetInfo, error)
306+
name string
307+
lnetLabel string
308+
fakeGetInfo func(lnetLabel string) (NetInfo, error)
295309
expectedActive bool
296310
}{
297311
{
@@ -367,10 +381,10 @@ func TestLnetService_IsLnetActive_TableDriven(t *testing.T) {
367381
func TestLnetService_ApplyLustreParameters(t *testing.T) {
368382
logger := zap.NewExample().Sugar()
369383
tests := []struct {
370-
name string
371-
lustreParamsJSON string
372-
fakeExec func(args ...string) (string, error)
373-
expectedErr bool
384+
name string
385+
lustreParamsJSON string
386+
fakeExec func(args ...string) (string, error)
387+
expectedErr bool
374388
}{
375389
{
376390
name: "Valid Lustre Parameters Json Provided",
@@ -417,38 +431,37 @@ func TestLnetService_ApplyLustreParameters(t *testing.T) {
417431
}
418432
}
419433

420-
421434
func TestLnetService_ValidateLustreParameters(t *testing.T) {
422435
logger := zap.NewExample().Sugar()
423436
tests := []struct {
424-
name string
425-
lustreParamsJSON string
426-
expectedErr error
437+
name string
438+
lustreParamsJSON string
439+
expectedErr error
427440
}{
428441
{
429442
name: "Valid Lustre Parameters Json Provided",
430443
lustreParamsJSON: `[{"failover.recovery_mode":"quorum","lnet.debug":"0x200"}]`,
431-
expectedErr: nil,
444+
expectedErr: nil,
432445
},
433446
{
434447
name: "No Lustre Parameters Provided",
435448
lustreParamsJSON: "",
436-
expectedErr: nil,
449+
expectedErr: nil,
437450
},
438451
{
439452
name: "Invalid Lustre Parameters Json Provided",
440453
lustreParamsJSON: `invalid-json`,
441-
expectedErr: fmt.Errorf("%s","invalid character 'i' looking for beginning of value"),
454+
expectedErr: fmt.Errorf("%s", "invalid character 'i' looking for beginning of value"),
442455
},
443456
{
444457
name: "Valid and Invalid Lustre Parameters Provided",
445458
lustreParamsJSON: `[{"failover.recovery_mode":"quorum","lnet.debug":"0x200","lnet.debug && ls -l | wc -l":"0x200 I am Invalid"}]`,
446-
expectedErr: fmt.Errorf("%v","lnet.debug && ls -l | wc -l=0x200 I am Invalid"),
459+
expectedErr: fmt.Errorf("%v", "lnet.debug && ls -l | wc -l=0x200 I am Invalid"),
447460
},
448461
{
449462
name: "Invalid Lustre Parameters Provided",
450463
lustreParamsJSON: `[{"failover.recovery_mode;cat /var/log/cloud-init.log":"quorum; echo Hello"}]`,
451-
expectedErr: fmt.Errorf("%v","failover.recovery_mode;cat /var/log/cloud-init.log=quorum; echo Hello"),
464+
expectedErr: fmt.Errorf("%v", "failover.recovery_mode;cat /var/log/cloud-init.log=quorum; echo Hello"),
452465
},
453466
}
454467

‎pkg/csi/driver/lustre_node.go‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,7 @@ func (d LustreNodeDriver) loadCSIConfig() {
158158

159159

160160
func (d LustreNodeDriver) NodeUnstageVolume(ctx context.Context, req *csi.NodeUnstageVolumeRequest) (*csi.NodeUnstageVolumeResponse, error) {
161+
161162
if req.VolumeId == "" {
162163
return nil, status.Error(codes.InvalidArgument, "Volume ID must be provided")
163164
}
@@ -253,7 +254,10 @@ func (d LustreNodeDriver) NodePublishVolume(ctx context.Context, req *csi.NodePu
253254
logger := d.logger.With("volumeID", req.VolumeId)
254255
logger.Debugf("volume context: %v", req.VolumeContext)
255256

256-
_, lnetLabel := csi_util.ValidateLustreVolumeId(req.VolumeId)
257+
isValidVolumeId, lnetLabel := csi_util.ValidateLustreVolumeId(req.VolumeId)
258+
if !isValidVolumeId {
259+
return nil, status.Error(codes.InvalidArgument, "Invalid Volume Handle provided.")
260+
}
257261

258262
//Lnet Setup
259263
if setupLnet, ok := req.GetVolumeContext()[SetupLnet]; ok && setupLnet == "true" {
@@ -324,6 +328,7 @@ func (d LustreNodeDriver) NodePublishVolume(ctx context.Context, req *csi.NodePu
324328
}
325329

326330
func (d LustreNodeDriver) NodeUnpublishVolume(ctx context.Context, req *csi.NodeUnpublishVolumeRequest) (*csi.NodeUnpublishVolumeResponse, error) {
331+
327332
if req.VolumeId == "" {
328333
return nil, status.Error(codes.InvalidArgument, "NodeUnpublishVolume: Volume ID must be provided")
329334
}

0 commit comments

Comments
 (0)