"Enabled" state not persisted through YAML files (works with Protobuf)

[ngregoire]

Piper behaves differently when importing YAML and Protobuf files. YAML won't persist the "enabled" state

How to reproduce:

• Add a new action (here a HTTP listener) to Piper and enable it
• Export the custom config to YAML
• Export the custom config to Protobuf
• Restore the default config
• When the YAML file is loaded, the action will be disabled (looks like a bug)
• When the Protobuf file is loaded, the action will be enabled (as expected)

Workaround:

• Use the Protobuf format

[dnet]

Thanks, you found a weird combination of intended and unintended effects:

• The protobuf models (which are also used for serializing the settings for automatic save/load) contain the enabled boolean flag so that users can enable/disable plugins.
• The YAML import/export ignores this in both directions (as you correctly recognized) since YAML was designed for easy exchange of settings between users/machines/etc. and I considered enabled as a local setting of the user, not something inherently tied to the setting itself.
• Since YAML has no concept of enabled, it has to be supplied from somewhere. It's left false (the default of boolean values) upon import to minimize surprising behavior, such as breaking an ongoing scan by inserting a HTTP listener that mangles requests and/or responses in an unexpected/unintended way.
• A special exception is made for the default config (see loadDefaultConfig), where it obviously makes sense to load everything enabled since we know it probably won't break anything as it's a known state hardcoded into the JAR file.

The end result is what you experienced, and I agree that it's not the best outcome. As I mentioned above, I intended YAML as the (more) user-readable, stand-alone, shareable version, while ProtoBuf is somewhere between a temptation (it's about 10 lines of code since the rest is already there for settings persistence, so why not) and the notion of it being easier to parse reliably in a standardized way as opposed to YAML which has lots of issues between different implementations.

Knowing all this, what would you consider a better solution?

[ngregoire]

I prefer to use YAML (easier to read / edit / compare) and I would like to see the Prototobuf and YAML versions working in a same way. That implies adding "enabled" to the YAML format.

Regarding YAML parsing differences: you made me curious given that, usually, parsing mismatches lead to interesting bugs

[dnet]

I prefer to use YAML (easier to read / edit / compare) and I would like to see the Prototobuf and YAML versions working in a same way. That implies adding "enabled" to the YAML format.

I agree, YAML is the way for Piper to interact with humans, that's also why I use it for the default config format. What are your thoughts about ignoring enabled in the Protobuf import/export? That would also result in both formats working the same way.

Regarding YAML parsing differences: you made me curious given that, usually, parsing mismatches lead to interesting bugs

Exactly, this is why YAML is a bad format, as everyone can confirm who tried listing countries using 2-character ISO codes and someone picked Norway. ;) So right now, YAML works for Piper in a way that the same library is used to read and write. But I cannot guarantee that any other YAML parser could

1. parse a Piper YAML correctly and
2. produce YAML that can be understood by Piper.

It's a shitty format, but I couldn't find anything better with the same level of human readability and descriptive force such as arbitrary nesting (needed for filters), binary content (also needed for filters), etc.

[ngregoire]

Ignoring enabled in the Protobuf implementation: why not, so that both parsers work in a same way. But we would loose the possibility to load enabled-by-default configs (that's not a problem for me)

YAML: thanks for opening my eyes on how bad this format is ;-) Despite the possible incompatibilities, I'd keep it around, as text-based config is much easier to deal with

[dnet]

Ignoring enabled in the Protobuf implementation: why not, so that both parsers work in a same way. But we would loose the possibility to load enabled-by-default configs (that's not a problem for me)

Thanks for the confirmation, I'll do it that way then.

YAML: thanks for opening my eyes on how bad this format is ;-)

Happy to help you become one of today's lucky 10,000 \o/

Despite the possible incompatibilities, I'd keep it around, as text-based config is much easier to deal with

I agree, that was my original intention. So YAML has always been the main format and ProtoBuf is just there because it would've been a sin not to expose the already existing option to users.

[ngregoire]

All good, many thanks!

[v-p-b]

Reopening, as this issue came up while developing #29.

1) This statement turned out to be too optimistic:

A special exception is made for the default config (see loadDefaultConfig), where it obviously makes sense to load everything enabled since we know it probably won't break anything as it's a known state hardcoded into the JAR file.

