Handle binary values in YAML files #223

OiCMudkips · 2019-08-14T23:31:46Z

(WIP as of writing)

This is step (1) in supporting binary in both YAML and non-YAML files.

This makes it so that instead of immediately converting the base64-encoded binary into a binary value in python, we just interpret the binary as a normal string, but annotate it as such with the is_binary flag.

This is needed so that plugins can scan a different value from the value hashed into baselines.

Alternatives:

Convert the binary to the desired form discussed here in YamlFileParser and put the desired form into __value__
- Why not?: We need the original base64-format in the plugin to hash into baselines. It's possible to retrieve it, but why do extra work?
Instead of __value__ and __is_binary__, have fields __scan_value__ and __plaintext_value__
- Why not?: I think we're going to end up with a new method in the high-entropy plugin analyze_binary_content (in addition to analyze_string_content) which knows how to do the conversion to the "desired format", but that depends on us knowing that the string is a binary as opposed to some other special sauce, so I think the is_binary information is important.
- Also my gut feeling says what I've written will be fine; I don't think there's any other data types we'll need to consider.

Closes #202

The next steps would be to make the high-entropy plugin actually use the is_binary information to do the conversion and output results correctly.

KevinHock

looks good so far.

I like your plan, maybe it would be easier to do regular binary strings first, and then do yaml binary strings, but it's up to you.

KevinHock · 2019-08-14T23:41:41Z

detect_secrets/plugins/common/yaml_file_parser.py

@@ -75,7 +75,7 @@ def _tag_dict_values(self, map_node):
        """
        new_values = []
        for key, value in map_node.value:
-            if not value.tag.endswith(':str'):
+            if not value.tag.endswith(':str') and not value.tag.endswith(':binary'):


super nit: feel free to ignore
i feel like

if not ( value.tag.endswith(':str') or value.tag.endswith(':binary') ):

is a little prettier ⚡️ 💟

KevinHock · 2019-08-14T23:48:44Z

detect_secrets/plugins/common/yaml_file_parser.py

@@ -92,6 +92,11 @@ def _tag_dict_values(self, map_node):
                        str(value.__line__),
                        'tag:yaml.org,2002:int',
                    ),
+                    self._create_key_value_pair_for_mapping_node_value(
+                        '__is_binary__',


I realize the other calls of this don't do this, but maybe we can use keyword args to make things clearer, it helps with the other function calls in this file.

This is step (1) in supporting binary in both YAML and non- YAML files. This makes it so that instead of immediately converting the base64-encoded binary into a binary value in python, we just interpret the binary as a normal string, but annotate it as such with the `is_binary` flag. This is needed so that plugins can scan a different value from the value hashed into baselines.

This implements support for high-entropy secrets in binary values in yaml files. We encode the binary value into a hex- or base64-encoded string (based on the plugin), and run the normal entropy check. If the string is deemed to be high-entropy, we re encode the string into a yaml binary (using `yaml.dump`) and strip the `!!binary`. This yaml binary is considered the secret, and is put into the baseline as normal. I had to update a test function so that it uses a custom hex high-entropy detector, since `HighEntropyStringsPlugin` is now an abstract class.

To test this you would need to import an unused class into the module so that it's in `globals()`, and have the test know what class that is. Seems messy to me, and not worth what it would be testing.

OiCMudkips · 2019-08-29T01:18:52Z

This is done. The general strategy is:

Parse YAML binaries, so that they eventually become a bytes object b'something'. In Python 2, this will be the same as 'something' though, so we have an __is_binary__ field to tell us whether the value is actually a binary in the plugin.
For any value with the is_binary field set to True, decode_binary it to turn it into a str and run it through the plugin's analyze_string function.
If the value is a secret, create a PotentialSecret with the str. This str isn't what's actually in the file, so apply the reverse of decode_binary (which is encode_to_binary), yaml.dump it, then strip the !!binary. This will leave us with the secret encoded in the binary yaml format. Set this as the secret in the PotentialSecret object.

The complicated-ness of (3) is needed so that we can actually find the secret in the file afterwards using the existing audit functionality.

KevinHock · 2019-08-30T17:50:49Z

re: ‘HighEntropyStringsPlugin is now an abstract class’ in commit message, for the explanation of the new class in the test, I’m on my phone but I don’t see that it’s abstract.

KevinHock · 2019-08-30T17:53:37Z

Never mind me lol, I see the ABCMeta metaclass.

I guess it was an abstract class before this PR and I read the message as this making it abstract.

KevinHock

Lgtm

⛴

Note to self to check when I’m on desktop: I’m not sure how we were instantiating an abstract class in a test before, I should check I understand that.

OiCMudkips · 2019-09-04T15:42:43Z

KevinHock++ I think the test was able to instantiate an abstract class because we didn’t actually mark any methods as abstract, but once I added some abstract methods, the test started failing so I had to fix it.

OiCMudkips requested a review from KevinHock August 14, 2019 23:31

OiCMudkips self-assigned this Aug 14, 2019

KevinHock reviewed Aug 14, 2019

View reviewed changes

OiCMudkips force-pushed the yaml_binary branch from 65e126c to 4107194 Compare August 15, 2019 18:46

Victor Zhou added 3 commits August 21, 2019 14:24

Don't enforce coverage for untestable line

869033d

To test this you would need to import an unused class into the module so that it's in `globals()`, and have the test know what class that is. Seems messy to me, and not worth what it would be testing.

OiCMudkips force-pushed the yaml_binary branch from 4107194 to 869033d Compare August 29, 2019 01:12

OiCMudkips changed the title ~~WIP: Handle binary values in YAML files~~ Handle binary values in YAML files Aug 29, 2019

KevinHock approved these changes Aug 30, 2019

View reviewed changes

Change one variable name

34a730d

OiCMudkips merged commit f996791 into Yelp:master Sep 4, 2019

KevinHock mentioned this pull request Oct 21, 2019

Change default entropy of binary Yaml secrets #257

Open

KevinHock mentioned this pull request Mar 30, 2020

💥 Drop Python 2 support #292

Merged

killuazhu pushed a commit to IBM/detect-secrets that referenced this pull request May 28, 2020

feat: add prepare upstream script (Yelp#223)

953fb9e

killuazhu pushed a commit to IBM/detect-secrets that referenced this pull request Jul 9, 2020

feat: add prepare upstream script (Yelp#223)

202207b

killuazhu pushed a commit to IBM/detect-secrets that referenced this pull request Sep 17, 2020

feat: add prepare upstream script (Yelp#223)

06ce085

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle binary values in YAML files #223

Handle binary values in YAML files #223

OiCMudkips commented Aug 14, 2019 •

edited

Loading

KevinHock left a comment

KevinHock Aug 14, 2019

KevinHock Aug 14, 2019

OiCMudkips commented Aug 29, 2019

KevinHock commented Aug 30, 2019

KevinHock commented Aug 30, 2019 •

edited

Loading

KevinHock left a comment

OiCMudkips commented Sep 4, 2019 •

edited

Loading

Handle binary values in YAML files #223

Handle binary values in YAML files #223

Conversation

OiCMudkips commented Aug 14, 2019 • edited Loading

KevinHock left a comment

Choose a reason for hiding this comment

KevinHock Aug 14, 2019

Choose a reason for hiding this comment

KevinHock Aug 14, 2019

Choose a reason for hiding this comment

OiCMudkips commented Aug 29, 2019

KevinHock commented Aug 30, 2019

KevinHock commented Aug 30, 2019 • edited Loading

KevinHock left a comment

Choose a reason for hiding this comment

OiCMudkips commented Sep 4, 2019 • edited Loading

OiCMudkips commented Aug 14, 2019 •

edited

Loading

KevinHock commented Aug 30, 2019 •

edited

Loading

OiCMudkips commented Sep 4, 2019 •

edited

Loading