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

Prevents arbitrary code execution during python/object/new constructor #386

Merged
merged 3 commits into from Mar 17, 2020

Conversation

@ret2libc
Copy link
Contributor

@ret2libc ret2libc commented Mar 2, 2020

In FullLoader python/object/new constructor, implemented by
construct_python_object_apply, has support for setting the state of a
deserialized instance through the set_python_instance_state method.
After setting the state, some operations are performed on the instance
to complete its initialization, however it is possible for an attacker
to set the instance' state in such a way that arbitrary code is executed
by the FullLoader.

This patch tries to block such attacks in FullLoader by preventing
set_python_instance_state from setting arbitrary properties. It
implements a blacklist that includes extend method (called by
construct_python_object_apply) and all special methods (e.g. __set__,
__setitem__, etc.).

Users who need special attributes being set in the state of a
deserialized object can still do it through the UnsafeLoader, which
however should not be used on untrusted input. Additionally, they can
subclass FullLoader and redefine state_blacklist_regexp to include the
additional attributes they need, passing the subclassed loader to
yaml.load.

@ret2libc
Copy link
Contributor Author

@ret2libc ret2libc commented Mar 2, 2020

What shall I do to fix travis?

@ret2libc
Copy link
Contributor Author

@ret2libc ret2libc commented Mar 2, 2020

CVE-2020-1747 has been assigned to this flaw.

@perlpunk
Copy link
Member

@perlpunk perlpunk commented Mar 2, 2020

Thanks!
I fixed travis. Reason was that the libyaml testsuite is run by travis, and it pointed to a non-existing commit of the yaml-test-suite.

@perlpunk
Copy link
Member

@perlpunk perlpunk commented Mar 2, 2020

@ingydotnet my suggestion would be to prepare 5.3.1 with this fix.
I'm at a workshop this week, so I might only have time next weekend.

@kitterma
Copy link

@kitterma kitterma commented Mar 3, 2020

It looks like lib/yaml/contructor.py needs the same/similar changes too.

@ret2libc
Copy link
Contributor Author

@ret2libc ret2libc commented Mar 3, 2020

Ah, yes right. I forgot about python2.

@ret2libc ret2libc force-pushed the fix-object-new branch from 0e40611 to 8d9043c Mar 3, 2020
@ret2libc
Copy link
Contributor Author

@ret2libc ret2libc commented Mar 3, 2020

Changed lib/yaml/constructor.py as well. Thanks for noticing it.

@kitterma
Copy link

@kitterma kitterma commented Mar 3, 2020

I'm not part of the upstream team, so don't take this as any kind of direction to get this merged, but tests would be nice too. It would make it less likely that this issue would recur in the future.

Scott K

@ret2libc
Copy link
Contributor Author

@ret2libc ret2libc commented Mar 5, 2020

Thanks for the suggestion! Added a test as well.

Is there anything else I can do to improve the PR?

@perlpunk perlpunk requested a review from ingydotnet Mar 7, 2020
@perlpunk
Copy link
Member

@perlpunk perlpunk commented Mar 9, 2020

I requested a review from @ingydotnet , I messaged him in two IRC channels and in a private message, and sent a message on his phone.

@perlpunk
Copy link
Member

@perlpunk perlpunk commented Mar 9, 2020

@ingydotnet currently has limited internet access and will look at it as soon as possible.

lib/yaml/constructor.py Outdated Show resolved Hide resolved
Copy link
Member

@ingydotnet ingydotnet left a comment

Thanks for the work. I want to get this right because it is user facing and harder to change later.

tests/data/myfullloader.subclass_blacklist Outdated Show resolved Hide resolved
tests/lib3/test_constructor.py Outdated Show resolved Hide resolved
@ret2libc ret2libc force-pushed the fix-object-new branch 2 times, most recently from 43c1a0e to fdc125f Mar 11, 2020
Copy link
Member

@ingydotnet ingydotnet left a comment

Thanks for your timely worked. I wish we were in more amenable timezones. :)

Address the "private" method concern, and I think we'll be good. Cheers.

tests/lib3/test_constructor.py Outdated Show resolved Hide resolved
ret2libc added 2 commits Mar 13, 2020
In FullLoader python/object/new constructor, implemented by
construct_python_object_apply, has support for setting the state of a
deserialized instance through the set_python_instance_state method.
After setting the state, some operations are performed on the instance
to complete its initialization, however it is possible for an attacker
to set the instance' state in such a way that arbitrary code is executed
by the FullLoader.

This patch tries to block such attacks in FullLoader by preventing
set_python_instance_state from setting arbitrary properties. It
implements a blacklist that includes `extend` method (called by
construct_python_object_apply) and all special methods (e.g. __set__,
__setitem__, etc.).

Users who need special attributes being set in the state of a
deserialized object can still do it through the UnsafeLoader, which
however should not be used on untrusted input. Additionally, they can
subclass FullLoader and redefine `get_state_keys_blacklist()` to
extend/replace the list of blacklisted keys, passing the subclassed
loader to yaml.load.
@ingydotnet
Copy link
Member

@ingydotnet ingydotnet commented Mar 16, 2020

@ret2libc ping. I'm back from vacation. Where we at with this? Was waiting for you to comment on the API methods starting with an underscore. If we resolve that, I think @perlpunk can get a release out as soon as she has time.

@ingydotnet
Copy link
Member

@ingydotnet ingydotnet commented Mar 16, 2020

Sorry. Looks like you did fix that. Not sure how I missed it. I'll approve this.

Copy link
Member

@ingydotnet ingydotnet left a comment

I think this is ready to make a beta release. @perlpunk please proceed. (assuming you want to and have time, else let me know...)

@ret2libc hats off to your professional reporting of this and coming up with a solution that serves both YAML and Python well. Let me know if I can return the favor. 👍

def get_state_keys_blacklist(self):
return ['^extend$', '^__.*__$']

def get_state_keys_blacklist_regexp(self):
Copy link
Member

@ingydotnet ingydotnet Mar 16, 2020

I'm not sure this needs to be public method. But I'll go ahead and approve it. :)

Copy link
Member

@ingydotnet ingydotnet left a comment

Try my suggested fix to get travis et al passing.

tests/lib/test_constructor.py Outdated Show resolved Hide resolved
@perlpunk
Copy link
Member

@perlpunk perlpunk commented Mar 17, 2020

Great, thanks!
Will proceed with 5.3.1 release as soon as possible

@perlpunk perlpunk changed the base branch from master to release/5.3.1 Mar 17, 2020
@perlpunk perlpunk merged commit 5080ba5 into yaml:release/5.3.1 Mar 17, 2020
2 checks passed
@perlpunk
Copy link
Member

@perlpunk perlpunk commented Mar 18, 2020

Thanks @ret2libc
we just released 5.3.1
https://pypi.org/project/PyYAML/5.3.1/

perlpunk added a commit that referenced this issue Apr 2, 2020
#386)

* Prevents arbitrary code execution during python/object/new constructor

In FullLoader python/object/new constructor, implemented by
construct_python_object_apply, has support for setting the state of a
deserialized instance through the set_python_instance_state method.
After setting the state, some operations are performed on the instance
to complete its initialization, however it is possible for an attacker
to set the instance' state in such a way that arbitrary code is executed
by the FullLoader.

This patch tries to block such attacks in FullLoader by preventing
set_python_instance_state from setting arbitrary properties. It
implements a blacklist that includes `extend` method (called by
construct_python_object_apply) and all special methods (e.g. __set__,
__setitem__, etc.).

Users who need special attributes being set in the state of a
deserialized object can still do it through the UnsafeLoader, which
however should not be used on untrusted input. Additionally, they can
subclass FullLoader and redefine `get_state_keys_blacklist()` to
extend/replace the list of blacklisted keys, passing the subclassed
loader to yaml.load.

* Make sure python/object/new constructor does not set some properties

* Add test to show how to subclass FullLoader with new blacklist
buildroot-auto-update pushed a commit to buildroot/buildroot that referenced this issue Apr 7, 2020
Fixes the following security issue:

 386: Prevents arbitrary code execution during python/object/new
 constructor

yaml/pyyaml#386

The hash of the license file changed due to the following diff:

-Copyright (c) 2017-2019 Ingy döt Net
+Copyright (c) 2017-2020 Ingy döt Net

Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
(cherry picked from commit 9063df4)
[Peter: mention security impact]
Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
@kane3345678
Copy link

@kane3345678 kane3345678 commented Apr 25, 2020

Hello,
we recently download the 5.3.1 PyYAML-5.3.1-cp36-cp36m-win_amd64.whl and run the blackduck scan.

The tool still finds CVE-2020-1747 issue in _yaml.cp36-win_amd64.pyd .

May i know if that's fixed in the pyd file as well?
thank you!

uqs pushed a commit to freebsd/freebsd-ports that referenced this issue Apr 27, 2020
This release contains a security fix for CVE-2020-1747. FullLoader was still
exploitable for arbitrary command execution.
https://bugzilla.redhat.com/show_bug.cgi?id=1807367

Thanks to Riccardo Schirone (https://github.com/ret2libc) for both reporting
this and providing the fixes to resolve it.

  - yaml/pyyaml#386

PR:	245937
Submitted by:	daniel.engberg.lists@pyret.net
MFH:	2020Q2
Security:	http://vuxml.freebsd.org/freebsd/aae8fecf-888e-11ea-9714-08002718de91.html


git-svn-id: svn+ssh://svn.freebsd.org/ports/head@533167 35697150-7ecd-e111-bb59-0022644237b5
uqs pushed a commit to freebsd/freebsd-ports that referenced this issue Apr 27, 2020
This release contains a security fix for CVE-2020-1747. FullLoader was still
exploitable for arbitrary command execution.
https://bugzilla.redhat.com/show_bug.cgi?id=1807367

Thanks to Riccardo Schirone (https://github.com/ret2libc) for both reporting
this and providing the fixes to resolve it.

  - yaml/pyyaml#386

PR:	245937
Submitted by:	daniel.engberg.lists@pyret.net
MFH:	2020Q2
Security:	http://vuxml.freebsd.org/freebsd/aae8fecf-888e-11ea-9714-08002718de91.html
Jehops pushed a commit to Jehops/freebsd-ports-legacy that referenced this issue Apr 27, 2020
This release contains a security fix for CVE-2020-1747. FullLoader was still
exploitable for arbitrary command execution.
https://bugzilla.redhat.com/show_bug.cgi?id=1807367

Thanks to Riccardo Schirone (https://github.com/ret2libc) for both reporting
this and providing the fixes to resolve it.

  - yaml/pyyaml#386

PR:	245937
Submitted by:	daniel.engberg.lists@pyret.net
MFH:	2020Q2
Security:	http://vuxml.freebsd.org/freebsd/aae8fecf-888e-11ea-9714-08002718de91.html


git-svn-id: svn+ssh://svn.freebsd.org/ports/head@533167 35697150-7ecd-e111-bb59-0022644237b5
uqs pushed a commit to freebsd/freebsd-ports that referenced this issue Apr 28, 2020
Update to 5.3.1

This release contains a security fix for CVE-2020-1747. FullLoader was still
exploitable for arbitrary command execution.
https://bugzilla.redhat.com/show_bug.cgi?id=1807367

Thanks to Riccardo Schirone (https://github.com/ret2libc) for both reporting
this and providing the fixes to resolve it.

  - yaml/pyyaml#386

PR:	245937
Submitted by:	daniel.engberg.lists@pyret.net
Security:	http://vuxml.freebsd.org/freebsd/aae8fecf-888e-11ea-9714-08002718de91.html

Approved by:	portmgr (joneum)
asherf added a commit to asherf/pants that referenced this issue Apr 28, 2020
https://github.com/yaml/pyyaml/blob/d0d660d035905d9c49fc0f8dafb579d2cc68c0c8/CHANGES#L7

5.3.1 (2020-03-18)

* yaml/pyyaml#386 -- Prevents arbitrary code execution during python/object/new constructor

5.3 (2020-01-06)

* yaml/pyyaml#290 -- Use `is` instead of equality for comparing with `None`
* yaml/pyyaml#270 -- fix typos and stylistic nit
* yaml/pyyaml#309 -- Fix up small typo
* yaml/pyyaml#161 -- Fix handling of __slots__
* yaml/pyyaml#358 -- Allow calling add_multi_constructor with None
* yaml/pyyaml#285 -- Add use of safe_load() function in README
* yaml/pyyaml#351 -- Fix reader for Unicode code points over 0xFFFF
* yaml/pyyaml#360 -- Enable certain unicode tests when maxunicode not > 0xffff
* yaml/pyyaml#359 -- Use full_load in yaml-highlight example
* yaml/pyyaml#244 -- Document that PyYAML is implemented with Cython
* yaml/pyyaml#329 -- Fix for Python 3.10
* yaml/pyyaml#310 -- increase size of index, line, and column fields
* yaml/pyyaml#260 -- remove some unused imports
* yaml/pyyaml#163 -- Create timezone-aware datetimes when parsed as such
* yaml/pyyaml#363 -- Add tests for timezone

5.2 (2019-12-02)
------------------

* Repair incompatibilities introduced with 5.1. The default Loader was changed,
  but several methods like add_constructor still used the old default
  yaml/pyyaml#279 -- A more flexible fix for custom tag constructors
  yaml/pyyaml#287 -- Change default loader for yaml.add_constructor
  yaml/pyyaml#305 -- Change default loader for add_implicit_resolver, add_path_resolver
* Make FullLoader safer by removing python/object/apply from the default FullLoader
  yaml/pyyaml#347 -- Move constructor for object/apply to UnsafeConstructor
* Fix bug introduced in 5.1 where quoting went wrong on systems with sys.maxunicode <= 0xffff
  yaml/pyyaml#276 -- Fix logic for quoting special characters
* Other PRs:
  yaml/pyyaml#280 -- Update CHANGES for 5.1
asherf added a commit to asherf/pants that referenced this issue Apr 29, 2020
https://github.com/yaml/pyyaml/blob/d0d660d035905d9c49fc0f8dafb579d2cc68c0c8/CHANGES#L7

5.3.1 (2020-03-18)

* yaml/pyyaml#386 -- Prevents arbitrary code execution during python/object/new constructor

5.3 (2020-01-06)

* yaml/pyyaml#290 -- Use `is` instead of equality for comparing with `None`
* yaml/pyyaml#270 -- fix typos and stylistic nit
* yaml/pyyaml#309 -- Fix up small typo
* yaml/pyyaml#161 -- Fix handling of __slots__
* yaml/pyyaml#358 -- Allow calling add_multi_constructor with None
* yaml/pyyaml#285 -- Add use of safe_load() function in README
* yaml/pyyaml#351 -- Fix reader for Unicode code points over 0xFFFF
* yaml/pyyaml#360 -- Enable certain unicode tests when maxunicode not > 0xffff
* yaml/pyyaml#359 -- Use full_load in yaml-highlight example
* yaml/pyyaml#244 -- Document that PyYAML is implemented with Cython
* yaml/pyyaml#329 -- Fix for Python 3.10
* yaml/pyyaml#310 -- increase size of index, line, and column fields
* yaml/pyyaml#260 -- remove some unused imports
* yaml/pyyaml#163 -- Create timezone-aware datetimes when parsed as such
* yaml/pyyaml#363 -- Add tests for timezone

5.2 (2019-12-02)
------------------

* Repair incompatibilities introduced with 5.1. The default Loader was changed,
  but several methods like add_constructor still used the old default
  yaml/pyyaml#279 -- A more flexible fix for custom tag constructors
  yaml/pyyaml#287 -- Change default loader for yaml.add_constructor
  yaml/pyyaml#305 -- Change default loader for add_implicit_resolver, add_path_resolver
* Make FullLoader safer by removing python/object/apply from the default FullLoader
  yaml/pyyaml#347 -- Move constructor for object/apply to UnsafeConstructor
* Fix bug introduced in 5.1 where quoting went wrong on systems with sys.maxunicode <= 0xffff
  yaml/pyyaml#276 -- Fix logic for quoting special characters
* Other PRs:
  yaml/pyyaml#280 -- Update CHANGES for 5.1
Eric-Arellano pushed a commit to pantsbuild/pants that referenced this issue May 1, 2020
https://github.com/yaml/pyyaml/blob/d0d660d035905d9c49fc0f8dafb579d2cc68c0c8/CHANGES#L7

5.3.1 (2020-03-18)

* yaml/pyyaml#386 -- Prevents arbitrary code execution during python/object/new constructor

5.3 (2020-01-06)

* yaml/pyyaml#290 -- Use `is` instead of equality for comparing with `None`
* yaml/pyyaml#270 -- fix typos and stylistic nit
* yaml/pyyaml#309 -- Fix up small typo
* yaml/pyyaml#161 -- Fix handling of __slots__
* yaml/pyyaml#358 -- Allow calling add_multi_constructor with None
* yaml/pyyaml#285 -- Add use of safe_load() function in README
* yaml/pyyaml#351 -- Fix reader for Unicode code points over 0xFFFF
* yaml/pyyaml#360 -- Enable certain unicode tests when maxunicode not > 0xffff
* yaml/pyyaml#359 -- Use full_load in yaml-highlight example
* yaml/pyyaml#244 -- Document that PyYAML is implemented with Cython
* yaml/pyyaml#329 -- Fix for Python 3.10
* yaml/pyyaml#310 -- increase size of index, line, and column fields
* yaml/pyyaml#260 -- remove some unused imports
* yaml/pyyaml#163 -- Create timezone-aware datetimes when parsed as such
* yaml/pyyaml#363 -- Add tests for timezone

5.2 (2019-12-02)
------------------

* Repair incompatibilities introduced with 5.1. The default Loader was changed,
  but several methods like add_constructor still used the old default
  yaml/pyyaml#279 -- A more flexible fix for custom tag constructors
  yaml/pyyaml#287 -- Change default loader for yaml.add_constructor
  yaml/pyyaml#305 -- Change default loader for add_implicit_resolver, add_path_resolver
* Make FullLoader safer by removing python/object/apply from the default FullLoader
  yaml/pyyaml#347 -- Move constructor for object/apply to UnsafeConstructor
* Fix bug introduced in 5.1 where quoting went wrong on systems with sys.maxunicode <= 0xffff
  yaml/pyyaml#276 -- Fix logic for quoting special characters
* Other PRs:
  yaml/pyyaml#280 -- Update CHANGES for 5.1
netgate-git-updates pushed a commit to pfsense/FreeBSD-ports that referenced this issue May 4, 2020
Update to 5.3.1

This release contains a security fix for CVE-2020-1747. FullLoader was still
exploitable for arbitrary command execution.
https://bugzilla.redhat.com/show_bug.cgi?id=1807367

Thanks to Riccardo Schirone (https://github.com/ret2libc) for both reporting
this and providing the fixes to resolve it.

  - yaml/pyyaml#386

PR:	245937
Submitted by:	daniel.engberg.lists@pyret.net
Security:	http://vuxml.freebsd.org/freebsd/aae8fecf-888e-11ea-9714-08002718de91.html

Approved by:	portmgr (joneum)

(cherry picked from commit ed0efb6)
william-gr added a commit to truenas/ports that referenced this issue Aug 10, 2020
This release contains a security fix for CVE-2020-1747. FullLoader was still
exploitable for arbitrary command execution.
https://bugzilla.redhat.com/show_bug.cgi?id=1807367

Thanks to Riccardo Schirone (https://github.com/ret2libc) for both reporting
this and providing the fixes to resolve it.

  - yaml/pyyaml#386

PR:	245937
Submitted by:	daniel.engberg.lists@pyret.net
MFH:	2020Q2
Security:	http://vuxml.freebsd.org/freebsd/aae8fecf-888e-11ea-9714-08002718de91.html
uqs pushed a commit to freebsd/freebsd-ports that referenced this issue Apr 1, 2021
Update to 5.3.1

This release contains a security fix for CVE-2020-1747. FullLoader was still
exploitable for arbitrary command execution.
https://bugzilla.redhat.com/show_bug.cgi?id=1807367

Thanks to Riccardo Schirone (https://github.com/ret2libc) for both reporting
this and providing the fixes to resolve it.

  - yaml/pyyaml#386

PR:	245937
Submitted by:	daniel.engberg.lists@pyret.net
Security:	http://vuxml.freebsd.org/freebsd/aae8fecf-888e-11ea-9714-08002718de91.html

Approved by:	portmgr (joneum)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants