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

OpenSslSession#initPeerCerts creates too long almost empty arrays #5945

Closed
ichaki5748 opened this issue Oct 26, 2016 · 9 comments
Closed

OpenSslSession#initPeerCerts creates too long almost empty arrays #5945

ichaki5748 opened this issue Oct 26, 2016 · 9 comments
Assignees
Labels
Milestone

Comments

@ichaki5748
Copy link
Contributor

netty 4.1.6

io.netty.handler.ssl.ReferenceCountedOpenSslEngine.OpenSslSession#initPeerCerts

final byte[] clientCert;
...
int len = clientCert.length + 1; // <------ BUG: raw certificate byte array size is used
peerCerts = new Certificate[len];
x509PeerCerts = new X509Certificate[len];

But should probably be

int len = chain.length + 1; // <------ so we have first entry as client cert + chain.length

@normanmaurer
Copy link
Member

Maybe want to provide a PR with a fix?

Am 26.10.2016 um 21:25 schrieb ichaki5748 notifications@github.com:

netty 4.1.6

io.netty.handler.ssl.ReferenceCountedOpenSslEngine.OpenSslSession#initPeerCerts

final byte[] clientCert;
...
int len = clientCert.length + 1; // <------ BUG: raw certificate byte array size is used
peerCerts = new Certificate[len];
x509PeerCerts = new X509Certificate[len];

But should probably be

int len = chain.length + 1; // <------ so we have first entry as client cert + chain.length


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

@ichaki5748
Copy link
Contributor Author

Yes, sure, will try to do it this week

@normanmaurer
Copy link
Member

Thx

Am 26.10.2016 um 23:54 schrieb ichaki5748 notifications@github.com:

Yes, sure, will try to do it this week


You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

@ichaki5748
Copy link
Contributor Author

ichaki5748 commented Oct 27, 2016

Also io.netty.handler.ssl.OpenSslJavaxX509Certificate.*
could theoretically blow in concurrent usage because of lazy init in io.netty.handler.ssl.OpenSslJavaxX509Certificate#unwrap because
io.netty.handler.ssl.OpenSslJavaxX509Certificate#wrapped is not volatile (certificate would not be fully initialized)?

@normanmaurer
Copy link
Member

@iainmcgin please open separate PRs for the separate issues and ping me once you are ready. I will review then

@iainmcgin
Copy link
Contributor

You probably meant to reference @ichaki5748

@normanmaurer
Copy link
Member

@iainmcgin ups yes... sorry I need more ☕️

ichaki5748 added a commit to ichaki5748/netty that referenced this issue Oct 31, 2016
Motivation:

netty#5945

Modifications:

Refactored initialization of arrays. Fixed arrays length

Result:

Cert arrays have proper length. Testing added
ichaki5748 added a commit to ichaki5748/netty that referenced this issue Oct 31, 2016
Motivation:

netty#5945 (comment)

Modifications:

Added volatile for correct initialization of actual certificates: netty#5945 (comment)

Result:

Concurrency issue fixed
@ichaki5748
Copy link
Contributor Author

@normanmaurer Hi, please see #5967 #5966

ichaki5748 added a commit to ichaki5748/netty that referenced this issue Nov 3, 2016
Motivation:

netty#5945

Modifications:

Refactored initialization of arrays. Fixed arrays length

Result:

Cert arrays have proper length. Testing added
normanmaurer pushed a commit that referenced this issue Nov 3, 2016
Motivation:

#5945

Modifications:

Refactored initialization of arrays. Fixed arrays length

Result:

Cert arrays have proper length. Testing added
normanmaurer pushed a commit that referenced this issue Nov 3, 2016
Motivation:

#5945

Modifications:

Refactored initialization of arrays. Fixed arrays length

Result:

Cert arrays have proper length. Testing added
@normanmaurer
Copy link
Member

Fixed by #5967

@normanmaurer normanmaurer self-assigned this Nov 3, 2016
@normanmaurer normanmaurer added this to the 4.0.43.Final milestone Nov 3, 2016
liuzhengyang pushed a commit to liuzhengyang/netty that referenced this issue Sep 10, 2017
Motivation:

netty#5945

Modifications:

Refactored initialization of arrays. Fixed arrays length

Result:

Cert arrays have proper length. Testing added
pulllock pushed a commit to pulllock/netty that referenced this issue Oct 19, 2023
Motivation:

netty#5945

Modifications:

Refactored initialization of arrays. Fixed arrays length

Result:

Cert arrays have proper length. Testing added
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants