From 40f55ca3fe06d2e5d0232c1f07911ea728fd1bc1 Mon Sep 17 00:00:00 2001
From: Ed Santiago <santiago@redhat.com>
Date: Wed, 25 Dec 2019 11:51:06 -0700
Subject: signal parsing - better input validation

The helper function we use for signal name mapping does not
check for negative numbers nor invalid (too-high) ones. This
can yield unexpected error messages:

   # podman kill -s -1 foo
   ERRO[0000] unknown signal "18446744073709551615"

This PR introduces a small wrapper for it that:

  1) Strips off a leading dash, allowing '-1' or '-HUP'
     as valid inputs; and
  2) Rejects numbers <1 or >64 (SIGRTMAX)

Also adds a test suite checking signal handling as well as
ensuring that invalid signals are rejected by the command line.

Fixes: #4746

Signed-off-by: Ed Santiago <santiago@redhat.com>
---
 pkg/util/utils.go | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

(limited to 'pkg')

diff --git a/pkg/util/utils.go b/pkg/util/utils.go
index 5b4dfe9fa..f7d04c73b 100644
--- a/pkg/util/utils.go
+++ b/pkg/util/utils.go
@@ -9,6 +9,7 @@ import (
 	"strconv"
 	"strings"
 	"sync"
+	"syscall"
 	"time"
 
 	"github.com/BurntSushi/toml"
@@ -284,9 +285,7 @@ func GetImageConfig(changes []string) (ImageConfig, error) {
 			config.Labels[key] = val
 		case "STOPSIGNAL":
 			// Check the provided signal for validity.
-			// TODO: Worth checking range? ParseSignal allows
-			// negative numbers.
-			killSignal, err := signal.ParseSignal(value)
+			killSignal, err := ParseSignal(value)
 			if err != nil {
 				return ImageConfig{}, errors.Wrapf(err, "invalid change %q - KILLSIGNAL must be given a valid signal", change)
 			}
@@ -305,6 +304,22 @@ func GetImageConfig(changes []string) (ImageConfig, error) {
 	return config, nil
 }
 
+// Parse and validate a signal name or number
+func ParseSignal(rawSignal string) (syscall.Signal, error) {
+	// Strip off leading dash, to allow -1 or -HUP
+	basename := strings.TrimPrefix(rawSignal, "-")
+
+	signal, err := signal.ParseSignal(basename)
+	if err != nil {
+		return -1, err
+	}
+	// 64 is SIGRTMAX; wish we could get this from a standard Go library
+	if signal < 1 || signal > 64 {
+		return -1, errors.Errorf("valid signals are 1 through 64")
+	}
+	return signal, nil
+}
+
 // ParseIDMapping takes idmappings and subuid and subgid maps and returns a storage mapping
 func ParseIDMapping(mode namespaces.UsernsMode, UIDMapSlice, GIDMapSlice []string, subUIDMap, subGIDMap string) (*storage.IDMappingOptions, error) {
 	options := storage.IDMappingOptions{
-- 
cgit v1.2.3-54-g00ecf