Misc updates for cisco router configurations #466

dspicuzzbbn · 2017-09-22T20:46:47Z

Found on a cisco ASA

This change is

arifogel

Need changes to enable_password and variable_password. Recommend offline discussion due to complexity of implementing lexer modes.

arifogel · 2017-09-25T23:56:31Z

projects/batfish/src/main/antlr4/org/batfish/grammar/cisco/Cisco_common.g4

@@ -491,6 +491,11 @@ variable_interface_name
   NAME | NEWLINE | TAG | TRACK | VARIABLE )
 ;

+variable_password
+:
+   (~NEWLINE)+


With respect to ENCRYPTED:
Based on the usage above, the variable_password rule must specifically exclude ENCRYPTED, or it will swallow it. Therefore a potential definition here is: ~(NEWLINE | ENCRYPTED)+

Also, any definition that generally eats up non-newline tokens will necessarily swallow up the variable named seed in the change to enable_password above. That is, seed will never be assigned. In fact this is visible in the updated reference output in this PR. The only solution that will allow a multi-token password will necessitate use of lexer modes and at least some modification to variable_password. This is tricky, and we should discuss in depth if you want to fix yourself.

I have no desire to learn the deep complexity of ANTLR... so if it's easy for you to fix, by all means do so. We encountered the 'enable password' line on an ASA, so... as long as that doesn't throw a parsing error as it did previously, that's all I care about. :)

I don't think Batfish currently does anything with the password anyways, so if it swallowed the seed then that's probably "ok", though probably not the Right Thing to do.

That's fine. I'll just finish this one then.

arifogel

There are breaking changes here. Please add unit tests justifying each change here, so I know how to fix.

arifogel · 2017-09-28T01:47:58Z

projects/batfish/src/main/antlr4/org/batfish/grammar/cisco/Cisco_ignored.g4

@@ -237,6 +238,7 @@ null_block
      | TEMPLATE
      | TERMINAL
      | TIME_RANGE
+      | TIMEOUT


This is a breaking change. It needs to be more granular. In particular, it interferes with ts_timeout.

Sad.. it's true, I hadn't ran the unit tests yet. Alright, I'll fix it on Friday. Here are some of the line(s):

timeout xlate 3:00:00 timeout pat-xlate 0:00:30 ... there are a bunch of other variations also

From ASA command reference: https://www.cisco.com/c/en/us/td/docs/security/asa/asa-command-reference/T-Z/cmdref4/t1.html?dtid=osscdc0002833#pgfId-1649892

virtuald · 2017-09-28T03:00:18Z

btw, If you made your other changes (the encrypted variable stuff), you could just cherry pick the first commit from this and push that and I'll fix the second commit later.

arifogel · 2017-09-28T17:29:08Z

I made PR #481 off your first commit, and added grammar changes for password handling.
When #481 is merged, you should update this PR before asking for another review.

arifogel · 2017-09-28T19:16:45Z

test_rigs/unit-tests/configs/cisco_enable

@@ -6,5 +6,6 @@ enable secret 5 $1$aaaaaaaaaaaaaaaaaaaaaaaaaaa
 enable super-user-password aaaaaaaaaaaaaaaaaaaaa
 enable telnet authentication
 enable telnet password aaaaaaaaaaaaaaaaaaaaaaaaa
+enable password $sha512$5000$98pqLCa2lpNvN6gnQl3zyQ==$QvdeF/4EqAATT2/e64ujg== woldj4


What configuration language is this? I cannot enter this on my ASA VM. Also, it looks like it contains an invalid base64 string after the last $.

The password was from an ASA and 'looks like' that (the important part that breaks parsing are the random $ symbols and the last part) -- but in reality I just randomly changed various characters so it wouldn't match, so I'm not surprised it's not a valid base64 string.

I didn't create the ASA config, @asydney did, so if the password isn't sensitive I suspect he could share the original string if you need it.

No I just made it valid base64 by inserting a character. It's just a hash, so I don't need anything original.

arifogel · 2017-09-28T22:30:09Z

#481 has been merged. You should update this branch before your next review request.

dspicuzzbbn · 2017-09-29T20:48:17Z

Updated, should be good to go now.

arifogel

The changes to the cisco_misc file do not nearly justify the changes to the grammar files. Please add complete unit tests justifying all changes.
By the way, a recently merged PR includes major changes to the Cisco parser/lexer files. Don't merge it into your PR - I will resolve conflicts myself after this is ready to go.

virtuald · 2017-10-05T03:14:14Z

Alright. I won't have time to get to it until late next week.

dspicuzzbbn · 2017-12-12T23:30:16Z

Sorry for the delay on this, we got redirected and paused for awhile. I added more to the tests, this should be ready to be merged now.

arifogel · 2017-12-14T21:29:47Z

Hey thanks for the update! I just need a few more unit test lines from you. See inline comments.

Reviewed 6 of 6 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

projects/batfish/src/main/antlr4/org/batfish/grammar/cisco/Cisco_ignored.g4, line 919 at r1 (raw file):

            | HALF_CLOSED
            | ICMP
            | ICMP_ERROR

Missing unit test.

projects/batfish/src/main/antlr4/org/batfish/grammar/cisco/Cisco_ospf.g4, line 194 at r1 (raw file):

      )
      | LOG
      | LOG_ADJ_CHANGES

Missing unit test.

projects/batfish/src/main/antlr4/org/batfish/grammar/cisco/CiscoLexer.g4, line 10678 at r1 (raw file):

;

TCP_INSPECTION

Missing parser rule referencing this token, and missing unit test.

Comments from Reviewable

dspicuzzbbn · 2017-12-19T22:41:08Z

Updated.

arifogel · 2017-12-20T07:51:12Z

Reviewed 3 of 3 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

ratulm requested a review from arifogel September 23, 2017 21:54

arifogel requested changes Sep 26, 2017

View reviewed changes

dhalperi assigned arifogel Sep 27, 2017

arifogel requested changes Sep 28, 2017

View reviewed changes

arifogel mentioned this pull request Sep 28, 2017

Update enable password handling #481

Merged

arifogel reviewed Sep 28, 2017

View reviewed changes

dspicuzzbbn force-pushed the fix-cisco-misc branch from ca49057 to 3a56b45 Compare September 29, 2017 20:46

arifogel requested changes Oct 5, 2017

View reviewed changes

Fix misc cisco ASA directives

0a248b4

dspicuzzbbn force-pushed the fix-cisco-misc branch from 3a56b45 to 0a248b4 Compare December 12, 2017 23:27

Add additional tests

ce8101f

Missed an output

b095e20

arifogel merged commit bbc5153 into batfish:master Dec 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Misc updates for cisco router configurations #466

Misc updates for cisco router configurations #466

dspicuzzbbn commented Sep 22, 2017 •

edited by dhalperi

arifogel left a comment

arifogel Sep 25, 2017

virtuald Sep 26, 2017 •

edited

arifogel Sep 26, 2017

virtuald Sep 26, 2017

arifogel left a comment

arifogel Sep 28, 2017

virtuald Sep 28, 2017

virtuald commented Sep 28, 2017

arifogel commented Sep 28, 2017

arifogel Sep 28, 2017

virtuald Sep 28, 2017

arifogel Sep 28, 2017

arifogel commented Sep 28, 2017

dspicuzzbbn commented Sep 29, 2017

arifogel left a comment

virtuald commented Oct 5, 2017

dspicuzzbbn commented Dec 12, 2017

arifogel commented Dec 14, 2017

dspicuzzbbn commented Dec 19, 2017

arifogel commented Dec 20, 2017

Misc updates for cisco router configurations #466

Misc updates for cisco router configurations #466

Conversation

dspicuzzbbn commented Sep 22, 2017 • edited by dhalperi

arifogel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

virtuald Sep 26, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arifogel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

virtuald commented Sep 28, 2017

arifogel commented Sep 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arifogel commented Sep 28, 2017

dspicuzzbbn commented Sep 29, 2017

arifogel left a comment

Choose a reason for hiding this comment

virtuald commented Oct 5, 2017

dspicuzzbbn commented Dec 12, 2017

arifogel commented Dec 14, 2017

dspicuzzbbn commented Dec 19, 2017

arifogel commented Dec 20, 2017

dspicuzzbbn commented Sep 22, 2017 •

edited by dhalperi

virtuald Sep 26, 2017 •

edited