Add path safety fixes (#3061)

* Add path safety fixes Signed-off-by: Sanket Sudake <sanketsudake@gmail.com> * minor changes Signed-off-by: Sanket Sudake <sanketsudake@gmail.com> * revert test_huge_response test Signed-off-by: Sanket Sudake <sanketsudake@gmail.com> --------- Signed-off-by: Sanket Sudake <sanketsudake@gmail.com>
2024-11-18 12:19:34 +05:30
parent 82c0fbaeda
commit 59267e3a6b
5 changed files with 57 additions and 12 deletions
@@ -121,6 +121,12 @@ func (builder *Builder) Handler(w http.ResponseWriter, r *http.Request) {
 	}
 	logger.Info("builder received request", zap.Any("request", req))

+	if !utils.ValidateFilePathComponent(req.SrcPkgFilename) {
+		e := "invalid source package filename"
+		logger.Error(e, zap.String("filename", req.SrcPkgFilename))
+		builder.reply(r.Context(), w, "", e, http.StatusBadRequest)
+		return
+	}
 	logger.Debug("starting build")
 	srcPkgPath := filepath.Join(builder.sharedVolumePath, req.SrcPkgFilename)
 	deployPkgFilename := fmt.Sprintf("%s-%s", req.SrcPkgFilename, strings.ToLower(uniuri.NewLen(6)))
@@ -304,16 +304,18 @@ func (gp *GenericPool) choosePod(ctx context.Context, newLabels map[string]strin
 			// Relabel.  If the pod already got picked and
 			// modified, this should fail; in that case just
 			// retry.
-			labelPatch, _ := json.Marshal(newLabels)
-
 			// Append executor instance id to pod annotations to
 			// indicate this pod is managed by this executor.
 			annotations := gp.getDeployAnnotations(gp.env)
-			annotationPatch, _ := json.Marshal(annotations)
-
-			patch := fmt.Sprintf(`{"metadata":{"annotations":%v, "labels":%v}}`, string(annotationPatch), string(labelPatch))
-			logger.Info("relabel pod", zap.String("pod", patch))
-			newPod, err := gp.kubernetesClient.CoreV1().Pods(chosenPod.Namespace).Patch(ctx, chosenPod.Name, k8sTypes.StrategicMergePatchType, []byte(patch), metav1.PatchOptions{})
+			patch := map[string]interface{}{
+				"metadata": map[string]interface{}{
+					"annotations": annotations,
+					"labels":      newLabels,
+				},
+			}
+			patchBytes, _ := json.Marshal(patch)
+			logger.Info("relabel pod", zap.String("pod", string((patchBytes))))
+			newPod, err := gp.kubernetesClient.CoreV1().Pods(chosenPod.Namespace).Patch(ctx, chosenPod.Name, k8sTypes.StrategicMergePatchType, patchBytes, metav1.PatchOptions{})
 			if err != nil && errors.Is(err, context.Canceled) {
 				// ending retry loop when the request canceled
 				gp.readyPodQueue.Done(key)
@@ -253,9 +253,9 @@ func (fetcher *Fetcher) SpecializeHandler(w http.ResponseWriter, r *http.Request
 func (fetcher *Fetcher) Fetch(ctx context.Context, pkg *fv1.Package, req FunctionFetchRequest) (int, error) {
 	logger := otelUtils.LoggerWithTraceID(ctx, fetcher.logger)

-	// check that the requested filename is not an empty string and error out if so
-	if len(req.Filename) == 0 {
-		e := "fetch request received for an empty file name"
+	// check that the requested filename is not an empty string and doest not contain any path traversal
+	if !utils.ValidateFilePathComponent(req.Filename) {
+		e := "fetch request received for an invalid file name"
 		logger.Error(e, zap.Any("request", req))
 		return http.StatusBadRequest, errors.New(fmt.Sprintf("%s, request: %v", e, req))
 	}
@@ -406,6 +406,11 @@ func (fetcher *Fetcher) FetchSecretsAndCfgMaps(ctx context.Context, secrets []fv
 				return httpCode, errors.New(e)
 			}

+			if !utils.ValidateFilePathComponent(secret.Namespace) && !utils.ValidateFilePathComponent(secret.Name) {
+				e := "fetch request received for an invalid secret name or namespace"
+				logger.Error(e, zap.Any("request", secret))
+				return http.StatusBadRequest, errors.New(fmt.Sprintf("%s, request: %v", e, secret))
+			}
 			secretPath := filepath.Join(secret.Namespace, secret.Name)
 			secretDir := filepath.Join(fetcher.sharedSecretPath, secretPath)
 			err = os.MkdirAll(secretDir, os.ModeDir|0750)
@@ -454,6 +459,12 @@ func (fetcher *Fetcher) FetchSecretsAndCfgMaps(ctx context.Context, secrets []fv
 				return httpCode, errors.New(e)
 			}

+			if !utils.ValidateFilePathComponent(config.Namespace) && !utils.ValidateFilePathComponent(config.Name) {
+				e := "fetch request received for an invalid configmap name or namespace"
+				logger.Error(e, zap.Any("request", config))
+				return http.StatusBadRequest, errors.New(fmt.Sprintf("%s, request: %v", e, config))
+			}
+
 			configPath := filepath.Join(config.Namespace, config.Name)
 			configDir := filepath.Join(fetcher.sharedConfigPath, configPath)
 			err = os.MkdirAll(configDir, os.ModeDir|0750)
@@ -519,6 +530,13 @@ func (fetcher *Fetcher) UploadHandler(w http.ResponseWriter, r *http.Request) {
 		http.Error(w, err.Error(), http.StatusBadRequest)
 		return
 	}
+
+	if !utils.ValidateFilePathComponent(req.Filename) {
+		logger.Error("invalid filename in request", zap.String("filename", req.Filename))
+		http.Error(w, "Invalid file name", http.StatusBadRequest)
+		return
+	}
+
 	logger.Info("fetcher received upload request", zap.Any("request", req))

 	zipFilename := req.Filename + ".zip"
@@ -96,7 +96,7 @@ func CreateMissingPermissionForSA(ctx context.Context, kubernetesClient kubernet
 	if enableSA {
 		interval := getSAInterval()
 		logger.Debug("interval value", zap.Any("interval", interval))
-		sa := getSAObj(ctx, kubernetesClient, logger)
+		sa := getSAObj(kubernetesClient, logger)
 		logger.Info("Starting service account check", zap.Any("interval", interval))
 		if interval > 0 {
 			go wait.UntilWithContext(ctx, sa.runSACheck, interval)
@@ -106,7 +106,7 @@ func CreateMissingPermissionForSA(ctx context.Context, kubernetesClient kubernet
 	}
 }

-func getSAObj(ctx context.Context, kubernetesClient kubernetes.Interface, logger *zap.Logger) *ServiceAccount {
+func getSAObj(kubernetesClient kubernetes.Interface, logger *zap.Logger) *ServiceAccount {
 	saObj := &ServiceAccount{
 		kubernetesClient: kubernetesClient,
 		logger:           logger,
@@ -180,6 +180,10 @@ func isHttp2xxSuccessful(status int) bool {
 }

 func DownloadUrl(ctx context.Context, httpClient *http.Client, url string, localPath string) error {
+	// validate local path for directory traversal attacks
+	if filepath.Clean(localPath) != localPath {
+		return errors.Errorf("invalid local path: %s", localPath)
+	}
 	resp, err := ctxhttp.Get(ctx, httpClient, url)
 	if err != nil {
 		return err
@@ -305,3 +309,18 @@ func IsOwnerReferencesEnabled() bool {
 	disableOwnerReference, _ := strconv.ParseBool(os.Getenv(ENV_DISABLE_OWNER_REFERENCES))
 	return !disableOwnerReference
 }
+
+// ValidateFilePathComponent checks if the filename is valid to prevent directory traversal attacks.
+func ValidateFilePathComponent(filename string) bool {
+	return len(filename) > 0 && !containsInvalidChars(filename)
+}
+
+func containsInvalidChars(filename string) bool {
+	invalidChars := []string{"/", "\\", ".."}
+	for _, char := range invalidChars {
+		if strings.Contains(filename, char) {
+			return true
+		}
+	}
+	return false
+}