postgres: validate message syntax before parsing #16575
Conversation
and postgres message body.. Signed-off-by: Christoph Pakulski <[email protected]>
Signed-off-by: Christoph Pakulski <[email protected]>
…alidator. Signed-off-by: Christoph Pakulski <[email protected]>
Signed-off-by: Christoph Pakulski <[email protected]>
Signed-off-by: Christoph Pakulski <[email protected]>
Signed-off-by: Christoph Pakulski <[email protected]>
Signed-off-by: Christoph Pakulski <[email protected]>
Signed-off-by: Christoph Pakulski <[email protected]>
Signed-off-by: Christoph Pakulski <[email protected]>
Signed-off-by: Christoph Pakulski <[email protected]>
| std::string message_; | ||
| uint32_t message_len_{}; | ||
|
|
||
| static const std::string FRONTEND; |
There was a problem hiding this comment.
static const string (non-POD) is source of flaky test, use singleton (CONSTRUCT_ON_FIRST_USE) or constexpr string_view.
There was a problem hiding this comment.
Converted to constexpr string_view. Thanks!
…inmutable strings used for logging. Signed-off-by: Christoph Pakulski <[email protected]>
Signed-off-by: Christoph Pakulski <[email protected]>
|
/retest |
|
Retrying Azure Pipelines: |
|
cc @fabriziomello could have a look as an extension owner? |
| message_len_ = data.peekBEInt<uint32_t>(0); | ||
| // MAX_STARTUP_PACKET_LENGTH is defined in Postgres source code | ||
| // as maximum size of initial packet. | ||
| #define MAX_STARTUP_PACKET_LENGTH 10000 |
There was a problem hiding this comment.
Perhaps move it to postgres_decoder.h and add the following link to comments: https://github.com/postgres/postgres/search?q=MAX_STARTUP_PACKET_LENGTH&type=code
|
|
||
| // The minimum size of the message sufficient for parsing is 5 bytes. | ||
| if (data.length() < 5) { | ||
| if (data.length() < 4) { |
There was a problem hiding this comment.
What about create something like #define MIN_MESSAGE_LENGTH 4 ? And seems the comment above is not in sync what code does...
| encrypted_ = true; | ||
| // Handler for SSLRequest (Int32(80877103) = 0x04d2162f) | ||
| // See details in https://www.postgresql.org/docs/current/protocol-message-formats.html. | ||
| if (code == 0x04d2162f) { |
There was a problem hiding this comment.
Both lines 249 and 253 use SSLRequest codes... for code legibility what about add some macros?
There was a problem hiding this comment.
IMHO macro will make it more difficult to read and will not add any value. The meaning of this value is documented pretty clearly here.
| data.drain(data.length()); | ||
| return encrypted_ ? Decoder::ReadyForNext : Decoder::Stopped; | ||
| Decoder::Result result = Decoder::Result::ReadyForNext; | ||
| uint32_t code = data.peekBEInt<uint32_t>(4); |
There was a problem hiding this comment.
Rename code to startup_packet just to use the Postgres vocabulary.
There was a problem hiding this comment.
Yeah, but this is not really a packet itself, but just 4-bytes code which can be either a version or encryption type.
source/extensions/filters/network/postgres_proxy/postgres_decoder.cc
Outdated
Show resolved
Hide resolved
Thanks for remember me ... did some minor comments. Me and @cpakulski already discussed a bit about this in PVT before he send the PR. Mostly about |
Signed-off-by: Christoph Pakulski <[email protected]>
Signed-off-by: Christoph Pakulski <[email protected]>
|
@cpakulski the Postgres Regression using your last commit 60b5585 worked but seems our current pipeline doesn't like it so much. |
Signed-off-by: Christoph Pakulski <[email protected]>
|
@fabriziomello Would you mind running Postgres Regression test on the latest commit? Thanks. |
I did it when I saw your new commit and it ran without any issue: https://github.com/fabriziomello/envoy-postgres-regression/actions/runs/940000625 |
|
/LGTM |
|
@lizan How can we move this PR forward? All regression tests have passed. |
Commit Message: Validate postgres messages before parsing. Additional Description: Introduced InSync and OutOfSync states in decoder. When decoder detects a wrongly formatted message, it stops parsing and moves to OutOfSync state. Continuing parsing after encountering message with wrong syntax may lead to interpreting random bytes as length of the message and possibly causing OOM. Risk Level: Low Testing: Added unit tests and run full regression tests. Docs Changes: No. Release Notes: No. Platform Specific Features: Fixes envoyproxy#12340 Signed-off-by: Christoph Pakulski <[email protected]>
Commit Message:
Validate postgres messages before parsing.
Additional Description:
Introduced InSync and OutOfSync states in decoder. When decoder detects a wrongly formatted message, it stops parsing and moves to OutOfSync state. Continuing parsing after encountering message with wrong syntax may lead to interpreting random bytes as length of the message and possibly causing OOM.
Risk Level: Low
Testing: Added unit tests and run full regression tests.
Docs Changes: No.
Release Notes: No.
Platform Specific Features:
Fixes #12340