Skip to content

Conversation

@shinrich
Copy link
Member

It appears that currently the major browsers do not set the max frame size in the settings, but it seems that we should take them up on larger frame sizes if they are willing to handle them.

@shinrich shinrich added this to the 10.0.0 milestone Aug 30, 2019
@shinrich shinrich self-assigned this Aug 30, 2019
@shinrich
Copy link
Member Author

[approve ci clang-analyzer]

@shinrich
Copy link
Member Author

[approve ci clang-format]

@masaori335
Copy link
Contributor

Nice catch. This is what we're missing. Does this improve performance when client set the setting?
Also, I'm wondering if we should use the value from the client naively. When the client says 2^24-1 (the maximum value), do we really allow to create 16MB data frame?

SETTINGS_MAX_FRAME_SIZE (0x5): Indicates the size of the largest
frame payload that the sender is willing to receive, in octets.

 The initial value is 2^14 (16,384) octets.  The value advertised
 by an endpoint MUST be between this initial value and the maximum
 allowed frame size (2^24-1 or 16,777,215 octets), inclusive.
 Values outside this range MUST be treated as a connection error
 (Section 5.4.1) of type PROTOCOL_ERROR.

@zwoop
Copy link
Contributor

zwoop commented Jan 15, 2020

@masaori335 @shinrich It seems your comments / concerns around unfiltered use of this settings was ignored? Are we ok with this-as is? Is there a DOS attack vector here ? If so, how would we address this, another records.config setting with a max-cap that we allow ?

@masaori335
Copy link
Contributor

masaori335 commented Jan 17, 2020

When malicious client set 16MB, ATS easily get a stack overflow. (proxy.config.thread.default.stacksize is 1MB ) We need a setting to control the buffer size, I think.
@shinrich Am I missing something?


UPDATE: The buffer I'm worrying about is in line 1485

uint8_t payload_buffer[buf_len];

Copy link
Contributor

@masaori335 masaori335 left a comment

Choose a reason for hiding this comment

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

I have a security concern commented above.

@zwoop zwoop added the OnDocs This is for PR currently running, or will run, on the Docs ATS server label Mar 11, 2020
@zwoop zwoop removed the OnDocs This is for PR currently running, or will run, on the Docs ATS server label Jun 9, 2020
@zwoop
Copy link
Contributor

zwoop commented Jun 9, 2020

Moving out to 9.1.x

@shinrich
Copy link
Member Author

shinrich commented Jun 9, 2020

Agreed if we take their value, we should have another max possible setting to limit how much we are willing to open things up.

@shinrich
Copy link
Member Author

Closing this for now. Sounds like we need to take a more nuanced approach

@shinrich shinrich closed this Jun 24, 2020
@shinrich shinrich removed this from the 10.0.0 milestone Jun 24, 2020
@maskit
Copy link
Member

maskit commented Oct 16, 2020

Since #6337 removed the concerning buffer, maybe we can do this now?

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants