Close Denial of Service issue with TCPConnection.expect #3197
Conversation
SeanTAllen
force-pushed
the
expect-max-size
branch
from
db66148 to
f18b221
Compare
packages/net/_test.pony
Outdated
| conn.expect(_expect)? | ||
| _send(conn, "hi there") | ||
| else | ||
| conn.close() |
There was a problem hiding this comment.
If hitting the else here is an unexpected condition, should we mark a failure of the test rather than just closing the connection?
Same question with the other tests that were changed in this PR.
There was a problem hiding this comment.
interesting. yes. we should. i was thinking about it from the perspective of "not a test".
packages/net/tcp_connection.pony
Outdated
| """ | ||
| A `received` call on the notifier must contain exactly `qty` bytes. If | ||
| `qty` is zero, the call can contain any amount of data. This has no effect | ||
| if called in the `sent` notifier callback. | ||
|
|
||
| `qty` can not exceed the max buffer size as indicated by the |
There was a problem hiding this comment.
This docstring change is useful, but it doesn't explicitly say what happens if qty does exceed the max buffer size - it just says that it "can not" but doesn't state what happens in that case. So I suggest this minor change, or something similar:
| `qty` can not exceed the max buffer size as indicated by the | |
| Errors if `qty` exceeds the max buffer size indicated by the |
|
@jemc test sounds good. |
Prior to this commit, `expect` could be used to create a denial of service on a Pony application. If you used expect to handle a framed protocol, then, you'd by default accept any sized `except` value which could result in huge amounts of memory being allocated. Memory that could vastly outstrip the max buffer size set for controlling memory usage. After this commit, `expect` is partial and will result in an error if attempting to set an expect value that is greater than our max read buffer size. I intended to commit this prior to another PR, but forgot and thought that I had. As it is, this PR works in conjunction with #3185 to prevent expect from being passed that is greater than the max buffer size thereby deadlocking the logic post #3185.
SeanTAllen
force-pushed
the
expect-max-size
branch
from
3975bd9 to
87d2e4e
Compare
|
Ok @jemc, I believe all comments are addressed. This needs to be squashed before merging. Add "DO NOT MERGE" for now. If you are good with everyone I will squash once it all passes CI. |
Co-Authored-By: Joe Eli McIlvain <[email protected]>
|
@SeanTAllen sorry for my late comment on this. Some protocols have an average request size that is small but can support a larger request size also. Currently, these protocols would always have to use the largest possible request size for their buffer size for the connection or they would be unable to handle the larger requests if/when they come in. Would it make sense to allow the buffer size to be changed for this scenario that would allow the application to temporarily grow to a larger buffer size for the larger requests prior to reverting to the smaller/usual sized buffer? This would allow an application to be able to handle both scenarios without always having to use the extra memory required for the larger buffer size all the time and still benefit from the Denial of Service protection because the application can then control if/when to grow the buffer to something larger than usual and have their own max allowed request size limit that they enforce. |
|
@dipinhora allowing the end user control over the buffer sizing is something that could be done. without a specific use case to address, i'm not sure exactly what would be exposed aside from "change buffer to this size". really at this point, you can do all of this by using expect == 0 and then handling any buffering etc as needed in a notify. this allows the end programmer to have complete control to take as much data as they want. you can receive packets of any size. this only controls what is possible if you are giving up some control to |
|
@SeanTAllen The kafka protocol is a good example. It is a framed protocol where usually the sizes are small but it is possible that someone might publish a large message (usually limited at the server/client via a config that defaults to 1 MB). As someone writing a kafka client in Pony, I would want to rely on the Yes, I agree, that if one needs |
|
I think the previous behavior was the worst of all worlds. If you want to handle 1 MB messages, I think its reasonable to have a 1 MB buffer. Or, I think it would be reasonable to change TCPConnection to take: "buffer size", "max buffer size". So buffer size is the normal allocation. And max buffer size is what it is allowed to grow to. This would only be of use when Without that distinction, I think it makes sense to cap everything at |
|
Agreed that the previous behavior was not great and I'm not asking for the previous behavior to be returned. While sizing the buffer at 1 mb for a max 1 mb message size is reasonable. That value is configurable in the kafka protocol and it could easily be set to be 10 mb or 100 mb by an overzealous user. I'm not sure always sizing the buffer to the largest message size would be reasonable if the largest message size is configured to be 100 mb. Your suggestion of |
|
I think that is a reasonable behavior and I would approve of such a PR. I might do it myself before the end of month when the next release comes out. |
Prior to this commit, `expect` could be used to create a denial of service on a Pony application. If you used expect to handle a framed protocol, then, you'd by default accept any sized `except` value which could result in huge amounts of memory being allocated. Memory that could vastly outstrip the max buffer size set for controlling memory usage. After this commit, `expect` is partial and will result in an error if attempting to set an expect value that is greater than our max read buffer size. I intended to commit this prior to another PR, but forgot and thought that I had. As it is, this PR works in conjunction with ponylang#3185 to prevent expect from being passed that is greater than the max buffer size thereby deadlocking the logic post ponylang#3185.
Prior to this commit,
expectcould be used to create a denial ofservice on a Pony application. If you used expect to handle a framed
protocol, then, you'd by default accept any sized
exceptvalue whichcould result in huge amounts of memory being allocated. Memory that
could vastly outstrip the max buffer size set for controlling memory
usage.
After this commit,
expectis partial and will result in an error ifattempting to set an expect value that is greater than our max read
buffer size.
I intended to commit this prior to another PR, but forgot and thought
that I had. As it is, this PR works in conjunction with #3185 to prevent
expect from being passed that is greater than the max buffer size
thereby deadlocking the logic post #3185.