From 98e136eb7baa2b66f4233d96875c1490144e1594 Mon Sep 17 00:00:00 2001 From: Nikita Pivkin Date: Thu, 29 Aug 2024 06:02:05 +0600 Subject: [PATCH] feat(misconf): port and protocol support for EC2 networks (#7146) Signed-off-by: nikpivkin --- .../cloudformation/aws/ec2/adapt_test.go | 23 ++++++++++--- .../adapters/cloudformation/aws/ec2/nacl.go | 15 +++++++- .../cloudformation/aws/ec2/security_group.go | 19 +++++++++++ pkg/iac/adapters/terraform/aws/ec2/subnet.go | 4 +-- .../adapters/terraform/aws/ec2/subnet_test.go | 2 +- pkg/iac/adapters/terraform/aws/ec2/vpc.go | 34 +++++++++---------- .../adapters/terraform/aws/ec2/vpc_test.go | 24 ++++++++++++- pkg/iac/providers/aws/ec2/vpc.go | 5 +++ .../parser/property_conversion.go | 8 ++++- 9 files changed, 106 insertions(+), 28 deletions(-) diff --git a/pkg/iac/adapters/cloudformation/aws/ec2/adapt_test.go b/pkg/iac/adapters/cloudformation/aws/ec2/adapt_test.go index 01fb201768e9..b218b95a4e99 100644 --- a/pkg/iac/adapters/cloudformation/aws/ec2/adapt_test.go +++ b/pkg/iac/adapters/cloudformation/aws/ec2/adapt_test.go @@ -51,15 +51,15 @@ Resources: SecurityGroupIngress: - IpProtocol: tcp Description: ingress - FromPort: 80 + FromPort: "80" ToPort: 80 CidrIp: 0.0.0.0/0 SecurityGroupEgress: - - IpProtocol: tcp + - IpProtocol: -1 Description: egress FromPort: 80 - ToPort: 80 - CidrIp: 0.0.0.0/0 + ToPort: "80" + CidrIp: "0.0.0.0/0" myNetworkAcl: Type: AWS::EC2::NetworkAcl Properties: @@ -73,6 +73,9 @@ Resources: Protocol: 6 RuleAction: allow CidrBlock: 172.16.0.0/24 + PortRange: + From: 22 + To: "23" myLaunchConfig: Type: AWS::AutoScaling::LaunchConfiguration Properties: @@ -137,6 +140,9 @@ Resources: CIDRs: []types.StringValue{ types.StringTest("0.0.0.0/0"), }, + FromPort: types.IntTest(80), + ToPort: types.IntTest(80), + Protocol: types.StringTest("tcp"), }, }, EgressRules: []ec2.SecurityGroupRule{ @@ -145,6 +151,9 @@ Resources: CIDRs: []types.StringValue{ types.StringTest("0.0.0.0/0"), }, + FromPort: types.IntTest(80), + ToPort: types.IntTest(80), + Protocol: types.StringTest("-1"), }, }, }, @@ -159,6 +168,8 @@ Resources: CIDRs: []types.StringValue{ types.StringTest("172.16.0.0/24"), }, + FromPort: types.IntTest(22), + ToPort: types.IntTest(23), }, }, }, @@ -309,6 +320,8 @@ Resources: CIDRs: []types.StringValue{ types.StringTest("0.0.0.0/0"), }, + FromPort: types.IntTest(-1), + ToPort: types.IntTest(-1), }, }, EgressRules: []ec2.SecurityGroupRule{ @@ -317,6 +330,8 @@ Resources: CIDRs: []types.StringValue{ types.StringTest("0.0.0.0/0"), }, + FromPort: types.IntTest(-1), + ToPort: types.IntTest(-1), }, }, }, diff --git a/pkg/iac/adapters/cloudformation/aws/ec2/nacl.go b/pkg/iac/adapters/cloudformation/aws/ec2/nacl.go index 0f908f78ae08..a91a12569994 100644 --- a/pkg/iac/adapters/cloudformation/aws/ec2/nacl.go +++ b/pkg/iac/adapters/cloudformation/aws/ec2/nacl.go @@ -4,6 +4,7 @@ import ( "strconv" "github.com/aquasecurity/trivy/pkg/iac/providers/aws/ec2" + "github.com/aquasecurity/trivy/pkg/iac/scanners/cloudformation/cftypes" "github.com/aquasecurity/trivy/pkg/iac/scanners/cloudformation/parser" iacTypes "github.com/aquasecurity/trivy/pkg/iac/types" ) @@ -29,7 +30,8 @@ func getRules(id string, ctx parser.FileContext) (rules []ec2.NetworkACLRule) { Metadata: ruleResource.Metadata(), Type: iacTypes.StringDefault(ec2.TypeIngress, ruleResource.Metadata()), Action: iacTypes.StringDefault(ec2.ActionAllow, ruleResource.Metadata()), - Protocol: iacTypes.String("-1", ruleResource.Metadata()), + FromPort: iacTypes.IntDefault(-1, ruleResource.Metadata()), + ToPort: iacTypes.IntDefault(-1, ruleResource.Metadata()), CIDRs: nil, } @@ -62,6 +64,17 @@ func getRules(id string, ctx parser.FileContext) (rules []ec2.NetworkACLRule) { rule.CIDRs = append(rule.CIDRs, ipv6Cidr.AsStringValue()) } + portRange := ruleResource.GetProperty("PortRange") + fromPort := portRange.GetProperty("From").ConvertTo(cftypes.Int) + if fromPort.IsInt() { + rule.FromPort = fromPort.AsIntValue() + } + + toPort := portRange.GetProperty("To").ConvertTo(cftypes.Int) + if toPort.IsInt() { + rule.ToPort = toPort.AsIntValue() + } + rules = append(rules, rule) } } diff --git a/pkg/iac/adapters/cloudformation/aws/ec2/security_group.go b/pkg/iac/adapters/cloudformation/aws/ec2/security_group.go index 6770062affb2..e85a91cdecb6 100644 --- a/pkg/iac/adapters/cloudformation/aws/ec2/security_group.go +++ b/pkg/iac/adapters/cloudformation/aws/ec2/security_group.go @@ -4,6 +4,7 @@ import ( "github.com/samber/lo" "github.com/aquasecurity/trivy/pkg/iac/providers/aws/ec2" + "github.com/aquasecurity/trivy/pkg/iac/scanners/cloudformation/cftypes" "github.com/aquasecurity/trivy/pkg/iac/scanners/cloudformation/parser" "github.com/aquasecurity/trivy/pkg/iac/types" ) @@ -75,7 +76,10 @@ func adaptRule(r interface { rule := ec2.SecurityGroupRule{ Metadata: r.Metadata(), Description: r.GetStringProperty("Description"), + FromPort: types.IntDefault(-1, r.Metadata()), + ToPort: types.IntDefault(-1, r.Metadata()), } + v4Cidr := r.GetProperty("CidrIp") if v4Cidr.IsString() && v4Cidr.AsStringValue().IsNotEmpty() { rule.CIDRs = append(rule.CIDRs, types.StringExplicit(v4Cidr.AsString(), v4Cidr.Metadata())) @@ -85,5 +89,20 @@ func adaptRule(r interface { rule.CIDRs = append(rule.CIDRs, types.StringExplicit(v6Cidr.AsString(), v6Cidr.Metadata())) } + fromPort := r.GetProperty("FromPort").ConvertTo(cftypes.Int) + if fromPort.IsInt() { + rule.FromPort = fromPort.AsIntValue() + } + + toPort := r.GetProperty("ToPort").ConvertTo(cftypes.Int) + if toPort.IsInt() { + rule.ToPort = toPort.AsIntValue() + } + + protocol := r.GetProperty("IpProtocol").ConvertTo(cftypes.String) + if protocol.IsString() { + rule.Protocol = protocol.AsStringValue() + } + return rule } diff --git a/pkg/iac/adapters/terraform/aws/ec2/subnet.go b/pkg/iac/adapters/terraform/aws/ec2/subnet.go index 4e4e49c4f59e..41a68eb2832c 100644 --- a/pkg/iac/adapters/terraform/aws/ec2/subnet.go +++ b/pkg/iac/adapters/terraform/aws/ec2/subnet.go @@ -9,13 +9,13 @@ func adaptSubnets(modules terraform.Modules) []ec2.Subnet { var subnets []ec2.Subnet for _, module := range modules { for _, resource := range module.GetResourcesByType("aws_subnet") { - subnets = append(subnets, adaptSubnet(resource, module)) + subnets = append(subnets, adaptSubnet(resource)) } } return subnets } -func adaptSubnet(resource *terraform.Block, module *terraform.Module) ec2.Subnet { +func adaptSubnet(resource *terraform.Block) ec2.Subnet { mapPublicIpOnLaunchAttr := resource.GetAttribute("map_public_ip_on_launch") mapPublicIpOnLaunchVal := mapPublicIpOnLaunchAttr.AsBoolValueOrDefault(false, resource) diff --git a/pkg/iac/adapters/terraform/aws/ec2/subnet_test.go b/pkg/iac/adapters/terraform/aws/ec2/subnet_test.go index c371ef4f504f..33a0a642245a 100644 --- a/pkg/iac/adapters/terraform/aws/ec2/subnet_test.go +++ b/pkg/iac/adapters/terraform/aws/ec2/subnet_test.go @@ -61,7 +61,7 @@ func Test_adaptSubnet(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { modules := tftestutil.CreateModulesFromSource(t, test.terraform, ".tf") - adapted := adaptSubnet(modules.GetBlocks()[0], modules[0]) + adapted := adaptSubnet(modules.GetBlocks()[0]) testutil.AssertDefsecEqual(t, test.expected, adapted) }) } diff --git a/pkg/iac/adapters/terraform/aws/ec2/vpc.go b/pkg/iac/adapters/terraform/aws/ec2/vpc.go index 92ee730e0947..6be6cbaf672d 100644 --- a/pkg/iac/adapters/terraform/aws/ec2/vpc.go +++ b/pkg/iac/adapters/terraform/aws/ec2/vpc.go @@ -72,9 +72,9 @@ func (a *sgAdapter) adaptSecurityGroups(modules terraform.Modules) []ec2.Securit } for _, sgRule := range orphanResources { if sgRule.GetAttribute("type").Equals("ingress") { - orphanage.IngressRules = append(orphanage.IngressRules, adaptSGRule(sgRule, modules)) + orphanage.IngressRules = append(orphanage.IngressRules, adaptSGRule(sgRule)) } else if sgRule.GetAttribute("type").Equals("egress") { - orphanage.EgressRules = append(orphanage.EgressRules, adaptSGRule(sgRule, modules)) + orphanage.EgressRules = append(orphanage.EgressRules, adaptSGRule(sgRule)) } } securityGroups = append(securityGroups, orphanage) @@ -116,21 +116,21 @@ func (a *sgAdapter) adaptSecurityGroup(resource *terraform.Block, module terrafo ingressBlocks := resource.GetBlocks("ingress") for _, ingressBlock := range ingressBlocks { - ingressRules = append(ingressRules, adaptSGRule(ingressBlock, module)) + ingressRules = append(ingressRules, adaptSGRule(ingressBlock)) } egressBlocks := resource.GetBlocks("egress") for _, egressBlock := range egressBlocks { - egressRules = append(egressRules, adaptSGRule(egressBlock, module)) + egressRules = append(egressRules, adaptSGRule(egressBlock)) } rulesBlocks := module.GetReferencingResources(resource, "aws_security_group_rule", "security_group_id") for _, ruleBlock := range rulesBlocks { a.sgRuleIDs.Resolve(ruleBlock.ID()) if ruleBlock.GetAttribute("type").Equals("ingress") { - ingressRules = append(ingressRules, adaptSGRule(ruleBlock, module)) + ingressRules = append(ingressRules, adaptSGRule(ruleBlock)) } else if ruleBlock.GetAttribute("type").Equals("egress") { - egressRules = append(egressRules, adaptSGRule(ruleBlock, module)) + egressRules = append(egressRules, adaptSGRule(ruleBlock)) } } @@ -154,7 +154,7 @@ func (a *sgAdapter) adaptSecurityGroup(resource *terraform.Block, module terrafo } } -func adaptSGRule(resource *terraform.Block, modules terraform.Modules) ec2.SecurityGroupRule { +func adaptSGRule(resource *terraform.Block) ec2.SecurityGroupRule { ruleDescAttr := resource.GetAttribute("description") ruleDescVal := ruleDescAttr.AsStringValueOrDefault("", resource) @@ -162,16 +162,6 @@ func adaptSGRule(resource *terraform.Block, modules terraform.Modules) ec2.Secur cidrBlocks := resource.GetAttribute("cidr_blocks") ipv6cidrBlocks := resource.GetAttribute("ipv6_cidr_blocks") - varBlocks := modules.GetBlocks().OfType("variable") - - for _, vb := range varBlocks { - if cidrBlocks.IsNotNil() && cidrBlocks.ReferencesBlock(vb) { - cidrBlocks = vb.GetAttribute("default") - } - if ipv6cidrBlocks.IsNotNil() && ipv6cidrBlocks.ReferencesBlock(vb) { - ipv6cidrBlocks = vb.GetAttribute("default") - } - } if cidrBlocks.IsNotNil() { cidrs = cidrBlocks.AsStringValues() @@ -185,6 +175,9 @@ func adaptSGRule(resource *terraform.Block, modules terraform.Modules) ec2.Secur Metadata: resource.GetMetadata(), Description: ruleDescVal, CIDRs: cidrs, + FromPort: resource.GetAttribute("from_port").AsIntValueOrDefault(-1, resource), + ToPort: resource.GetAttribute("to_port").AsIntValueOrDefault(-1, resource), + Protocol: resource.GetAttribute("protocol").AsStringValueOrDefault("", resource), } } @@ -203,6 +196,9 @@ func adaptSingleSGRule(resource *terraform.Block) ec2.SecurityGroupRule { Metadata: resource.GetMetadata(), Description: description, CIDRs: cidrs, + FromPort: resource.GetAttribute("from_port").AsIntValueOrDefault(-1, resource), + ToPort: resource.GetAttribute("to_port").AsIntValueOrDefault(-1, resource), + Protocol: resource.GetAttribute("ip_protocol").AsStringValueOrDefault("", resource), } } @@ -236,7 +232,7 @@ func adaptNetworkACLRule(resource *terraform.Block) ec2.NetworkACLRule { actionVal := actionAttr.AsStringValueOrDefault("", resource) protocolAtrr := resource.GetAttribute("protocol") - protocolVal := protocolAtrr.AsStringValueOrDefault("-1", resource) + protocolVal := protocolAtrr.AsStringValueOrDefault("", resource) cidrAttr := resource.GetAttribute("cidr_block") if cidrAttr.IsNotNil() { @@ -253,5 +249,7 @@ func adaptNetworkACLRule(resource *terraform.Block) ec2.NetworkACLRule { Action: actionVal, Protocol: protocolVal, CIDRs: cidrs, + FromPort: resource.GetAttribute("from_port").AsIntValueOrDefault(-1, resource), + ToPort: resource.GetAttribute("to_port").AsIntValueOrDefault(-1, resource), } } diff --git a/pkg/iac/adapters/terraform/aws/ec2/vpc_test.go b/pkg/iac/adapters/terraform/aws/ec2/vpc_test.go index 611f8ca8527d..1acee8146d3e 100644 --- a/pkg/iac/adapters/terraform/aws/ec2/vpc_test.go +++ b/pkg/iac/adapters/terraform/aws/ec2/vpc_test.go @@ -102,6 +102,9 @@ func Test_AdaptVPC(t *testing.T) { CIDRs: []iacTypes.StringValue{ iacTypes.String("4.5.6.7/32", iacTypes.NewTestMetadata()), }, + FromPort: iacTypes.IntTest(80), + ToPort: iacTypes.IntTest(80), + Protocol: iacTypes.StringTest("tcp"), }, { Metadata: iacTypes.NewTestMetadata(), @@ -111,6 +114,9 @@ func Test_AdaptVPC(t *testing.T) { iacTypes.String("1.2.3.4/32", iacTypes.NewTestMetadata()), iacTypes.String("4.5.6.7/32", iacTypes.NewTestMetadata()), }, + FromPort: iacTypes.IntTest(22), + ToPort: iacTypes.IntTest(22), + Protocol: iacTypes.StringTest("tcp"), }, }, @@ -121,6 +127,8 @@ func Test_AdaptVPC(t *testing.T) { CIDRs: []iacTypes.StringValue{ iacTypes.String("1.2.3.4/32", iacTypes.NewTestMetadata()), }, + FromPort: iacTypes.IntTest(-1), + ToPort: iacTypes.IntTest(-1), }, }, }, @@ -137,6 +145,8 @@ func Test_AdaptVPC(t *testing.T) { CIDRs: []iacTypes.StringValue{ iacTypes.String("10.0.0.0/16", iacTypes.NewTestMetadata()), }, + FromPort: iacTypes.IntTest(22), + ToPort: iacTypes.IntTest(22), }, }, IsDefaultRule: iacTypes.Bool(false, iacTypes.NewTestMetadata()), @@ -169,6 +179,8 @@ func Test_AdaptVPC(t *testing.T) { { Metadata: iacTypes.NewTestMetadata(), Description: iacTypes.String("", iacTypes.NewTestMetadata()), + FromPort: iacTypes.IntTest(-1), + ToPort: iacTypes.IntTest(-1), }, }, @@ -176,6 +188,8 @@ func Test_AdaptVPC(t *testing.T) { { Metadata: iacTypes.NewTestMetadata(), Description: iacTypes.String("", iacTypes.NewTestMetadata()), + FromPort: iacTypes.IntTest(-1), + ToPort: iacTypes.IntTest(-1), }, }, }, @@ -188,7 +202,9 @@ func Test_AdaptVPC(t *testing.T) { Metadata: iacTypes.NewTestMetadata(), Type: iacTypes.String("ingress", iacTypes.NewTestMetadata()), Action: iacTypes.String("", iacTypes.NewTestMetadata()), - Protocol: iacTypes.String("-1", iacTypes.NewTestMetadata()), + Protocol: iacTypes.String("", iacTypes.NewTestMetadata()), + FromPort: iacTypes.IntTest(-1), + ToPort: iacTypes.IntTest(-1), }, }, IsDefaultRule: iacTypes.Bool(false, iacTypes.NewTestMetadata()), @@ -252,6 +268,9 @@ resource "aws_vpc_security_group_ingress_rule" "test" { CIDRs: []iacTypes.StringValue{ iacTypes.StringTest("0.0.0.0/0"), }, + Protocol: iacTypes.StringTest("tcp"), + FromPort: iacTypes.IntTest(22), + ToPort: iacTypes.IntTest(22), }, }, EgressRules: []ec2.SecurityGroupRule{ @@ -259,6 +278,9 @@ resource "aws_vpc_security_group_ingress_rule" "test" { CIDRs: []iacTypes.StringValue{ iacTypes.StringTest("0.0.0.0/0"), }, + Protocol: iacTypes.StringTest("-1"), + FromPort: iacTypes.IntTest(-1), + ToPort: iacTypes.IntTest(-1), }, }, }, diff --git a/pkg/iac/providers/aws/ec2/vpc.go b/pkg/iac/providers/aws/ec2/vpc.go index bce7fb4de2a8..d6bff4fe7025 100644 --- a/pkg/iac/providers/aws/ec2/vpc.go +++ b/pkg/iac/providers/aws/ec2/vpc.go @@ -23,6 +23,9 @@ type SecurityGroupRule struct { Metadata iacTypes.Metadata Description iacTypes.StringValue CIDRs []iacTypes.StringValue + Protocol iacTypes.StringValue + FromPort iacTypes.IntValue + ToPort iacTypes.IntValue } type VPC struct { @@ -49,4 +52,6 @@ type NetworkACLRule struct { Action iacTypes.StringValue Protocol iacTypes.StringValue CIDRs []iacTypes.StringValue + FromPort iacTypes.IntValue + ToPort iacTypes.IntValue } diff --git a/pkg/iac/scanners/cloudformation/parser/property_conversion.go b/pkg/iac/scanners/cloudformation/parser/property_conversion.go index d286fa4dd797..35847bb35c5b 100644 --- a/pkg/iac/scanners/cloudformation/parser/property_conversion.go +++ b/pkg/iac/scanners/cloudformation/parser/property_conversion.go @@ -43,6 +43,8 @@ func (p *Property) isConvertableToBool() bool { case cftypes.Int: return p.EqualTo(1) || p.EqualTo(0) + case cftypes.Bool: + return true } return false } @@ -53,7 +55,7 @@ func (p *Property) isConvertableToInt() bool { if _, err := strconv.Atoi(p.AsString()); err == nil { return true } - case cftypes.Bool: + case cftypes.Bool, cftypes.Int: return true } return false @@ -61,6 +63,10 @@ func (p *Property) isConvertableToInt() bool { func (p *Property) ConvertTo(conversionType cftypes.CfType) *Property { + if p.Type() == conversionType { + return p + } + if !p.IsConvertableTo(conversionType) { _, _ = fmt.Fprintf(os.Stderr, "property of type %s cannot be converted to %s\n", p.Type(), conversionType) return p