testing: add Fuzz tests

auth/authtoken/parse.go

@@ -74,7 +74,7 @@ func Parse(s string, verifyFn VerifyFunc) (*Token, bool, error) {
 
 	switch data[1] {
 	case 2:
-		if len(data) < 26 {
+		if len(data) < 27 {
 			return nil, false, validation.NewGenericError("invalid length")
 		}
 		t := &Token{

auth/authtoken/parse_test.go

@@ -0,0 +1,37 @@
+package authtoken
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func FuzzParse(f *testing.F) {
+	f.Add("01020304-0506-0708-090a-0b0c0d0e0f10") // v0
+	f.Add("01020304-0506-0708-090a-0b0c0d0e0f1z") // v0 invalid
+
+	f.Add("U9obklyVC0wduWIy75nbivABDxwc-rANyqNA4CZQzhkJHuNlUCfJDPpcG6W9bEIPddqPbh-sxMS1Km87jC9yLASp3i1UWtdDu2udCzM=")  // v1
+	f.Add("U9obklyVC0wduWIy75nbivABDxwc-rANyqNA4CZQzhkJHuNlUCfJDPpcG6W9bEIPddqPbh-sxMS1Km87jC9yLASp3i1UWtdDu2udCzM==") // v1, invalid base64
+
+	f.Add("VgICAQIDBAUGBwgJCgsMDQ4PEAAAAAAAAAU5c2ln") // v2
+
+	f.Fuzz(func(t *testing.T, a string) {
+		verifyFn := func(t Type, payload, signature []byte) (isValid bool, isOldKey bool) {
+			return true, true
+		}
+		tok, _, err := Parse(a, verifyFn)
+		if err != nil {
+			return
+		}
+
+		s, err := tok.Encode(func(payload []byte) (signature []byte, err error) {
+			return []byte("sig"), nil
+		})
+		require.NoError(t, err)
+
+		tok2, _, err := Parse(s, verifyFn)
+		require.NoError(t, err)
+		assert.Equal(t, tok, tok2)
+	})
+}

auth/authtoken/testdata/fuzz/FuzzParse/55252af6e78583606e5e18afbd3c823376f613401644d6fbc0ac13fe08a6a784

@@ -0,0 +1,2 @@
+go test fuzz v1
+string("VgI00000000000000000000000000000000")

auth/handler.go

@@ -631,6 +631,10 @@ func (h *Handler) tryAuthUser(ctx context.Context, w http.ResponseWriter, req *h
 // Updating and clearing the session cookie is automatically handled.
 func (h *Handler) WrapHandler(wrapped http.Handler) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
+		if strings.HasPrefix(req.URL.Path, "/api/v2/slack") {
+			wrapped.ServeHTTP(w, req)
+			return
+		}
 		if req.URL.Path == "/api/v2/mailgun/incoming" || req.URL.Path == "/v1/webhooks/mailgun" {
 			// Mailgun handles it's own auth and has special
 			// requirements on status codes, so we pass it through

notification/slack/servemessageaction.go

@@ -1,9 +1,8 @@
 package slack
 
 import (
+	"bytes"
 	"crypto/hmac"
-	"crypto/sha256"
-	"encoding/hex"
 	"encoding/json"
 	"errors"
 	"fmt"
@@ -17,46 +16,45 @@ import (
 	"github.com/target/goalert/auth/authlink"
 	"github.com/target/goalert/config"
 	"github.com/target/goalert/notification"
+	"github.com/target/goalert/permission"
 	"github.com/target/goalert/util/errutil"
 	"github.com/target/goalert/util/log"
 	"github.com/target/goalert/validation"
 )
 
-func validateRequestSignature(req *http.Request) error {
+func validateRequestSignature(now time.Time, req *http.Request) error {
 	cfg := config.FromContext(req.Context())
 
-	var newBody struct {
-		io.Reader
-		io.Closer
+	if req.Form != nil {
+		return errors.New("request already parsed, can't validate signature")
 	}
 
-	h := hmac.New(sha256.New, []byte(cfg.Slack.SigningSecret))
-	tsStr := req.Header.Get("X-Slack-Request-Timestamp")
-	io.WriteString(h, "v0:"+tsStr+":")
-	newBody.Reader = io.TeeReader(req.Body, h)
-	newBody.Closer = req.Body
-	req.Body = newBody
-
-	err := req.ParseForm()
-	if err != nil {
-		return fmt.Errorf("failed to parse form: %w", err)
+	// copy body data
+	var buf bytes.Buffer
+	if req.Body != nil {
+		orig := req.Body
+		req.Body = io.NopCloser(io.TeeReader(req.Body, &buf))
+		err := req.ParseForm()
+		orig.Close()
+		if err != nil {
+			return err
+		}
 	}
 
-	ts, err := strconv.ParseInt(tsStr, 10, 64)
+	// read ts
+	tsStr := req.Header.Get("X-Slack-Request-Timestamp")
+	unixSec, err := strconv.ParseInt(tsStr, 10, 64)
 	if err != nil {
-		return fmt.Errorf("failed to parse timestamp: %w", err)
+		return permission.Unauthorized()
 	}
-	diff := time.Since(time.Unix(ts, 0))
-	if diff < 0 {
-		diff = -diff
-	}
-	if diff > 5*time.Minute {
-		return fmt.Errorf("timestamp too old: %s", diff)
+	ts := time.Unix(unixSec, 0)
+	if now.Sub(ts).Abs() > 5*time.Minute {
+		return permission.Unauthorized()
 	}
 
-	sig := "v0=" + hex.EncodeToString(h.Sum(nil))
-	if hmac.Equal([]byte(req.Header.Get("X-Slack-Signature")), []byte(sig)) {
-		return fmt.Errorf("invalid signature")
+	properSig := Signature(cfg.Slack.SigningSecret, ts, buf.Bytes())
+	if !hmac.Equal([]byte(req.Header.Get("X-Slack-Signature")), []byte(properSig)) {
+		return permission.Unauthorized()
 	}
 
 	return nil
@@ -71,9 +69,9 @@ func (s *ChannelSender) ServeMessageAction(w http.ResponseWriter, req *http.Requ
 		return
 	}
 
-	err := validateRequestSignature(req)
-	if err != nil {
-		http.Error(w, err.Error(), http.StatusBadRequest)
+	err := validateRequestSignature(time.Now(), req)
+	if errutil.HTTPError(ctx, w, err) {
+		return
 	}
 
 	var payload struct {

notification/slack/servemessageaction_test.go

@@ -0,0 +1,41 @@
+package slack
+
+import (
+	"context"
+	"net/http"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"github.com/target/goalert/config"
+	"github.com/target/goalert/permission"
+)
+
+func TestValidateRequestSignature(t *testing.T) {
+	// Values pulled directly from: https://api.slack.com/authentication/verifying-requests-from-slack
+	var cfg config.Config
+	cfg.Slack.SigningSecret = "8f742231b10e8888abcd99yyyzzz85a5"
+
+	req, err := http.NewRequestWithContext(cfg.Context(context.Background()), "POST", "http://example.com", strings.NewReader("token=xyzz0WbapA4vBCDEFasx0q6G&team_id=T1DC2JH3J&team_domain=testteamnow&channel_id=G8PSS9T3V&channel_name=foobar&user_id=U2CERLKJA&user_name=roadrunner&command=%2Fwebhook-collect&text=&response_url=https%3A%2F%2Fhooks.slack.com%2Fcommands%2FT1DC2JH3J%2F397700885554%2F96rGlfmibIGlgcZRskXaIFfN&trigger_id=398738663015.47445629121.803a0bc887a14d10d2c447fce8b6703c"))
+	require.NoError(t, err)
+
+	req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+	req.Header.Set("X-Slack-Request-Timestamp", "1531420618")
+	req.Header.Set("X-Slack-Signature", "v0=a2114d57b48eac39b9ad189dd8316235a7b4a8d21a10bd27519666489c69b503")
+
+	err = validateRequestSignature(time.Unix(1531420618, 0), req)
+	assert.NoError(t, err)
+
+	req, err = http.NewRequestWithContext(cfg.Context(context.Background()), "POST", "http://example.com", strings.NewReader("token=xyzz0WbapA4vBCDEFasx0q6G&team_id=T1DC2JH3J&team_domain=testteamnow&channel_id=G8PSS9T3V&channel_name=foobar&user_id=U2CERLKJA&user_name=roadrunner&command=%2Fwebhook-collect&text=&response_url=https%3A%2F%2Fhooks.slack.com%2Fcommands%2FT1DC2JH3J%2F397700885554%2F96rGlfmibIGlgcZRskXaIFfN&trigger_id=398738663015.47445629121.803a0bc887a14d10d2c447fce8b6703c"))
+	require.NoError(t, err)
+
+	req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+	req.Header.Set("X-Slack-Request-Timestamp", "15314206189") // changed timestamp
+	req.Header.Set("X-Slack-Signature", "v0=a2114d57b48eac39b9ad189dd8316235a7b4a8d21a10bd27519666489c69b503")
+
+	// different timestamp should invalidate the signature
+	err = validateRequestSignature(time.Unix(1531420618, 0), req)
+	assert.True(t, permission.IsUnauthorized(err), "expected unauthorized error, got: %v", err)
+}

notification/slack/signature.go

@@ -0,0 +1,20 @@
+package slack
+
+import (
+	"crypto/hmac"
+	"crypto/sha256"
+	"encoding/hex"
+	"fmt"
+	"time"
+)
+
+// Signature generates a signature for a Slack request.
+func Signature(signingSecret string, ts time.Time, data []byte) string {
+	h := hmac.New(sha256.New, []byte(signingSecret))
+	_, err := fmt.Fprintf(h, "v0:%d:%s", ts.Unix(), data)
+	if err != nil {
+		panic(err)
+	}
+
+	return "v0=" + hex.EncodeToString(h.Sum(nil))
+}

notification/slack/signature_test.go

@@ -0,0 +1,15 @@
+package slack
+
+import (
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestSignature(t *testing.T) {
+	// example pulled directly from: https://api.slack.com/authentication/verifying-requests-from-slack
+	sig := Signature("8f742231b10e8888abcd99yyyzzz85a5", time.Unix(1531420618, 0), []byte("token=xyzz0WbapA4vBCDEFasx0q6G&team_id=T1DC2JH3J&team_domain=testteamnow&channel_id=G8PSS9T3V&channel_name=foobar&user_id=U2CERLKJA&user_name=roadrunner&command=%2Fwebhook-collect&text=&response_url=https%3A%2F%2Fhooks.slack.com%2Fcommands%2FT1DC2JH3J%2F397700885554%2F96rGlfmibIGlgcZRskXaIFfN&trigger_id=398738663015.47445629121.803a0bc887a14d10d2c447fce8b6703c"))
+
+	assert.Equal(t, "v0=a2114d57b48eac39b9ad189dd8316235a7b4a8d21a10bd27519666489c69b503", sig)
+}

validation/validate/jmespath.go

@@ -1,12 +1,20 @@
 package validate
 
 import (
+	"unicode"
+
 	"github.com/jmespath/go-jmespath"
 	"github.com/target/goalert/validation"
 )
 
 // JMESPath will validate a JMESPath expression.
 func JMESPath(fname, expression string) error {
+	for _, c := range expression {
+		if !unicode.IsPrint(c) && c != '\t' && c != '\n' {
+			return validation.NewFieldError(fname, "only printable characters allowed")
+		}
+	}
+
 	_, err := jmespath.Compile(expression)
 	if err != nil {
 		return validation.NewFieldError(fname, err.Error())

validation/validate/jmespath_test.go

@@ -6,6 +6,19 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
+func FuzzJMESPath(f *testing.F) {
+	invalid := []string{
+		"foobar || ", "foo.bar", "foobar",
+	}
+	for _, s := range invalid {
+		f.Add(s)
+	}
+
+	f.Fuzz(func(t *testing.T, s string) {
+		JMESPath("Name", s)
+	})
+}
+
 func TestJMESPath(t *testing.T) {
 	err := JMESPath("test", "foobar")
 	assert.NoError(t, err)
@@ -15,5 +28,4 @@ func TestJMESPath(t *testing.T) {
 
 	err = JMESPath("test", "foobar || ")
 	assert.Error(t, err)
-
 }

validation/validate/phone_test.go

@@ -4,6 +4,34 @@ import (
 	"testing"
 )
 
+func FuzzPhone(f *testing.F) {
+	numbers := []string{
+		"+17633453456",
+		"+919632040000",
+		"+17734562190",
+		"+916301210000",
+		"+447480809090",
+		"+61455518786",   // Australia
+		"+498963648018",  // Germany
+		"+85268355559",   // Hong Kong
+		"+8618555196185", // China
+		"+50223753964",   // Guatemala
+		"+10633453456",
+		"+15555555555",
+		"+4474808090",
+		"+611111111111",
+		"+491515559510",
+		"+85211111111",
+	}
+	for _, number := range numbers {
+		f.Add(number)
+	}
+
+	f.Fuzz(func(t *testing.T, number string) {
+		Phone("Number", number)
+	})
+}
+
 func TestPhone(t *testing.T) {
 	check := func(number string, expValid bool) {
 		name := "valid"
@@ -47,5 +75,4 @@ func TestPhone(t *testing.T) {
 	for _, number := range invalid {
 		check(number, false)
 	}
-
 }

validation/validate/testdata/fuzz/FuzzJMESPath/56b1dff6e89281145efa3f46de8e3710d757f144714d54b9b5826e7701ded458

@@ -0,0 +1,2 @@
+go test fuzz v1
+string("A\u0080")