Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve validation and error handling for forgeticket module #17268

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

cgranleese-r7
Copy link
Contributor

@cgranleese-r7 cgranleese-r7 commented Nov 16, 2022

Note

If a user had their features set datastore_fallbacks false as well as saved options. The options would continue to fail to validate and would accept any value and not honour the options regex check.
This error does not occur with the features set datastore_fallbacks true, therefore I'll leave this in draft for now until I I can figure out a way to fix this.

This PR adds validation to some of the options within the forge_ticket.rb module. As well as improving error affordance when setting options. Adding examples to the API to aid in making errors more transparent for users, and works for both inline options as well as setting options via the set command, also updates some tests to reflect the changes in error outputs.

Before

[-] Msf::OptionValidateError The following options failed to validate: SPN 

After

[-] The following options failed to validate: Value 'test' is not valid for option 'SPN'. Example value: MSSqlSvc/host.domain.local:1433.

features set datastore_fallbacks true

image

features set datastore_fallbacks false

image

Verification

Needs testing with the datastore_fallbacks set to true then false

  • Below steps tested with features set datastore_fallbacks false
  • Below steps tested with features set datastore_fallbacks true

Testing steps:

  • Start msfconsole
  • Use auxiliary/admin/kerberos/forge_ticket
  • Attempt to set invalid option for DOMAIN_SID - set DOMAIN_SID test
  • Attempt to set invalid option for SPN - set SPN test
  • Verify the appropriate error message are returned
  • Verify the step above only this time using inline options run spn=test domain_sid=test

@cgranleese-r7 cgranleese-r7 force-pushed the improve-validation-and-error-handling-for-forgeticket-module branch 3 times, most recently from 3cfd7ba to ff818be Compare November 17, 2022 09:42
@cgranleese-r7 cgranleese-r7 added the feature-kerberos-authentication Adds Kerberos Authentication support to framework label Nov 21, 2022
@cgranleese-r7 cgranleese-r7 force-pushed the improve-validation-and-error-handling-for-forgeticket-module branch 4 times, most recently from d4c3138 to b2932d6 Compare December 12, 2022 10:41
@adfoster-r7
Copy link
Contributor

This will most likely land into master after the kerberos branch is merged, taking off the feature label for now 👍

@adfoster-r7 adfoster-r7 removed the feature-kerberos-authentication Adds Kerberos Authentication support to framework label Jan 20, 2023
@adfoster-r7
Copy link
Contributor

@cgranleese-r7 Looks like this needs a rebase

@adfoster-r7 adfoster-r7 changed the base branch from feature-kerberos-authentication to master January 31, 2023 17:18
@cgranleese-r7 cgranleese-r7 force-pushed the improve-validation-and-error-handling-for-forgeticket-module branch from b2932d6 to 95a33fe Compare February 1, 2023 09:50
@cgranleese-r7 cgranleese-r7 force-pushed the improve-validation-and-error-handling-for-forgeticket-module branch from 95a33fe to a661cdf Compare February 1, 2023 09:52
@cgranleese-r7 cgranleese-r7 marked this pull request as ready for review February 1, 2023 09:53
@gwillcox-r7
Copy link
Contributor

Can take a look at this shortly if no one has picked it up before then otherwise feel free to grab it.

@adfoster-r7 adfoster-r7 self-assigned this Feb 1, 2023
@dwelch-r7 dwelch-r7 assigned dwelch-r7 and unassigned adfoster-r7 Sep 25, 2023
Copy link
Contributor

@dwelch-r7 dwelch-r7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I'm not getting exactly the same output as you are but I think that's in part to some of the options no longer being required?
image

Either way I can take another look when the conflicts have been resolved

Comment on lines +47 to +48
OptString.new('DOMAIN', [ false, 'The Domain (upper case) Ex: DEMO.LOCAL' ]),
OptString.new('DOMAIN_SID', [ false, 'The Domain SID, Ex: S-1-5-21-1755879683-3641577184-3486455962'], regex: /^S-\d-\d+-(\d+-){1,14}\d+$/, examples: %w[S-1-5-21-1755879683-3641577184-3486455962 S-1-5-21-1180699209-877415012-3182924384-1004]),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
OptString.new('DOMAIN', [ false, 'The Domain (upper case) Ex: DEMO.LOCAL' ]),
OptString.new('DOMAIN_SID', [ false, 'The Domain SID, Ex: S-1-5-21-1755879683-3641577184-3486455962'], regex: /^S-\d-\d+-(\d+-){1,14}\d+$/, examples: %w[S-1-5-21-1755879683-3641577184-3486455962 S-1-5-21-1180699209-877415012-3182924384-1004]),
OptString.new('DOMAIN', [ true, 'The Domain (upper case) Ex: DEMO.LOCAL' ]),
OptString.new('DOMAIN_SID', [ true, 'The Domain SID, Ex: S-1-5-21-1755879683-3641577184-3486455962'], regex: /^S-\d-\d+-(\d+-){1,14}\d+$/, examples: %w[S-1-5-21-1755879683-3641577184-3486455962 S-1-5-21-1180699209-877415012-3182924384-1004]),

Not sure how/why these got changed, pretty sure they should be required

OptString.new('DOMAIN', [ false, 'The Domain (upper case) Ex: DEMO.LOCAL' ]),
OptString.new('DOMAIN_SID', [ false, 'The Domain SID, Ex: S-1-5-21-1755879683-3641577184-3486455962'], regex: /^S-\d-\d+-(\d+-){1,14}\d+$/, examples: %w[S-1-5-21-1755879683-3641577184-3486455962 S-1-5-21-1180699209-877415012-3182924384-1004]),
OptString.new('SPN', [ false, 'The Service Principal Name (Only used for silver ticket) Ex: MSSqlSvc/host.domain.local:1434'], conditions: %w[ACTION == FORGE_SILVER], regex: %r{.*/.*}, examples: %w[MSSqlSvc/host.domain.local:1433 MSSqlSvc/host.domain.local:1434]),
OptInt.new('DURATION', [ false, 'Duration of the ticket in days', 3650]),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
OptInt.new('DURATION', [ false, 'Duration of the ticket in days', 3650]),
OptInt.new('DURATION', [ true, 'Duration of the ticket in days', 3650]),

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants