Consolidate Kerberos Ticket Storage #17377
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
zeroSteiner
commented
Dec 13, 2022
zeroSteiner
commented
Dec 13, 2022
zeroSteiner
changed the title
adfoster-r7
reviewed
Dec 14, 2022
lib/msf/core/exploit/remote/kerberos/service_authenticator/base.rb
Outdated
Show resolved
Hide resolved
zeroSteiner
changed the title
adfoster-r7
reviewed
Dec 15, 2022
lib/msf/core/exploit/remote/kerberos/service_authenticator/base.rb
Outdated
Show resolved
Hide resolved
adfoster-r7
reviewed
Dec 15, 2022
|
Placeholder comment to double check ldap_query, as I had to add in a dependency within a previous PR: https://github.com/rapid7/metasploit-framework/pull/17236/files#r1020143352 |
adfoster-r7
reviewed
Dec 15, 2022
|
Not a blocker: Do we want to scope creep this PR and add support for invalidating tickets? |
adfoster-r7
reviewed
Dec 15, 2022
lib/msf/core/exploit/remote/kerberos/ticket/storage/stored_ticket.rb
Outdated
Show resolved
Hide resolved
zeroSteiner
force-pushed
the
feat/lib/kerberos/loot-info
branch
2 times, most recently
from
94f5513 to
fe15bfb
Compare
Testing Output |
zeroSteiner
force-pushed
the
feat/lib/kerberos/loot-info
branch
from
fe15bfb to
0b10245
Compare
adfoster-r7
reviewed
Dec 15, 2022
| @@ -130,8 +125,10 @@ def initialize( | |||
| @use_gss_checksum = use_gss_checksum | |||
| @mechanism = mechanism | |||
| @send_delegated_creds = send_delegated_creds | |||
| @use_cached_credentials = use_cached_credentials | |||
| @store_credential_cache = store_credential_cache | |||
| @ticket_storage = ticket_storage || Msf::Exploit::Remote::Kerberos::Ticket::Storage::ReadWrite.new( | |||
There was a problem hiding this comment.
What are your thoughts on defaulting to Write behavior by default? 👀
I'm thinking in terms of element of least surprise for existing workflows with tools like impacket, users might not expect the ticket re-use behavior occurring out of the box
There was a problem hiding this comment.
I don't have a strong opinion about the default storage mode right here. If you're instantiating the authenticator yourself, as a module developer to do something specific, it might make sense to just be WriteOnly.
Changing it here wouldn't change the default mode for modules though.
adfoster-r7
reviewed
Dec 15, 2022
zeroSteiner
force-pushed
the
feat/lib/kerberos/loot-info
branch
from
0b10245 to
81d4b25
Compare
The goal is to provide an abstraction for how Kerberos tickets are persisted to disk.
zeroSteiner
force-pushed
the
feat/lib/kerberos/loot-info
branch
from
81d4b25 to
60a76da
Compare
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Since we're keeping the loot-based storage for the foreseeable future, we should consolidate how tickets are stored in loot. This creates a series of new
TicketStorage::Baseclasses that expose a common interface. The classes include:TicketStorage::None-- Storage is not used at all, nothing is read from it and nothing is saved.TicketStorage::ReadOnly-- Previously stored tickets are used, but new tickets are not saved and no tickets can be deleted.TicketStorage::WriteOnly-- Previously stored tickets are not used, but new tickets are saved and tickets can be deleted.TicketStorage::ReadWrite-- Previously stored tickets are used, new tickets are saved and tickets can be deleted.These classes replace the
use_cached_credentialsandstore_credential_cacheoptions as used byServiceAuthenticator::Base. The original uses of these options have been replaced. More modules have also been updated with the newKrbCacheModeoption allowing the user to switch between the 4 available modes (none, read-only, write-only and read-write).The storage classes also return
TicketStorage::StoredTicketinstead ofMdm::Lootobjects. The intention is to abstract away how they're stored so the Mdm object itself should not be leaked. In the future, the stored tickets should expose the same API.Verification
List the steps needed to make sure this thing works
kerberos/forge_ticketare stored in a way that they can be fetched from the cachekerberos/pkinit_loginare stored in a way that they can be fetched from the cacheThere should be no different in functionality.