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

[FIXED] #2525 #2526

Merged
merged 1 commit into from Sep 14, 2021
Merged

[FIXED] #2525 #2526

merged 1 commit into from Sep 14, 2021

Conversation

derekcollison
Copy link
Member

We use u16 to encode header len when replicating JetStream messages.
Make sure to error if we exceed that limit.

Signed-off-by: Derek Collison derek@nats.io

Resolves #2525

/cc @nats-io/core

Make sure to error if we exceed that limit.

Signed-off-by: Derek Collison <derek@nats.io>
// Since we encode header len as u16 make sure we do not exceed.
// Again this works if it goes through but better to be pre-emptive.
if len(hdr) > math.MaxUint16 {
err := fmt.Errorf("JetStream header size exceeds limits for '%s > %s'", jsa.acc().Name, mset.cfg.Name)
Copy link
Contributor

@ripienaar ripienaar Sep 14, 2021

Choose a reason for hiding this comment

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

on the assumption someone might be trying to trigger overflows or memory exchaustion attacks should we maybe log the IP of the client or some info about him?

Otherwise LGTM

Copy link
Member Author

Choose a reason for hiding this comment

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

If the message violates total payload limits or control line limits the error is logged and the client cutoff.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but I am mainly interested in having a way to know who is the bad client - i guess events will be raised so its probably fine. LGTM..

Copy link
Contributor

@ripienaar ripienaar left a comment

Choose a reason for hiding this comment

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

LGTM

@derekcollison derekcollison merged commit 20574ff into main Sep 14, 2021
@derekcollison derekcollison deleted the issue-2525 branch September 14, 2021 13:41
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

LGTM

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.

Report of large message headers causing JetStream issues.
3 participants