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

Add a decoder option to limit the number of unmarshalled values #375

Closed
wants to merge 2 commits into from

Conversation

simonferquel
Copy link

This PR gives user code the opportunity to limit the number of values unmarshalled while decoding a yaml payload.

This is very usefull to protect programs from malicious external yaml documents such as these:

version: "3" 
services: &services ["lol","lol","lol","lol","lol","lol","lol","lol","lol"] 
b: &b [*services,*services,*services,*services,*services,*services,*services,*services,*services] 
c: &c [*b,*b,*b,*b,*b,*b,*b,*b,*b] 
d: &d [*c,*c,*c,*c,*c,*c,*c,*c,*c] 
e: &e [*d,*d,*d,*d,*d,*d,*d,*d,*d] 
f: &f [*e,*e,*e,*e,*e,*e,*e,*e,*e] 
g: &g [*f,*f,*f,*f,*f,*f,*f,*f,*f] 
h: &h [*g,*g,*g,*g,*g,*g,*g,*g,*g] 
i: &i [*h,*h,*h,*h,*h,*h,*h,*h,*h]

This is a valid document, but parsing this will crash the program with an out of memory panic at some point.

Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
decode_test.go Outdated
d = yaml.NewDecoder(bytes.NewBuffer([]byte(ordinalCase)), yaml.WithLimitDecodedValuesCount(3))
// decoded values are [Hello, World, [Hello:World pair]]
err = d.Decode(&v)
c.Assert(err, Equals, nil)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

c.Assert(err, IsNil)

yaml.go Outdated
@@ -89,26 +89,45 @@ func UnmarshalStrict(in []byte, out interface{}) (err error) {
return unmarshal(in, out, true)
}

// DecoderOption is an option to apply to modyfy a decoder's behavior

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: s/modyfy/modify

Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
@simonferquel
Copy link
Author

@silvin-lubecki fixed, thanks :)

@thaJeztah
Copy link

ping @rogpeppe @niemeyer PTAL

@tedyu
Copy link

tedyu commented Sep 30, 2019

@rogpeppe @niemeyer
Can you review this ?

The fix would help mitigate the CVE linked above.

@niemeyer
Copy link
Contributor

The issue has been addressed in v3 without additional API changes. The proper fix for v2 would be to backport that over. Closing this one on that basis.

@niemeyer niemeyer closed this Sep 30, 2019
@thaJeztah
Copy link

@niemeyer do you know which PR fixed it in v3? Happy to open a backport/cherry-pick (if possible)

@niemeyer
Copy link
Contributor

@thaJeztah Thanks! That was addressed with caeefd8

@niemeyer
Copy link
Contributor

It's been backported to v2 with bb4e33b.

@omeid
Copy link

omeid commented Oct 18, 2019

You literally copied his contribution without attribution. While it would have been best if the commit was cherry-picked, but the least @simonferquel should get credited for this fix.

@niemeyer
Copy link
Contributor

His proposal:

https://github.com/go-yaml/yaml/pull/375/files

What was committed:

caeefd8

Enjoy your day.

@simonferquel
Copy link
Author

Don't fight over me and this PR please, I don't care if I am mentioned in anything. This commit just allowed to workaround the issue for a project I had, and I made a PR in case it could help others.

@thaJeztah
Copy link

@omeid the implementation that was merged is different; this PR suggested a configurable limit, whereas the code merged has a hard coded limit.

The test case looks similar, because both are based on the same YAML exploit (the example can be found on WikiPedia: https://en.m.wikipedia.org/wiki/Billion_laughs_attack)

That said, it would've been nice if this PR had gotten attention earlier.

@niemeyer
Copy link
Contributor

@simonferquel You are right, and your help was appreciated. Even if we got something else in, you gave a plave for people to organize around the issue and think about it. Thank you.

@niemeyer
Copy link
Contributor

niemeyer commented Oct 18, 2019

@thaJeztah Yeah, it's internally handled, but it's not quite hardcoded. We consider the percentage of aliases in the overall document. And we also look at the depth since that's where the issue happens. You are also right that this should have been looked into before.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants