fix: reject inverted TCP/UDP port ranges when creating security group rules

spinifex/handlers/ec2/vpc/security_group.go

@@ -59,6 +59,16 @@ func normalizeIPProtocol(proto string) (string, error) {
 	return "", fmt.Errorf("invalid IpProtocol %q: supported values are tcp, udp, icmp, -1 (or 6, 17, 1)", proto)
 }
 
+// validateSGRulePorts rejects a tcp or udp rule whose FromPort exceeds its ToPort.
+// Only create paths call it, so a rule stored before the check can still be
+// revoked or re-described by its ports.
+func validateSGRulePorts(proto string, fromPort, toPort int64) error {
+	if (proto == "tcp" || proto == "udp") && fromPort > toPort {
+		return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "Invalid TCP/UDP port range(%d:%d)", fromPort, toPort)
+	}
+	return nil
+}
+
 // validateSGRule rejects CidrIp values that are non-canonical or IPv6, CidrIpv6
 // values that are non-canonical or IPv4, IpProtocol values the ACL builder
 // cannot express, and SourceSG values not matching the sg-ID format. At least
@@ -995,6 +1005,9 @@ func (s *VPCServiceImpl) AuthorizeSecurityGroupIngress(ctx context.Context, inpu
 	newRules, err := ipPermissionsToSGRules(input.IpPermissions, sgParseAuthorize)
 	if err != nil {
 		slog.WarnContext(ctx, "AuthorizeSecurityGroupIngress: invalid rule", "groupId", groupId, "err", err)
+		if _, ok := awserrors.ResolveErrorCode(err); ok {
+			return nil, err
+		}
 		return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "%s", err)
 	}
 	newRules = applySGRuleTags(newRules, input.TagSpecifications)
@@ -1075,6 +1088,9 @@ func (s *VPCServiceImpl) AuthorizeSecurityGroupEgress(ctx context.Context, input
 	newRules, err := ipPermissionsToSGRules(input.IpPermissions, sgParseAuthorize)
 	if err != nil {
 		slog.WarnContext(ctx, "AuthorizeSecurityGroupEgress: invalid rule", "groupId", groupId, "err", err)
+		if _, ok := awserrors.ResolveErrorCode(err); ok {
+			return nil, err
+		}
 		return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "%s", err)
 	}
 	newRules = applySGRuleTags(newRules, input.TagSpecifications)
@@ -1695,6 +1711,9 @@ func sgRuleRequestToSGRule(req *ec2.SecurityGroupRuleRequest) (SGRule, error) {
 				"Invalid value for portRange. Must specify both from and to ports with TCP/UDP.")
 		}
 		r.FromPort, r.ToPort = *req.FromPort, *req.ToPort
+		if err := validateSGRulePorts(proto, r.FromPort, r.ToPort); err != nil {
+			return SGRule{}, err
+		}
 	case allProtocols:
 		// Stored as 0/0, the form authorize stores for the no-ports case, so the
 		// rule's sgRuleKey matches Terraform's and the default egress rule's.
@@ -1825,6 +1844,11 @@ func ipPermissionsToSGRules(perms []*ec2.IpPermission, mode sgParseMode) ([]SGRu
 		if perm.ToPort != nil {
 			toPort = *perm.ToPort
 		}
+		if mode == sgParseAuthorize {
+			if err := validateSGRulePorts(proto, fromPort, toPort); err != nil {
+				return nil, err
+			}
+		}
 
 		appended := false
 		for _, ipRange := range perm.IpRanges {

spinifex/handlers/ec2/vpc/security_group_modify_test.go

@@ -7,6 +7,7 @@ package handlers_ec2_vpc
 import (
 	"context"
 	"encoding/json"
+	"fmt"
 	"sync"
 	"testing"
 
@@ -631,3 +632,85 @@ func TestModifySecurityGroupRules_ConcurrentAuthorize(t *testing.T) {
 	assert.Equal(t, modErr == nil, modified, "the modify succeeded iff its effect is stored")
 	assert.Equal(t, authErr == nil, authorized, "the authorize succeeded iff its effect is stored")
 }
+
+func requireInvertedPortRange(t *testing.T, err error, from, to int64) {
+	t.Helper()
+	require.Error(t, err)
+	code, message, ok := awserrors.ResolveErrorDetail(err)
+	require.True(t, ok, "error %q carries no AWS code", err)
+	assert.Equal(t, awserrors.ErrorInvalidParameterValue, code)
+	assert.Equal(t, fmt.Sprintf("Invalid TCP/UDP port range(%d:%d)", from, to), message)
+}
+
+func TestAuthorizeSecurityGroupRules_RejectsInvertedPortRange(t *testing.T) {
+	t.Parallel()
+	svc := setupTestVPCService(t)
+	vpcID := createTestVPC(t, svc, "10.0.0.0/16")
+	sgID := createTestSG(t, svc, vpcID, "inverted-ports")
+	before := storedSGRecord(t, svc, sgID)
+
+	perm := func(proto string) []*ec2.IpPermission {
+		return []*ec2.IpPermission{{
+			IpProtocol: aws.String(proto), FromPort: aws.Int64(100), ToPort: aws.Int64(50),
+			IpRanges: []*ec2.IpRange{{CidrIp: aws.String("10.0.0.0/24")}},
+		}}
+	}
+	_, err := svc.AuthorizeSecurityGroupIngress(context.Background(), &ec2.AuthorizeSecurityGroupIngressInput{
+		GroupId: aws.String(sgID), IpPermissions: perm("tcp"),
+	}, testAccountID)
+	requireInvertedPortRange(t, err, 100, 50)
+
+	_, err = svc.AuthorizeSecurityGroupEgress(context.Background(), &ec2.AuthorizeSecurityGroupEgressInput{
+		GroupId: aws.String(sgID), IpPermissions: perm("udp"),
+	}, testAccountID)
+	requireInvertedPortRange(t, err, 100, 50)
+
+	assert.Equal(t, before, storedSGRecord(t, svc, sgID), "a rejected authorize may store nothing")
+}
+
+func TestModifySecurityGroupRules_RejectsInvertedPortRange(t *testing.T) {
+	t.Parallel()
+	svc := setupTestVPCService(t)
+	vpcID := createTestVPC(t, svc, "10.0.0.0/16")
+	sgID := createTestSG(t, svc, vpcID, "mod-inverted-ports")
+	ruleID := authorizeIngressTCP(t, svc, sgID, 80, "10.0.0.0/24")
+
+	req := tcpCIDRRequest(100, "10.0.0.0/24")
+	req.ToPort = aws.Int64(50)
+	requireInvertedPortRange(t, modifyRules(svc, sgID, ruleUpdate(ruleID, req)), 100, 50)
+	assert.Equal(t, int64(80), storedSGRule(t, svc, sgID, ruleID).FromPort)
+}
+
+// TestRevokeSecurityGroupIngress_InvertedPortRangeStillRevocable covers a rule
+// stored before the range check existed: revoking it by its ports must work.
+func TestRevokeSecurityGroupIngress_InvertedPortRangeStillRevocable(t *testing.T) {
+	t.Parallel()
+	svc := setupTestVPCService(t)
+	vpcID := createTestVPC(t, svc, "10.0.0.0/16")
+	sgID := createTestSG(t, svc, vpcID, "stale-inverted-ports")
+
+	key := utils.AccountKey(testAccountID, sgID)
+	var rec SecurityGroupRecord
+	require.NoError(t, json.Unmarshal(storedSGRecord(t, svc, sgID), &rec))
+	rec.IngressRules = append(rec.IngressRules, SGRule{
+		RuleId: "sgr-0123456789abcdef0", IpProtocol: "tcp", FromPort: 100, ToPort: 50, CidrIp: "10.0.0.0/24",
+	})
+	data, err := json.Marshal(rec)
+	require.NoError(t, err)
+	_, err = svc.sgKV.Put(t.Context(), key, data)
+	require.NoError(t, err)
+
+	_, err = svc.RevokeSecurityGroupIngress(context.Background(), &ec2.RevokeSecurityGroupIngressInput{
+		GroupId: aws.String(sgID),
+		IpPermissions: []*ec2.IpPermission{{
+			IpProtocol: aws.String("tcp"), FromPort: aws.Int64(100), ToPort: aws.Int64(50),
+			IpRanges: []*ec2.IpRange{{CidrIp: aws.String("10.0.0.0/24")}},
+		}},
+	}, testAccountID)
+	require.NoError(t, err)
+
+	require.NoError(t, json.Unmarshal(storedSGRecord(t, svc, sgID), &rec))
+	for _, r := range rec.IngressRules {
+		assert.NotEqual(t, "sgr-0123456789abcdef0", r.RuleId)
+	}
+}