On Tuesday, March 3, 2020 11:41:26 AM EST Salvatore Bonaccorso wrote: > Hi Scott, > > On Tue, Mar 03, 2020 at 09:19:06AM -0500, Scott Kitterman wrote: > > On Tuesday, March 3, 2020 2:29:51 AM EST Salvatore Bonaccorso wrote: > > > Source: pyyaml > > > Version: 5.3-1 > > > Severity: important > > > Tags: security upstream > > > Forwarded: https://github.com/yaml/pyyaml/pull/386 > > > > > > Hi, > > > > > > The following vulnerability was published for pyyaml. > > > > > > CVE-2020-1747[0]: > > > |arbitrary command execution through python/object/new when FullLoader > > > |is used > > > > > > If you fix the vulnerability please also make sure to include the > > > CVE (Common Vulnerabilities & Exposures) id in your changelog entry. > > > > > > For further information see: > > > > > > [0] https://security-tracker.debian.org/tracker/CVE-2020-1747 > > > > > > https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-1747 > > > > > > [1] https://github.com/yaml/pyyaml/pull/386 > > > > > > Please adjust the affected versions in the BTS as needed. > > > > Thank you for the report. > > > > I've reviewed the current security-tracker entry and I believe it's > > incorrect. While it's true that the unsafe loader was the default loader > > in releases prior to 5.0, the safe loader has always been part of pyyaml > > and recommended for cases where untrusted input needed to be parsed. > > > > Although I haven't actually tried it yet, I reviewed the relevant code > > from > > the earlier releases and I believe the same fix would work. Unfortunately > > I don't have a test case to verify that. > > > > Also, the upstream pull request to fix this is incomplete. It only > > provides a fix for python3-yaml, not python-yaml. > > > > I've updated the affected versions in the BTS and will take a shot at > > updating the security tracker shortly. > > I'm actually unsure how to handle that CVE. While I believe to > undrstand that the unsafe loader are present, my understanding of the > CVE-2020-1747 is directly associated with the FullLoader use and > looking at the Red Hat bug where the CVE originates, > https://bugzilla.redhat.com/show_bug.cgi?id=1807367 it looks to me > that the CVE's scope is only to cover FullLoader. > > Again I'm not really sure. But if that interpretation would be > correct, and given upstream recommended for for cases where untrusted > input needed to be parsed to use the safe loaders, then not-affected > would theoretically be correct, but maybe with changed/expanded > explanation on that FullLoader is not present until 5.1. > > I'm not going to touch the CVE entry in the security-tracker now, and > looking for feedback from you all (you, Emilio, Moritz said to look as > well a the CVE later).
I have investigated this issue further for buster. With the test now provided in the upstream pull request added to the buster pyyaml package, the new test fails (there are other failures, but those are known and can be ignored). The patch applies with only minimal adjustment. Once the patch is applied, the new test then passes. What I can't determine is a clean way to control if the new checks are enabled. They are enabled by default in the upstream patch (which is appropriate for FullLoader, which is supposed to be safe. The Loader class in the buster pyyaml is, however not. I don't see a way around the default. We can either make the default safe and break backward compatibility or default unsafe and no one will get the benefit of the improved behavior without changes to the calling code (what are the odds that would happen). This behavior is essentially what CVE-2017-18342 describes, which is open against stretch and buster, so the issue is documented. I reluctantly conclude that it's best left alone for stretch/buster and because of CVE-2017-18342, the problem is reported, so we don't have any additional disclosures we need to make. I've provided the debdiff for buster in case the Security Team feels differently and for anyone interested in rebuilding their pyyaml locally. Scott K
diff -Nru pyyaml-3.13/debian/changelog pyyaml-3.13/debian/changelog --- pyyaml-3.13/debian/changelog 2019-01-10 11:09:51.000000000 +0000 +++ pyyaml-3.13/debian/changelog 2020-03-07 05:43:00.000000000 +0000 @@ -1,3 +1,10 @@ +pyyaml (3.13-2+deb10u1~) buster; urgency=high + + * Disable arbitrary code execution (CVE-2017-18342) + - See Debian #953013 for details + + -- Scott Kitterman <sc...@kitterman.com> Sat, 07 Mar 2020 01:43:00 -0400 + pyyaml (3.13-2) unstable; urgency=medium * Team upload diff -Nru pyyaml-3.13/debian/patches/safety.patch pyyaml-3.13/debian/patches/safety.patch --- pyyaml-3.13/debian/patches/safety.patch 1970-01-01 00:00:00.000000000 +0000 +++ pyyaml-3.13/debian/patches/safety.patch 2020-03-07 05:43:00.000000000 +0000 @@ -0,0 +1,127 @@ +Description: Disable arbitrary code execution + * Disable arbitrary code execution (CVE-2017-18342) + - See Debian #953013 for details +Author: Scott Kitterman <sc...@kitterman.com> +Origin: vendor +Bug-Debian: https://bugs.debian.org/953013 +Forwarded: not-needed +Last-Update: 2020-03-07 + +--- pyyaml-3.13.orig/lib/yaml/constructor.py ++++ pyyaml-3.13/lib/yaml/constructor.py +@@ -27,6 +27,14 @@ class BaseConstructor(object): + # If there are more documents available? + return self.check_node() + ++ def check_state_key(self, key): ++ """Block special attributes/methods from being set in a newly created ++ object, to prevent user-controlled methods from being called during ++ deserialization""" ++ if self.state_blacklist_regexp.match(key): ++ raise ConstructorError(None, None, ++ "blacklisted key '%s' in instance state found" % (key,), None) ++ + def get_data(self): + # Construct and return the next document. + if self.check_node(): +@@ -465,6 +473,12 @@ SafeConstructor.add_constructor(None, + SafeConstructor.construct_undefined) + + class Constructor(SafeConstructor): ++ # 'extend' is blacklisted because it is used by ++ # construct_python_object_apply to add `listitems` to a newly generate ++ # python instance ++ STATE_BLACKLIST_KEYS = ['^extend$', '^__.*__$'] ++ ++ state_blacklist_regexp = re.compile('(' + '|'.join(STATE_BLACKLIST_KEYS) + ')') + + def construct_python_str(self, node): + return self.construct_scalar(node).encode('utf-8') +@@ -548,7 +562,7 @@ class Constructor(SafeConstructor): + else: + return cls(*args, **kwds) + +- def set_python_instance_state(self, instance, state): ++ def set_python_instance_state(self, instance, state, unsafe=False): + if hasattr(instance, '__setstate__'): + instance.__setstate__(state) + else: +@@ -556,10 +570,15 @@ class Constructor(SafeConstructor): + if isinstance(state, tuple) and len(state) == 2: + state, slotstate = state + if hasattr(instance, '__dict__'): ++ if not unsafe and state: ++ for key in state.keys(): ++ self.check_state_key(key) + instance.__dict__.update(state) + elif state: + slotstate.update(state) + for key, value in slotstate.items(): ++ if not unsafe: ++ self.check_state_key(key) + setattr(object, key, value) + + def construct_python_object(self, suffix, node): +--- pyyaml-3.13.orig/lib3/yaml/constructor.py ++++ pyyaml-3.13/lib3/yaml/constructor.py +@@ -25,6 +25,14 @@ class BaseConstructor: + # If there are more documents available? + return self.check_node() + ++ def check_state_key(self, key): ++ """Block special attributes/methods from being set in a newly created ++ object, to prevent user-controlled methods from being called during ++ deserialization""" ++ if self.state_blacklist_regexp.match(key): ++ raise ConstructorError(None, None, ++ "blacklisted key '%s' in instance state found" % (key,), None) ++ + def get_data(self): + # Construct and return the next document. + if self.check_node(): +@@ -465,6 +473,12 @@ SafeConstructor.add_constructor(None, + SafeConstructor.construct_undefined) + + class Constructor(SafeConstructor): ++ # 'extend' is blacklisted because it is used by ++ # construct_python_object_apply to add `listitems` to a newly generate ++ # python instance ++ STATE_BLACKLIST_KEYS = ['^extend$', '^__.*__$'] ++ ++ state_blacklist_regexp = re.compile('(' + '|'.join(STATE_BLACKLIST_KEYS) + ')') + + def construct_python_str(self, node): + return self.construct_scalar(node) +@@ -555,7 +569,7 @@ class Constructor(SafeConstructor): + else: + return cls(*args, **kwds) + +- def set_python_instance_state(self, instance, state): ++ def set_python_instance_state(self, instance, state, unsafe=False): + if hasattr(instance, '__setstate__'): + instance.__setstate__(state) + else: +@@ -563,10 +577,15 @@ class Constructor(SafeConstructor): + if isinstance(state, tuple) and len(state) == 2: + state, slotstate = state + if hasattr(instance, '__dict__'): ++ if not unsafe and state: ++ for key in state.keys(): ++ self.check_state_key(key) + instance.__dict__.update(state) + elif state: + slotstate.update(state) + for key, value in slotstate.items(): ++ if not unsafe: ++ self.check_state_key(key) + setattr(object, key, value) + + def construct_python_object(self, suffix, node): +--- /dev/null ++++ pyyaml-3.13/tests/data/overwrite-state-new-constructor.loader-error +@@ -0,0 +1,5 @@ ++- !!python/object/new:yaml.MappingNode ++ args: ++ state: ++ extend: test ++ __test__: test diff -Nru pyyaml-3.13/debian/patches/series pyyaml-3.13/debian/patches/series --- pyyaml-3.13/debian/patches/series 2019-01-10 11:09:51.000000000 +0000 +++ pyyaml-3.13/debian/patches/series 2020-03-07 05:43:00.000000000 +0000 @@ -1,2 +1,3 @@ 0001-support-high-codepoints.patch.patch 0002-Import-Hashable-from-collections.abc.patch +safety.patch
signature.asc
Description: This is a digitally signed message part.
