IOS: fix a crash in malformed isakmp profiles (batfish#5851)

Fix batfish#5849
arifogel · Jun 3, 2020 · 208b6f2 · 208b6f2
1 parent 7160cc3
commit 208b6f2
Show file tree

Hide file tree

Showing 5 changed files with 48 additions and 17 deletions.
diff --git a/projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/IkePhase1Policy.java b/projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/IkePhase1Policy.java
@@ -1,5 +1,8 @@
 package org.batfish.datamodel;
 
+import static com.google.common.base.MoreObjects.firstNonNull;
+import static org.batfish.datamodel.Interface.UNSET_LOCAL_INTERFACE;
+
 import com.fasterxml.jackson.annotation.JsonCreator;
 import com.fasterxml.jackson.annotation.JsonProperty;
 import com.google.common.collect.ImmutableList;
@@ -21,49 +24,45 @@ public class IkePhase1Policy extends ComparableStructure<String> {
   private static final String PROP_LOCAL_INTERFACE = "localInterface";
 
   private @Nonnull List<String> _ikePhase1Proposals;
-
-  private IkePhase1Key _ikePhase1Key;
-
-  private IpSpace _remoteIdentity;
-
-  private Ip _selfIdentity;
-
-  private String _localInterface;
+  private @Nullable IkePhase1Key _ikePhase1Key;
+  private @Nullable IpSpace _remoteIdentity;
+  private @Nullable Ip _selfIdentity;
+  private @Nonnull String _localInterface;
 
   @JsonCreator
   public IkePhase1Policy(@JsonProperty(PROP_NAME) String name) {
     super(name);
     _ikePhase1Proposals = ImmutableList.of();
+    _localInterface = UNSET_LOCAL_INTERFACE;
   }
 
   /** IKE phase 1 proposals to be used with this IKE phase 1 policy. */
   @JsonProperty(PROP_IKE_PHASE1_PROPOSALS)
-  @Nonnull
-  public List<String> getIkePhase1Proposals() {
+  public @Nonnull List<String> getIkePhase1Proposals() {
     return _ikePhase1Proposals;
   }
 
   /** Key to be used with this IKE phase 1 policy. */
   @JsonProperty(PROP_IKE_PHASE1_KEY)
-  public IkePhase1Key getIkePhase1Key() {
+  public @Nullable IkePhase1Key getIkePhase1Key() {
     return _ikePhase1Key;
   }
 
   /** Identity of the remote peer that can match with this IKE phase 1 policy. */
   @JsonProperty(PROP_REMOTE_IDENTITY)
-  public IpSpace getRemoteIdentity() {
+  public @Nullable IpSpace getRemoteIdentity() {
     return _remoteIdentity;
   }
 
   /** Self identity to be used with a remote peer while using this IKE phase 1 policy. */
   @JsonProperty(PROP_SELF_IDENTITY)
-  public Ip getSelfIdentity() {
+  public @Nullable Ip getSelfIdentity() {
     return _selfIdentity;
   }
 
   /** Local interface on which this IKE phase 1 policy can be used. */
   @JsonProperty(PROP_LOCAL_INTERFACE)
-  public String getLocalInterface() {
+  public @Nonnull String getLocalInterface() {
     return _localInterface;
   }
 
@@ -90,6 +89,6 @@ public void setSelfIdentity(@Nullable Ip selfIdentity) {
 
   @JsonProperty(PROP_LOCAL_INTERFACE)
   public void setLocalInterface(@Nullable String localInterface) {
-    _localInterface = localInterface;
+    _localInterface = firstNonNull(localInterface, UNSET_LOCAL_INTERFACE);
   }
 }
diff --git a/projects/batfish/src/main/java/org/batfish/representation/cisco/CiscoConversions.java b/projects/batfish/src/main/java/org/batfish/representation/cisco/CiscoConversions.java
@@ -993,7 +993,9 @@ private static String getIkePhase1Policy(
     for (Entry<String, IkePhase1Policy> e : ikePhase1Policies.entrySet()) {
       IkePhase1Policy ikePhase1Policy = e.getValue();
       String ikePhase1PolicyLocalInterface = ikePhase1Policy.getLocalInterface();
-      if (ikePhase1Policy.getRemoteIdentity().containsIp(remoteAddress, ImmutableMap.of())
+      IpSpace remoteAddressSpace =
+          firstNonNull(ikePhase1Policy.getRemoteIdentity(), EmptyIpSpace.INSTANCE);
+      if (remoteAddressSpace.containsIp(remoteAddress, ImmutableMap.of())
           && (UNSET_LOCAL_INTERFACE.equals(ikePhase1PolicyLocalInterface)
               || ikePhase1PolicyLocalInterface.equals(localInterface))) {
         return e.getKey();

diff --git a/projects/batfish/src/test/java/org/batfish/grammar/cisco/CiscoGrammarTest.java b/projects/batfish/src/test/java/org/batfish/grammar/cisco/CiscoGrammarTest.java
@@ -2002,6 +2002,13 @@ public void testIosIpRouteVrf() throws IOException {
                                 .build()))))));
   }
 
+  /** Tests that Cisco parser doesn't crash in conversion for incomplete ipsec profile. */
+  @Test
+  public void testIosIpsecGh5849() throws IOException {
+    // doesn't crash.
+    parseConfig("ios-ipsec-gh-5849");
+  }
+
   @Test
   public void testIosIsisConfigOnInterface() throws IOException {
     String hostname = "ios-isis";

diff --git a/projects/batfish/src/test/resources/org/batfish/grammar/cisco/testconfigs/ios-ipsec-gh-5849 b/projects/batfish/src/test/resources/org/batfish/grammar/cisco/testconfigs/ios-ipsec-gh-5849
@@ -0,0 +1,22 @@
+!
+hostname ios-ipsec-gh-5849
+!
+crypto keyring KEYRING
+  pre-shared-key address 2.2.2.2 key SOMEKEY
+!
+crypto isakmp profile PROFILE
+  keyring KEYRING
+  ! no match identity statement
+  local-address Ethernet1
+!
+interface Tunnel1
+ ip address 3.3.3.3 255.255.255.252
+ tunnel source Ethernet1
+ tunnel mode ipsec ipv4
+ tunnel destination 4.4.4.4
+ tunnel protection ipsec profile PROFILE
+interface Ethernet1
+  no shutdown
+  no switchport
+  ip address 1.1.1.1 255.255.255.255
+!
diff --git a/tests/parsing-tests/unit-tests-vimodel.ref b/tests/parsing-tests/unit-tests-vimodel.ref
@@ -28800,7 +28800,8 @@
             },
             "ikePhase1Proposals" : [
               "PROPOSAL"
-            ]
+            ],
+            "localInterface" : "unset_local_interface"
           }
         },
         "ikePhase1Proposals" : {