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 Client protocol frame size limit #15396

Merged
merged 1 commit into from Oct 1, 2019

Conversation

@kwart
Copy link
Contributor

kwart commented Jul 30, 2019

This PR adds a level of memory protection to the Hazelcast client protocol.
Untrusted connections (the ones which haven't finish authentication yet) will not accept fragmented messages and they will check frame size against a configurable limit.

EE: hazelcast/hazelcast-enterprise#3100

@kwart kwart self-assigned this Jul 30, 2019
@kwart kwart added this to the 4.0 milestone Jul 30, 2019
@kwart kwart marked this pull request as ready for review Jul 30, 2019
@kwart kwart force-pushed the kwart:client-protocol-frame-limit branch from 04209b8 to 4509e44 Sep 25, 2019
@kwart kwart requested a review from hazelcast/clients as a code owner Sep 25, 2019
@kwart kwart requested review from pveentjer and tkountis Sep 25, 2019
@kwart kwart force-pushed the kwart:client-protocol-frame-limit branch 4 times, most recently from a24e17d to 58fee7d Sep 25, 2019
* frame).
*/
public static final HazelcastProperty CLIENT_PROTOCOL_UNVERIFIED_FRAME_LENGTH =
new HazelcastProperty("hazelcast.client.protocol.max.frame.length", 1024);

This comment has been minimized.

Copy link
@tkountis

tkountis Sep 26, 2019

Contributor

Maybe include the word bytes in the prop name? We tend to do that with sizes/lengths on properties, easier to know what to put as a value without looking it up every time.

Copy link
Contributor

tkountis left a comment

I believe there is a range check error which can allow a special crafted packet to pass your checks and still crash the endpoint.

For instance a frame with size, Integer.MIN_VALUE can pass your checks for max size per frame, but then cause trouble at the following line: https://github.com/hazelcast/hazelcast/pull/15396/files#diff-777042f2e7bb74fb094f22df0d18eb84L72 which subtracts the SZ_OF_FRAME_LEN from the actual frame length, and will result in a value close to Integer.MAX_VALUE. Upon allocating that byte-array, we can end up with OOME.

I didn't attempt to reproduce it, but its worth testing it.

@kwart

This comment has been minimized.

Copy link
Contributor Author

kwart commented Sep 26, 2019

Thanks for reviewing, Thomas. I'm going to cover the comments now.

@kwart

This comment has been minimized.

Copy link
Contributor Author

kwart commented Sep 27, 2019

run-lab-run

@kwart kwart requested a review from ihsandemir Sep 27, 2019
@mmedenjak mmedenjak added Team: Core and removed Team: Core labels Sep 30, 2019
@kwart kwart requested a review from ihsandemir Oct 1, 2019
@kwart kwart force-pushed the kwart:client-protocol-frame-limit branch from 95b9f41 to 63e1758 Oct 1, 2019
@kwart kwart merged commit 9b1c2ce into hazelcast:master Oct 1, 2019
1 check passed
1 check passed
default Test PASSed.
Details
@kwart kwart deleted the kwart:client-protocol-frame-limit branch Oct 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.