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

Added type check disable to ActivitySurrogateSelector generators #41

Merged
merged 2 commits into from Jun 7, 2019

Conversation

Projects
None yet
4 participants
@monoxgas
Copy link

commented Jun 4, 2019

Reference: https://silentbreaksecurity.com/re-animating-activitysurrogateselector/

The current changes are very minimal in terms of "integration". I had some additional thoughts that might be good to discuss before merging:

  • Currently both surrogate payloads will always be paired with the disable, but this would only be required if you were targeting newer framework versions. I don't suspect it would cause issues in earlier versions, but I hate making that assumption.
  • I had considered some sort of flag argument to control code flow, but didn't want to alter the core Generate(input, fmt, test) format. If you are okay with this, maybe a flag for either version support or "mitigation bypasses". I'm happy to go any direction.
  • Because we have to modify the AppSettings in ysoserial itself before serializing, testing becomes inaccurate (we don't know if the bypass is working). During my testing I just reset the flag after the fmt.Serialize call, but this would be repetitive due to the way the internal Serialize() function is currently layed out.
@irsdl

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

I think perhaps adding a note would be suffice otherwise this may break some of the plugins that rely on this gadget too? Or maybe we should disable them too?

@JarLob

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

  • Because we have to modify the AppSettings in ysoserial itself before serializing, testing becomes inaccurate (we don't know if the bypass is working). During my testing I just reset the flag after the fmt.Serialize call, but this would be repetitive due to the way the internal Serialize() function is currently layed out.

IMHO, test flag was always just for quick PoC, because anyway the payload is deserialized on another computer with potentially different .NET framework installed and can't be a real test.

@pwntester

This comment has been minimized.

Copy link
Owner

commented Jun 5, 2019

Thanks for the PR!

I think it would make sense to create two new separate generators, something such as WrappedActivitySurrogateSelectorGenerator.cs and WrappedActivitySurrogateSelectorGenerator.cs, thoughts?

@monoxgas

This comment has been minimized.

Copy link
Author

commented Jun 5, 2019

IMHO, test flag was always just for quick PoC

I'm in agreement there, might be worth adding a note at the very least for user edification.

I think it would make sense to create two new separate generators, something such as WrappedActivitySurrogateSelectorGenerator.cs

Creating two more generators might be the cleanest way to go for now. Minimal core code changes and disambiguation between functionality. The FromFileGenerator variation could be seen as following the same strategy if we are aiming for consistency.

Really the worst con is excessive/repetitive information while printing the gadgets. The help text gets quite a bit longer to show very similar information. Also worth noting that pairing/augmenting existing gadgets might become a more common occurrence. For that it might be best to consider some kind of "inheritance respect" for the GenericGenerator classes. Maybe the help text could reflect that one gadget is just an alternative to another gadget.

An even larger consideration might be to expand gadgets out like sub-commands, and allow them to have contextually different arguments and help text.

I'd be happy to help with any of this.

@monoxgas

This comment has been minimized.

Copy link
Author

commented Jun 5, 2019

After some additional testing, it appears my initial assumption was incorrect regarding the ability to stack the objects in a list.

I erroneously assumed that I could flip the AppSetting disableTypeCheck back to false before testing deserialization, but the global flag is only read once. After the settings are "initialized", they are no longer referenced and the internal boolean holds the value. It appeared to work only because I was testing in the same app instance.

To actually reset the setting, you have to add reflection:

ConfigurationManager.AppSettings.Set("microsoft:WorkflowComponentModel:DisableActivitySurrogateSelectorTypeCheck", "false");

Type appSettings = Type.GetType("System.Workflow.ComponentModel.AppSettings, System.Workflow.ComponentModel, Version=4.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35");
if (appSettings != null)
{
    var disableTypeCheckFlag = appSettings.GetField("disableActivitySurrogateSelectorTypeCheck", BindingFlags.NonPublic | BindingFlags.Static);
    disableTypeCheckFlag.SetValue(null, false);

    var initializedFlag = appSettings.GetField("settingsInitialized", BindingFlags.NonPublic | BindingFlags.Static);
    initializedFlag.SetValue(null, false);
}

The disable payload still works correctly, but only when triggered first via fmt.Deserialize, following by a separate call for the real payload. My assumption is that once the disable payload is finished, the process stops due to the exception raised. Therefore the formatter never gets to the second object in the list.

I'll keep working on this to try and get a single "wrapped" payload. If anyone has any ideas, let me know.

@pwntester

This comment has been minimized.

Copy link
Owner

commented Jun 5, 2019

For that it might be best to consider some kind of "inheritance respect" for the GenericGenerator classes.

That sounds good, I dont see any problems doing that.

An even larger consideration might be to expand gadgets out like sub-commands, and allow them to have contextually different arguments and help text.

This sounds similar to plugins, but I think in this case it makes more sense to use raw generators, even if two more are added to the help text.

I'll keep working on this to try and get a single "wrapped" payload. If anyone has any ideas, let me know.

Im not sure I followed correctly, so I may be saying something stupid here, but what about adding the resting flag reflective code as part of the ActivitySurrogate payload class?

@monoxgas

This comment has been minimized.

Copy link
Author

commented Jun 5, 2019

Im not sure I followed correctly, so I may be saying something stupid here, but what about adding the resting flag reflective code as part of the ActivitySurrogate payload class?

In essence, I was hoping to avoid requiring that multiple payloads be used sequentially (separate calls to fmt.Deserialize), but it appears that's the case. My theory of using a List<Object> = {disable, payload} fails due to the exception being raised when the first object is deserialized, but before the second is touched.

I might be missing something here myself, but provided there is no workaround, it might be best to create a separate generator called ActivitySurrogateDisableTypeCheck which you would send to a target initially before using the traditional ActivitySurrogateSelector.

@pwntester

This comment has been minimized.

Copy link
Owner

commented Jun 6, 2019

Ok, then it would make more sense to create the ActivitySurrogateDisableTypeCheck generator unless you can find a new gadget to handle the exception

Nick Landers
Moved DisableTypeCheck to a separate gadget.
Expanded disable payload to support more situations
@monoxgas

This comment has been minimized.

Copy link
Author

commented Jun 6, 2019

Alright, changes are complete, this should be good for the time being. I also expanded the gadget to use reflection to support situations where the setting has already been loaded into the WorkFlow..AppSettings boolean.

@pwntester pwntester merged commit b3c3400 into pwntester:master Jun 7, 2019

1 check passed

pwntester.ysoserial.net Build #20190606.1 succeeded
Details
@pwntester

This comment has been minimized.

Copy link
Owner

commented Jun 7, 2019

Awesome, thanks! Can you also edit the README.md?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.