During testing, my code fell back to loadDefaultConfig(), that has some MessageViewers defined. Some binaries required by MessageViewers are not present on the system. I also have some Stepper sequences defined, that for some reason try to execute all MessageViewers on the defined steps, which results in tons of exceptions because of invoking non-existent binaries on the system.

While this can be a bug of Stepper, I think, that assuming every dependency of every Piper script in the default config will be present on every system is unrealistic. We also don't know the edge cases where different Piper scripts with missing dependencies may be triggered. On the other hand when a user explicitly loads some scripts (or enables some defaults) I think it's fair to assume that she wants to use them, and is ready to deal with system dependencies.

2) Not being able to load scripts in an enabled state also hinders automation (think Burp Enterprise) significantly.

[dnet]

Maybe I didn't make myself clear enough: by won't break anything I meant not doing anything unexpected for the user, this covers

• modifying requests silently
• modifying responses silently
• executing malicious code

The first two are typical examples for HTTP listeners which the default config lacks while the last one covers the commands themselves which are considered trusted by definition, since if your threat model includes Piper JAR being mailicous, it's much easier to plant a backdoor in the Kotlin code itself instead of the much shorter and more readable YAML default config.

I guess your interpretation of braking thinks (results in tons of exceptions) is fair, yet it's not a big deal in my view: exceptions are part of life, and shouldn't break anything. Yet there's already code in place to handle this, just unused:

https://github.com/silentsignal/burp-piper/blob/634bd29ac125ea61ee5d47ff4cc83641d880cdb1/src/main/kotlin/burp/Extensions.kt#L108-L116

So if the code below would change from true to null (and Piper.Config.updateEnabled would add a ? to its Boolean argument type to allow null values), this would affect loading the default config by enabling only the things that have their dependencies installed.

https://github.com/silentsignal/burp-piper/blob/634bd29ac125ea61ee5d47ff4cc83641d880cdb1/src/main/kotlin/burp/BurpExtender.kt#L766-L767

Would that solve your issue?

Not being able to load scripts in an enabled state also hinders automation (think Burp Enterprise) significantly.

That would be solved by #29 I guess.

[v-p-b]

exceptions are part of life, and shouldn't break anything

I got one of my homeworks rejected you know where because of this, so maybe it's just my PTSD triggered :D

Would that solve your issue?

Yes, I think this is a good solution to the dependency problem.

That would be solved by https://github.com/silentsignal/burp-piper/pull/29 I guess.

True, but with this we enable scripts on one load path but not on another, which I think is inconsistent and goes against your stated concerns. Frankly, I don't see how support for automation and protecting the "Piper script supply chain" can be solved together.

[dnet]

we enable scripts on one load path but not on another, which I think is inconsistent and goes against your stated concerns

Inconsistent? Maybe. Against my concerns? I don't think so. Think Mark-of-the-Web:

• default configuration is trusted-by-design (see my threat model above), thus can be enabled (MotW: parts of the Windows OS are trusted)
• environment variable is set explicitly by user, thus can be enabled (MotW: if user unchecks the box, they know what they are doing)
• file inputs on the GUI can come from anywhere, thus are disabled by default (MotW: you downloaded some crap from the web, thus it's blocked by default)

So I think it's quite the opposite of what you stated: the current behavior supports automation yet prevents users from shooting themselves in the foot by loading either malicious or well-intended but flawed configurations.

Where do you see our opinions diverge?

[v-p-b]

I simply don't see that much of a difference between the GUI and the env var paths in terms of the trustworthiness of the input, or user consent. In both cases the user has to perform explicit actions to make the scripts load, and have the chance to review the scripts, but we can't ensure that she did.

If you argue that clicking buttons is a "weaker" consent than setting environment variables, I can accept that, but I think it is important to clarify (and maybe document) this, esp. since there are security concerns.

[dnet]

clicking buttons is a "weaker" consent than setting environment variables

Thanks, that's exactly my point. Although the use-cases of GUI and environment variables overlap somewhat, I feel that

• GUI users include both newbies and casual, everyday usage (git calls this facade), while
• the environment variable approach covers deliberate tinkering and low-level usage where determined hackers know what they're doing (git calls this plumbing)

And you're right, this should be documented as soon as it's ready for a release.