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

Type registration should be required #398

Closed
nahsra opened this Issue Mar 1, 2016 · 12 comments

Comments

Projects
None yet
6 participants
@nahsra

nahsra commented Mar 1, 2016

Type registration (class whitelisting) should be turned on by default. Otherwise, it's too simple for real-world users to be exploited by attacks like those discussed in the link below. These attacks abuse side effects resulting from constructors and finalize() in commonly available types.

This problem is compounded by adding custom deserializers, since they will probably invoke more methods on attacker-controlled objects, thus enabling new gadget chains.

I realize this will be a backwards-incompatible change.

https://www.contrastsecurity.com/security-influencers/serialization-must-die-act-1-kryo

@magro

This comment has been minimized.

Member

magro commented May 9, 2016

Because registering classes up front and setting Kryo.setRegistrationRequired(true) is considered good practice I also think we should make setRegistrationRequired(true) the default. We're heading towards a change in the minor version so this would be ok according to our compatibility policy.

@NathanSweet @romix WDYT?

@magro

This comment has been minimized.

Member

magro commented Jun 7, 2016

@NathanSweet @romix I'd vote for this, any objections from your side?

@romix

This comment has been minimized.

Contributor

romix commented Feb 1, 2017

I agree that typically if you care about performance (and this is usually why you use Kryo), you'd register classes. So, making setRegistrationRequired(true) the default makes sense.

My only concern is: How many existing clients would be broken by this change and would need to update their code either by adding setRegistrationRequired(false) or by registering classes? If would be nice to hear from people who would get affected by such a change.

@magro

This comment has been minimized.

Member

magro commented Feb 1, 2017

True, we could ask on the mailing list, gitter, twitter etc for feedback. We could also investigate how (with which default settings) major open source projects like spark use kryo.

If we decide against changing the default we could prominently recommend good practices like this.

@romix

This comment has been minimized.

Contributor

romix commented Feb 1, 2017

@magro BTW, if we ask on the mailing list, gitter, twitter ,etc for feedback, we may use this opportunity to ask multiple questions ;-) E.g. we may ask specifically:

  • what is the default setting for setRegistrationRequired that people use and if they are comfortable with changing it?
  • what are other settings that people would like to see set correctly by default to make it easier for them to use Kryo?
@magro

This comment has been minimized.

Member

magro commented Feb 1, 2017

Ok, I can ask on the mailing list, on gitter etc we can link to that thread then.

@magro

This comment has been minimized.

@boylemic

This comment has been minimized.

boylemic commented Jul 13, 2017

Can someone tell me if this has been resolved in the new version?
http://www.openwall.com/lists/oss-security/2016/03/01/16

@magro

This comment has been minimized.

Member

magro commented Jul 13, 2017

The status of this ticket is correct.

@NathanSweet

This comment has been minimized.

Member

NathanSweet commented Jul 16, 2017

Kryo users can always change from the defaults to the settings they want. I think it is smart that the default settings are secure, and we document potential security problems for each setting.

I've made this change in a local branch and this issue will be closed when that gets merged.

NathanSweet added a commit that referenced this issue Jul 18, 2017

@wsargent

This comment has been minimized.

wsargent commented Feb 22, 2018

This has also shown up in https://nvisium.com/resources/blog/2017/11/30/owasp-top-10-2007-2017-the-fall-of-csrf.html as well.

If setRegistrationRequired(true) is too much of a change, then it would be good to let people know about the issue by having a WARN message logged when Kryo initializes and setRegistrationRequired is set to false.

@NathanSweet

This comment has been minimized.

Member

NathanSweet commented Mar 7, 2018

setRegistrationRequired(true) is the default in the kryo-5.0.0-dev branch.

@NathanSweet NathanSweet closed this Jun 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment