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

need SafeLoad and SafeDump analog to python #45

Closed
rurban opened this Issue Mar 10, 2016 · 37 comments

Comments

Projects
None yet
6 participants
@rurban

rurban commented Mar 10, 2016

In python you have to register safe classes which are then allowed to be dumped or loaded.
See the available metasploits to weaponize not being able to safe load.

This will need OO methods, not just the silly (no warnings 'once') options handling as of now.

$yaml = new YAML::XS::SafeLoader;
$yaml->add_safe("DateTime");
$yaml->SafeLoadFile("my.yml");

with a !DateTime object in the yml.

So the other methods should accept the optional 1st class arg also. And then you could just add the options behind the mandatory one-only string/filename/data argument as pairs as in python

@rurban

This comment has been minimized.

Show comment
Hide comment
@rurban

rurban Mar 13, 2016

I started implementing it now, with new Dumper, Loader, SafeDumper and SafeLoader classes,
and options as methods. A proper OO interface.

rurban commented Mar 13, 2016

I started implementing it now, with new Dumper, Loader, SafeDumper and SafeLoader classes,
and options as methods. A proper OO interface.

rurban pushed a commit to rurban/yaml-libyaml-pm that referenced this issue Mar 13, 2016

Reini Urban
Safe methods + obj-> methods + options (WIP)
new classes Loader, SafeLoader, Dumper, SafeDumper
See GH ingydotnet#45

rurban pushed a commit to rurban/yaml-libyaml-pm that referenced this issue Mar 14, 2016

Reini Urban
Safe methods + obj-> methods + options (WIP)
new classes Loader, SafeLoader, Dumper, SafeDumper
See GH ingydotnet#45
@rurban

This comment has been minimized.

Show comment
Hide comment
@rurban

rurban Oct 15, 2016

or maybe it's overkill to filter certain classes/tags and just disable them completely, as in $YAML::Syck::LoadBlessed = 0;

rurban commented Oct 15, 2016

or maybe it's overkill to filter certain classes/tags and just disable them completely, as in $YAML::Syck::LoadBlessed = 0;

@perlpunk

This comment has been minimized.

Show comment
Hide comment
@perlpunk

perlpunk Oct 15, 2016

Collaborator

@rurban doesn't sound like overkill to me. do other implementations, like in python, support this?
of course i don't know what difference it would make regarding the amount of code.

Collaborator

perlpunk commented Oct 15, 2016

@rurban doesn't sound like overkill to me. do other implementations, like in python, support this?
of course i don't know what difference it would make regarding the amount of code.

@rurban

This comment has been minimized.

Show comment
Hide comment
@rurban

rurban Oct 15, 2016

@perlpunk yes, Python has it like described. I implemented it halfway in my safe branch. Have a look.

rurban commented Oct 15, 2016

@perlpunk yes, Python has it like described. I implemented it halfway in my safe branch. Have a look.

@nthykier

This comment has been minimized.

Show comment
Hide comment
@nthykier

nthykier May 6, 2017

Hi,

Anything that could speed up this ? I was quite to surprised to learn that YAML::XS would load classes by default (especially when it has a "LoadCode" variable set to 0 that apparently controls something else). Currently I am looking at an arbitrary code execution via YAML (CVE pending) and no option to solve it short of disabling my feature.

If possible, I would like to still have this feature, but I cannot support with YAML::XS not having a SafeLoad method.

nthykier commented May 6, 2017

Hi,

Anything that could speed up this ? I was quite to surprised to learn that YAML::XS would load classes by default (especially when it has a "LoadCode" variable set to 0 that apparently controls something else). Currently I am looking at an arbitrary code execution via YAML (CVE pending) and no option to solve it short of disabling my feature.

If possible, I would like to still have this feature, but I cannot support with YAML::XS not having a SafeLoad method.

@nthykier

This comment has been minimized.

Show comment
Hide comment
@nthykier

nthykier commented May 6, 2017

@rurban

This comment has been minimized.

Show comment
Hide comment
@rurban

rurban May 7, 2017

See perl11/cperl#198 So far I refrained from forking it under a new name.

I don't like the Cpanel::JSON:XS fork name neither. It should be JSON::XS proper. Both parties so far refrained from handing it over, and rather let their stuff die.

rurban commented May 7, 2017

See perl11/cperl#198 So far I refrained from forking it under a new name.

I don't like the Cpanel::JSON:XS fork name neither. It should be JSON::XS proper. Both parties so far refrained from handing it over, and rather let their stuff die.

@dod38fr

This comment has been minimized.

Show comment
Hide comment
@dod38fr

dod38fr May 12, 2017

Another possibility is to consider safe a class without DESTROY method. (I think this is the only way where "blessed" data can be harmful.)

Knowing that UNIVERSAL has no DESTROY method, It's fairly easy to test:

$ perl -MFile::Temp -E 'say File::Temp->can("DESTROY") ? "yes" : "no";'
yes
$ perl -E 'say UNIVERSAL->can("DESTROY") ? "yes" : "no";'
no
$ perl -MGetopt::Long -E 'say Getopt::Long->can("DESTROY") ? "yes" : "no";'
no
$ perl -MDateTime -E 'say DateTime->can("DESTROY") ? "yes" : "no";'
no

HTH

dod38fr commented May 12, 2017

Another possibility is to consider safe a class without DESTROY method. (I think this is the only way where "blessed" data can be harmful.)

Knowing that UNIVERSAL has no DESTROY method, It's fairly easy to test:

$ perl -MFile::Temp -E 'say File::Temp->can("DESTROY") ? "yes" : "no";'
yes
$ perl -E 'say UNIVERSAL->can("DESTROY") ? "yes" : "no";'
no
$ perl -MGetopt::Long -E 'say Getopt::Long->can("DESTROY") ? "yes" : "no";'
no
$ perl -MDateTime -E 'say DateTime->can("DESTROY") ? "yes" : "no";'
no

HTH

@rurban

This comment has been minimized.

Show comment
Hide comment
@rurban

rurban commented May 12, 2017

@dod38fr yes, agreed.

@jwilk

This comment has been minimized.

Show comment
Hide comment
@jwilk

jwilk May 12, 2017

Contributor

I don't believe it's enough to check for the DESTROY method.

You normally process the unserialized data somehow, and that may invoke overloaded operators. E.g.:

package Very::Dangerous;

use overload '""' => sub {
	die "Eeek! ${\ref $_[0]} objects are not safe to stringify";
};

package main;

use YAML::XS;

my $d = Load('- !Very::Dangerous');
for (@{$d}) {
	print "$_\n";
}

dies with:

Eeek! Very::Dangerous objects are not safe to stringify at test.pl line 4.

Other theoretical problems with using the can method:

  • An object can have a destructor even when can say it's missing, thanks to the AUTOLOAD mechanism. (But this was broken before Perl 5.24: https://rt.perl.org/Public/Bug/Display.html?id=124387).

  • A class could override the can method. Mere checking for existence of DESTROY could cause execution of unwanted code.

Contributor

jwilk commented May 12, 2017

I don't believe it's enough to check for the DESTROY method.

You normally process the unserialized data somehow, and that may invoke overloaded operators. E.g.:

package Very::Dangerous;

use overload '""' => sub {
	die "Eeek! ${\ref $_[0]} objects are not safe to stringify";
};

package main;

use YAML::XS;

my $d = Load('- !Very::Dangerous');
for (@{$d}) {
	print "$_\n";
}

dies with:

Eeek! Very::Dangerous objects are not safe to stringify at test.pl line 4.

Other theoretical problems with using the can method:

  • An object can have a destructor even when can say it's missing, thanks to the AUTOLOAD mechanism. (But this was broken before Perl 5.24: https://rt.perl.org/Public/Bug/Display.html?id=124387).

  • A class could override the can method. Mere checking for existence of DESTROY could cause execution of unwanted code.

@rurban

This comment has been minimized.

Show comment
Hide comment
@rurban

rurban May 12, 2017

Ok.

  • The first overload example can be checked for an overloaded class. (AMT magic). This should be enough.

But:

  • The can override and AUTOLOAD mechanism can bring in an undesired DETROY, so it cannot be safely checked. So we cannot declare such classes as safe. Better be safe and explicit without our safe declarations than sorry.

Thanks a lot!

rurban commented May 12, 2017

Ok.

  • The first overload example can be checked for an overloaded class. (AMT magic). This should be enough.

But:

  • The can override and AUTOLOAD mechanism can bring in an undesired DETROY, so it cannot be safely checked. So we cannot declare such classes as safe. Better be safe and explicit without our safe declarations than sorry.

Thanks a lot!

@dod38fr

This comment has been minimized.

Show comment
Hide comment
@dod38fr

dod38fr Nov 11, 2017

I've prepared a patch for Debian to provide a SafeLoad method (with self-tests).

What do you think of this solution ?

If you want, I can prepare a PR for this repo.

All the best

dod38fr commented Nov 11, 2017

I've prepared a patch for Debian to provide a SafeLoad method (with self-tests).

What do you think of this solution ?

If you want, I can prepare a PR for this repo.

All the best

@perlpunk

This comment has been minimized.

Show comment
Hide comment
@perlpunk

perlpunk Nov 11, 2017

Collaborator

@dod38fr Thanks.
I have been working/thinking on this, too.
My point of view on this is:

  1. safe loading should become the default (at some point)
  2. There shouldn't be a subroutine SafeLoad (and possible SafeLoadFile), but a more general Schema API that allows to enable/disable several schemas (like !local tags, tag:example.org:foo and the Failsafe/JSON/Core schema from YAML 1.2). It would also allow you to only load objects for specific classes.

I am currently working on a pure perl processor YAML::PP and want to implement the API there first.
For most users that don't know about those YAML features this might sound like overkill, but that's what YAML was also designed for, and if we introduce SafeLoad now, we have to support this later too.

Personally, I would want to have safe loading rather today than tomorrow. I also talked to @ingydotnet, and he really doesn't want to introduce something like that before we have a standard API for it.

Collaborator

perlpunk commented Nov 11, 2017

@dod38fr Thanks.
I have been working/thinking on this, too.
My point of view on this is:

  1. safe loading should become the default (at some point)
  2. There shouldn't be a subroutine SafeLoad (and possible SafeLoadFile), but a more general Schema API that allows to enable/disable several schemas (like !local tags, tag:example.org:foo and the Failsafe/JSON/Core schema from YAML 1.2). It would also allow you to only load objects for specific classes.

I am currently working on a pure perl processor YAML::PP and want to implement the API there first.
For most users that don't know about those YAML features this might sound like overkill, but that's what YAML was also designed for, and if we introduce SafeLoad now, we have to support this later too.

Personally, I would want to have safe loading rather today than tomorrow. I also talked to @ingydotnet, and he really doesn't want to introduce something like that before we have a standard API for it.

@perlpunk

This comment has been minimized.

Show comment
Hide comment
@perlpunk

perlpunk Nov 11, 2017

Collaborator

Btw, for being safe after a Load you can also try https://metacpan.org/pod/Data::Structure::Util

use Data::Structure::Util qw/ unbless circular_off /;
# unbless all objects
unbless( $data );
# and possibly remove circular refs
circular_off( $data );

I can add this to YAML::XS documentation, if it makes sense.

Collaborator

perlpunk commented Nov 11, 2017

Btw, for being safe after a Load you can also try https://metacpan.org/pod/Data::Structure::Util

use Data::Structure::Util qw/ unbless circular_off /;
# unbless all objects
unbless( $data );
# and possibly remove circular refs
circular_off( $data );

I can add this to YAML::XS documentation, if it makes sense.

@dod38fr

This comment has been minimized.

Show comment
Hide comment
@dod38fr

dod38fr Nov 11, 2017

@perlpunk hmm. ok. Good points.

I've disabled the patch: SafeLoad won't be shipped on Debian.

Thanks for the idea about using unbless. This gives another way to patch an insecure application. This is probably easier than replacing YAML::XS with YAML::Tiny.

All the best

dod38fr commented Nov 11, 2017

@perlpunk hmm. ok. Good points.

I've disabled the patch: SafeLoad won't be shipped on Debian.

Thanks for the idea about using unbless. This gives another way to patch an insecure application. This is probably easier than replacing YAML::XS with YAML::Tiny.

All the best

@rurban

This comment has been minimized.

Show comment
Hide comment
@rurban

rurban Nov 11, 2017

rurban commented Nov 11, 2017

@perlpunk

This comment has been minimized.

Show comment
Hide comment
@perlpunk

perlpunk Nov 11, 2017

Collaborator

@rurban, I agree that unbless is a workaround solution, and objects shouldn't be created at all, but I would be interested in how that could still be exploitable.

Collaborator

perlpunk commented Nov 11, 2017

@rurban, I agree that unbless is a workaround solution, and objects shouldn't be created at all, but I would be interested in how that could still be exploitable.

@perlpunk

This comment has been minimized.

Show comment
Hide comment
@perlpunk

perlpunk Dec 9, 2017

Collaborator

@dod38fr I started with a Schema implementation in YAML::PP, however, this is going to take more time, and even more time to support it in some way YAML::XS...
I'm leaning towards introducing a $SafeLoad variable now.
It would be false by default for now, and maybe set to true at some point.
Alternatively $LoadObjects = 1.
Of course, just like with SafeLoad(), this has to be supported in the future. It has the advantage that it covers also LoadFile without the need to write SafeLoadFile()
Unfortunately it has the usual disadvantages of such global variables, but that's the way YAML::XS works right now.

Collaborator

perlpunk commented Dec 9, 2017

@dod38fr I started with a Schema implementation in YAML::PP, however, this is going to take more time, and even more time to support it in some way YAML::XS...
I'm leaning towards introducing a $SafeLoad variable now.
It would be false by default for now, and maybe set to true at some point.
Alternatively $LoadObjects = 1.
Of course, just like with SafeLoad(), this has to be supported in the future. It has the advantage that it covers also LoadFile without the need to write SafeLoadFile()
Unfortunately it has the usual disadvantages of such global variables, but that's the way YAML::XS works right now.

@rurban

This comment has been minimized.

Show comment
Hide comment
@rurban

rurban Dec 9, 2017

Well, that's why I converted the global infected XS API to a better functional interface in my safe branch

rurban commented Dec 9, 2017

Well, that's why I converted the global infected XS API to a better functional interface in my safe branch

@perlpunk

This comment has been minimized.

Show comment
Hide comment
@perlpunk

perlpunk Dec 9, 2017

Collaborator

Maybe it helps if I write down what I would like to see in a new API, and why it should be called Schema.
The standard YAML Tags like !!str, !!bool are resolved with a schema in YAML 1.2.
(Since YAML::XS should support 1.2 in the future, it should support this).
YAML 1.2 has the schemas Failsafe, JSON and Core.
The tags like !!perl/type:Class can also be seen as a schema. (It should also be noted that YAML 1.2 suggests only one ! for such a language specific schema, since !! is "reserved" for the standard schema.)
This schema could be called Perl.
And then there are local tags like !Foo. This can also be seen as a schema.
For each userdefined schema you should have fine grained control on the behaviour.
Additionally you can have a URI schema (the standard ones !! are actually shorthands for !<tag:yaml.org,2002:...>).

My wishes for the new API regarding loading Objects:

  • Be able to turn on/off loading objects completely (that includes things like !!perl/hash:Foo and !Foo)
  • Turn on/off loading generic objects via the Perl schema (!!perl/hash:Foo, !perl/hash:Foo)
  • Turn on/off loading objects from local tags (!Foo)
  • Enable only certain classes (specific ones and via regex)
  • Decide what to do for certain classes via callback (i.e. you might want to do a Class->new($yamldata) or Class->inflate($yamldata) instead of just bless $yamldata, "Class").
  • Create an object from a matching regex (see the Dice examples in the PyYAML documentation: http://pyyaml.org/wiki/PyYAMLDocumentation)
  • Also you need the same kind of control over Dumping

This is what YAML was made for, and in other languages people are actually using this.

That's why I think now that simple adding SafeLoad or $SafeLoad will not be enough for the future. But OTOH we should make it safe ASAP.

Collaborator

perlpunk commented Dec 9, 2017

Maybe it helps if I write down what I would like to see in a new API, and why it should be called Schema.
The standard YAML Tags like !!str, !!bool are resolved with a schema in YAML 1.2.
(Since YAML::XS should support 1.2 in the future, it should support this).
YAML 1.2 has the schemas Failsafe, JSON and Core.
The tags like !!perl/type:Class can also be seen as a schema. (It should also be noted that YAML 1.2 suggests only one ! for such a language specific schema, since !! is "reserved" for the standard schema.)
This schema could be called Perl.
And then there are local tags like !Foo. This can also be seen as a schema.
For each userdefined schema you should have fine grained control on the behaviour.
Additionally you can have a URI schema (the standard ones !! are actually shorthands for !<tag:yaml.org,2002:...>).

My wishes for the new API regarding loading Objects:

  • Be able to turn on/off loading objects completely (that includes things like !!perl/hash:Foo and !Foo)
  • Turn on/off loading generic objects via the Perl schema (!!perl/hash:Foo, !perl/hash:Foo)
  • Turn on/off loading objects from local tags (!Foo)
  • Enable only certain classes (specific ones and via regex)
  • Decide what to do for certain classes via callback (i.e. you might want to do a Class->new($yamldata) or Class->inflate($yamldata) instead of just bless $yamldata, "Class").
  • Create an object from a matching regex (see the Dice examples in the PyYAML documentation: http://pyyaml.org/wiki/PyYAMLDocumentation)
  • Also you need the same kind of control over Dumping

This is what YAML was made for, and in other languages people are actually using this.

That's why I think now that simple adding SafeLoad or $SafeLoad will not be enough for the future. But OTOH we should make it safe ASAP.

@dod38fr

This comment has been minimized.

Show comment
Hide comment
@dod38fr

dod38fr Dec 10, 2017

@perlpunk Having a $SafeLoad global variable is better than nothing.

I'd rather see $SafeLoad than $LoadObject because:

  • its name is more likely to catch the attention of users than $LoadObjects: everybody is concerned by safety. On the other hand most people don't care about loading objects in Yaml so $loadObject is more likely to be ignored.
  • its default value is falsy, i.e. a missing value means falsy, which is easier to handle than a missing value meaning truthy value

Thanks for your work on YAML.

All the best

dod38fr commented Dec 10, 2017

@perlpunk Having a $SafeLoad global variable is better than nothing.

I'd rather see $SafeLoad than $LoadObject because:

  • its name is more likely to catch the attention of users than $LoadObjects: everybody is concerned by safety. On the other hand most people don't care about loading objects in Yaml so $loadObject is more likely to be ignored.
  • its default value is falsy, i.e. a missing value means falsy, which is easier to handle than a missing value meaning truthy value

Thanks for your work on YAML.

All the best

@perlpunk

This comment has been minimized.

Show comment
Hide comment
@perlpunk

perlpunk Dec 11, 2017

Collaborator

as I cannot decide this, trying to get feedback from @ingydotnet ...

Collaborator

perlpunk commented Dec 11, 2017

as I cannot decide this, trying to get feedback from @ingydotnet ...

@ingydotnet

This comment has been minimized.

Show comment
Hide comment
@ingydotnet

ingydotnet Dec 19, 2017

Owner

After talking to @perlpunk on the phone, the quickest solution is to add a $YAML::XS::LoadBlessed global that can be set to 0 or 1, and will default to 1 for now.

This global var name was chosen because it is already present in YAML::Syck. I didn't want to add any more globals, but @perlpunk convinced me it was best in this case. We are working on a new API for all YAML modules (pm XS Syck); it won't have globals, and no need to get fancy on the legacy global solution.

Expect this fix to be out in short order...

Owner

ingydotnet commented Dec 19, 2017

After talking to @perlpunk on the phone, the quickest solution is to add a $YAML::XS::LoadBlessed global that can be set to 0 or 1, and will default to 1 for now.

This global var name was chosen because it is already present in YAML::Syck. I didn't want to add any more globals, but @perlpunk convinced me it was best in this case. We are working on a new API for all YAML modules (pm XS Syck); it won't have globals, and no need to get fancy on the legacy global solution.

Expect this fix to be out in short order...

@dod38fr

This comment has been minimized.

Show comment
Hide comment
@dod38fr

dod38fr Dec 20, 2017

Fair enough. I'm fine with $YAML::XS::LoadBlessed global.

Thanks for your work.

All the best

dod38fr commented Dec 20, 2017

Fair enough. I'm fine with $YAML::XS::LoadBlessed global.

Thanks for your work.

All the best

perlpunk added a commit that referenced this issue Dec 20, 2017

Add $LoadBlessed
For backwards compatibility the default is true.

See issue #45

perlpunk added a commit that referenced this issue Dec 20, 2017

Add $LoadBlessed
For backwards compatibility the default is true.

See issue #45
@perlpunk

This comment has been minimized.

Show comment
Hide comment
@perlpunk

perlpunk Dec 20, 2017

Collaborator

PR #73 created

Collaborator

perlpunk commented Dec 20, 2017

PR #73 created

@perlpunk

This comment has been minimized.

Show comment
Hide comment
@perlpunk

perlpunk Dec 21, 2017

Collaborator

Sorry, I missed some spots as I was only looking for the call sv_bless. Will do another PR.

Collaborator

perlpunk commented Dec 21, 2017

Sorry, I missed some spots as I was only looking for the call sv_bless. Will do another PR.

@perlpunk

This comment has been minimized.

Show comment
Hide comment
@perlpunk

perlpunk Dec 21, 2017

Collaborator

PR #74 created

Collaborator

perlpunk commented Dec 21, 2017

PR #74 created

@perlpunk

This comment has been minimized.

Show comment
Hide comment
@perlpunk

perlpunk Dec 26, 2017

Collaborator

Released YAML-LibYAML-0.69.tar.gz
thanks for all comments!

Collaborator

perlpunk commented Dec 26, 2017

Released YAML-LibYAML-0.69.tar.gz
thanks for all comments!

@perlpunk perlpunk closed this Dec 26, 2017

@perlpunk

This comment has been minimized.

Show comment
Hide comment
@perlpunk

perlpunk Mar 9, 2018

Collaborator

@rurban I would be interested in why you still say that YAML::XS is vulnerable in https://news.ycombinator.com/item?id=16469364
Also I asked you before how using unbless() would be exploitable if you use YAML::XS < 0.69, but I didn't get an answer.
I think it would be more productive to discuss that here instead of spreading potential FUD.
If $LoadBlessed = 0 is vulnerable, I would like to hear about it.
If unless is vulnerable I'd like to hear about it.
I'm interested in making it safe, and if I wrote something wrong in my article, I would like to fix it.

Note: I know that Data::Structure::Util has problems with tied data, but YAML::XS does not produce such data. YAML.pm does when using $YAML::Preserve, maybe I should mention that in the article.

Collaborator

perlpunk commented Mar 9, 2018

@rurban I would be interested in why you still say that YAML::XS is vulnerable in https://news.ycombinator.com/item?id=16469364
Also I asked you before how using unbless() would be exploitable if you use YAML::XS < 0.69, but I didn't get an answer.
I think it would be more productive to discuss that here instead of spreading potential FUD.
If $LoadBlessed = 0 is vulnerable, I would like to hear about it.
If unless is vulnerable I'd like to hear about it.
I'm interested in making it safe, and if I wrote something wrong in my article, I would like to fix it.

Note: I know that Data::Structure::Util has problems with tied data, but YAML::XS does not produce such data. YAML.pm does when using $YAML::Preserve, maybe I should mention that in the article.

@jwilk

This comment has been minimized.

Show comment
Hide comment
@jwilk

jwilk Mar 9, 2018

Contributor

The unbless approach is indeed insecure. The example code from the blog post looks like this:

use Data::Structure::Util qw/ unbless /;
my $data = Load $yaml;
unbless $data;

But Load can return a list, and then you unbless only the last element.
There's a much bigger problem, though: if YAML is not syntactically correct, unbless won't even get a chance to be run.

Proof of concept:

package Very::Dangerous;

sub DESTROY {
	print STDERR "Eeek!\n";
}

package main;

use YAML::XS;
use Data::Structure::Util qw/ unbless /;

my $yaml = "!Very::Dangerous\n---\n:";
my @data = Load $yaml;
unbless @data;

Here's what happens:

YAML::XS::Load Error: The problem:

    did not find expected key

was found at document: 2, line: 3, column: 1
while parsing a block mapping at line: 3, column: 1
Eeek!
Contributor

jwilk commented Mar 9, 2018

The unbless approach is indeed insecure. The example code from the blog post looks like this:

use Data::Structure::Util qw/ unbless /;
my $data = Load $yaml;
unbless $data;

But Load can return a list, and then you unbless only the last element.
There's a much bigger problem, though: if YAML is not syntactically correct, unbless won't even get a chance to be run.

Proof of concept:

package Very::Dangerous;

sub DESTROY {
	print STDERR "Eeek!\n";
}

package main;

use YAML::XS;
use Data::Structure::Util qw/ unbless /;

my $yaml = "!Very::Dangerous\n---\n:";
my @data = Load $yaml;
unbless @data;

Here's what happens:

YAML::XS::Load Error: The problem:

    did not find expected key

was found at document: 2, line: 3, column: 1
while parsing a block mapping at line: 3, column: 1
Eeek!
@perlpunk

This comment has been minimized.

Show comment
Hide comment
@perlpunk

perlpunk Mar 9, 2018

Collaborator

@jwilk Thanks for the hint about the list!
Yes, it's correct that you should always assign to an array.

Also thanks for the second example. I did not think about that one. It's also not possible to catch that with an eval.
YAML::XS will start to bless things during parsing, so there's no way to prevent that.

I will edit my article!

Collaborator

perlpunk commented Mar 9, 2018

@jwilk Thanks for the hint about the list!
Yes, it's correct that you should always assign to an array.

Also thanks for the second example. I did not think about that one. It's also not possible to catch that with an eval.
YAML::XS will start to bless things during parsing, so there's no way to prevent that.

I will edit my article!

@perlpunk

This comment has been minimized.

Show comment
Hide comment
@perlpunk

perlpunk Mar 9, 2018

Collaborator

Edited the article. Sorry for writing a misleading article.

Now still missing from @rurban is how to exploit YAML::XS 0.69

Collaborator

perlpunk commented Mar 9, 2018

Edited the article. Sorry for writing a misleading article.

Now still missing from @rurban is how to exploit YAML::XS 0.69

@rurban

This comment has been minimized.

Show comment
Hide comment
@rurban

rurban Mar 9, 2018

Read my docs and watch the video

http://search.cpan.org/~rurban/Cpanel-JSON-XS-4.02/XS.pm#SECURITY_CONSIDERATIONS
"Watch https://www.youtube.com/watch?v=Gzx6KlqiIZE for an exploit demo for "CVE-2015-1592 SixApart MovableType Storable Perl Code Execution" for a deserializer which expands objects. Deserializing even coderefs (methods, functions) or external data would be considered the most dangerous."

rurban commented Mar 9, 2018

Read my docs and watch the video

http://search.cpan.org/~rurban/Cpanel-JSON-XS-4.02/XS.pm#SECURITY_CONSIDERATIONS
"Watch https://www.youtube.com/watch?v=Gzx6KlqiIZE for an exploit demo for "CVE-2015-1592 SixApart MovableType Storable Perl Code Execution" for a deserializer which expands objects. Deserializing even coderefs (methods, functions) or external data would be considered the most dangerous."

@perlpunk

This comment has been minimized.

Show comment
Hide comment
@perlpunk

perlpunk Mar 9, 2018

Collaborator

@rurban My question is rather, is it still exploitable if setting $YAML::XS::LoadBlessed = 0. I know it's bad that the default is currently 1, and we might change this in the future, but how can it be exploited? Only if I forgot something in my fix. The video is from 2015, so it can't have tested YAML::XS 0.69.

Collaborator

perlpunk commented Mar 9, 2018

@rurban My question is rather, is it still exploitable if setting $YAML::XS::LoadBlessed = 0. I know it's bad that the default is currently 1, and we might change this in the future, but how can it be exploited? Only if I forgot something in my fix. The video is from 2015, so it can't have tested YAML::XS 0.69.

@rurban

This comment has been minimized.

Show comment
Hide comment
@rurban

rurban Mar 9, 2018

The video is for Storable, but an attacker would use the same mechanisms as shown there. @jwilk hinted at it also.
With $YAML::XS::LoadBlessed = 0 of course this method would not work. But most people do want whitelisting, and know it from python.

rurban commented Mar 9, 2018

The video is for Storable, but an attacker would use the same mechanisms as shown there. @jwilk hinted at it also.
With $YAML::XS::LoadBlessed = 0 of course this method would not work. But most people do want whitelisting, and know it from python.

@perlpunk

This comment has been minimized.

Show comment
Hide comment
@perlpunk

perlpunk Mar 9, 2018

Collaborator

@rurban then please don't keep saying that YAML::XS is vulnerable in this regard.
(I still would personally not load untrusted YAML with it because of potential memory leaks, stack overflows. If, then I would run it in its own process, limit the input size and the process ressources. But it won't create objects.)

PyYAML implements its constructor in python, while YAML::XS is (almost) completely written in C.
PyYAML lets you define custom constructors, and that's something I started to implement in YAML::PP, too.

We discussed already why we didn't want to add a simple whitelisting mechanism in YAML::XS for now.
Adding an API like I plan for YAML::PP will be a lot of work for YAML::XS.
Maybe a Perl frontend with libyaml backend would be useful.

If you want to load untrusted YAML and want objects, you have to wait a bit until YAML::PP can do it.

Collaborator

perlpunk commented Mar 9, 2018

@rurban then please don't keep saying that YAML::XS is vulnerable in this regard.
(I still would personally not load untrusted YAML with it because of potential memory leaks, stack overflows. If, then I would run it in its own process, limit the input size and the process ressources. But it won't create objects.)

PyYAML implements its constructor in python, while YAML::XS is (almost) completely written in C.
PyYAML lets you define custom constructors, and that's something I started to implement in YAML::PP, too.

We discussed already why we didn't want to add a simple whitelisting mechanism in YAML::XS for now.
Adding an API like I plan for YAML::PP will be a lot of work for YAML::XS.
Maybe a Perl frontend with libyaml backend would be useful.

If you want to load untrusted YAML and want objects, you have to wait a bit until YAML::PP can do it.

@rurban

This comment has been minimized.

Show comment
Hide comment
@rurban

rurban Mar 10, 2018

If you want to load untrusted YAML and want objects, you have to wait a bit until YAML::PP can do it.

No. Just use my variant from cperl, which is fast and safe. And is usable for CPAN, unlike your variant.

rurban commented Mar 10, 2018

If you want to load untrusted YAML and want objects, you have to wait a bit until YAML::PP can do it.

No. Just use my variant from cperl, which is fast and safe. And is usable for CPAN, unlike your variant.

gentoo-bot pushed a commit to gentoo/gentoo that referenced this issue Apr 3, 2018

dev-perl/YAML-LibYAML: Bump to version 0.690.0
Upstream:
- (Security) Add $LoadBlessed option[1] to run on/off loading objects
- Fix regex roundtrip [2]
- Fix loading of many regex classes [3]
- Support tags !!str, !!map and !!seq [4]
- Support JSON::PP::Boolean via $YAML::XS::Boolean [5]
- Fix Dump modifying inspected values [6]

[1]
Bug: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=861958
Bug: ingydotnet/yaml-libyaml-pm#45
Bug: ingydotnet/yaml-libyaml-pm#73
Bug: ingydotnet/yaml-libyaml-pm#74
[2]
Bug: ingydotnet/yaml-libyaml-pm#69
Bug: ingydotnet/yaml-libyaml-pm#70
[3]
Bug: ingydotnet/yaml-libyaml-pm#64
Bug: ingydotnet/yaml-libyaml-pm#71
[4]
Bug: ingydotnet/yaml-libyaml-pm#67
[5]
Bug: ingydotnet/yaml-libyaml-pm#66
[6]
Bug: ingydotnet/yaml-libyaml-pm#32
Bug: ingydotnet/yaml-libyaml-pm#55
Package-Manager: Portage-2.3.24, Repoman-2.3.6

madmartin added a commit to madmartin/gentoo that referenced this issue Apr 4, 2018

dev-perl/YAML-LibYAML: Bump to version 0.690.0
Upstream:
- (Security) Add $LoadBlessed option[1] to run on/off loading objects
- Fix regex roundtrip [2]
- Fix loading of many regex classes [3]
- Support tags !!str, !!map and !!seq [4]
- Support JSON::PP::Boolean via $YAML::XS::Boolean [5]
- Fix Dump modifying inspected values [6]

[1]
Bug: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=861958
Bug: ingydotnet/yaml-libyaml-pm#45
Bug: ingydotnet/yaml-libyaml-pm#73
Bug: ingydotnet/yaml-libyaml-pm#74
[2]
Bug: ingydotnet/yaml-libyaml-pm#69
Bug: ingydotnet/yaml-libyaml-pm#70
[3]
Bug: ingydotnet/yaml-libyaml-pm#64
Bug: ingydotnet/yaml-libyaml-pm#71
[4]
Bug: ingydotnet/yaml-libyaml-pm#67
[5]
Bug: ingydotnet/yaml-libyaml-pm#66
[6]
Bug: ingydotnet/yaml-libyaml-pm#32
Bug: ingydotnet/yaml-libyaml-pm#55
Package-Manager: Portage-2.3.24, Repoman-2.3.6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment