[postgres_proxy] assert failure with untrusted buffer when onData()

[jianwen612]

I'm currently working on fuzz test(which generates random bytes for onData() and onWrite() to see whether we could crash the Envoy) for network-level filters.
When I was testing on postres_proxy filter with some untrusted data, an assert failure occurred inside linearize:
RELEASE_ASSERT(size <= length(), "Linearize size exceeds buffer size");
https://github.com/envoyproxy/envoy/blob/master/source/extensions/filters/network/postgres_proxy/postgres_decoder.cc#L201

This error only happens in fuzzer or when upstream server is on bad state, so it is not security-critical now.
But I think that we could deal with this error more gracefully. (So that we could make the filter more robust to upstream errors, and enable the fuzzer to continue testing it).

My idea is that we could make it just like other invalid error handles in this file, which is return false;, before calling linearize?
This solution looks like this(from line 200):

auto bytesToRead = length - 4;
if(bytesToRead>data.length()){
  return false;
}
message.assign(std::string(static_cast<char*>(data.linearize(bytesToRead)), bytesToRead));

This issue can be reproduced in unit test by adding a case as below(test/extensions/filters/network/postgres_proxy/postgres_decoder_test.cc):

TEST_P(PostgresProxyFrontendEncrDecoderTest, AssertFailure) {
  std::string str_data;
  for(int i=0;i<8;i++){
    str_data.push_back('\0');
  }
  Buffer::OwnedImpl data(str_data);
  decoder_->onData(data, false);
}

If anyone has a better idea, please share with me or make a pull request and link it here. Thanks!
/cc @dio
/cc @fabriziomello
/cc @cpakulski

[fabriziomello]

Hi @jianwen612, thanks for testing it... I totally agree with return false approach to handle invalid states.

And just 5 bytes is enough to throw exception:

TEST_P(PostgresProxyFrontendEncrDecoderTest, AssertFailure) {
  std::string str_data;
  for(int i=0;i<5;i++){
    str_data.push_back('\0');
  }
  Buffer::OwnedImpl data(str_data);
  decoder_->onData(data, false);
}

LGTM

[cpakulski]

I agree that adding return false will help with this one particular test case, but unfortunately is not enough from filter's logic point of view. The reason is that messages pass through the filter one after another. The previous message must be processed successfully in order to start processing the next one. If there is something wrong with one message, for example length indicated in a message header does not reflect the real message's length, then the whole processing goes out of sync and the filter will not be able to find where one message ends and the next one begins. Keeping processing messages will lead to OOM and crash. The current implementation of the filter does not really have a state. It should accept only certain messages in certain states and if it goes out of sync, it should stop interpreting messages or maybe even reset the connection.

@jianwen612 Thanks for testing it. Making this filter to pass fuzz testing requires more changes. I will address this issue in few weeks, as I will be away.

[jianwen612]

Hi @jianwen612, thanks for testing it... I totally agree with return false approach to handle invalid states.

And just 5 bytes is enough to throw exception:

TEST_P(PostgresProxyFrontendEncrDecoderTest, AssertFailure) {
  std::string str_data;
  for(int i=0;i<5;i++){
    str_data.push_back('\0');
  }
  Buffer::OwnedImpl data(str_data);
  decoder_->onData(data, false);
}

LGTM

Thanks for pointing this out!

[jianwen612]

I agree that adding return false will help with this one particular test case, but unfortunately is not enough from filter's logic point of view. The reason is that messages pass through the filter one after another. The previous message must be processed successfully in order to start processing the next one. If there is something wrong with one message, for example length indicated in a message header does not reflect the real message's length, then the whole processing goes out of sync and the filter will not be able to find where one message ends and the next one begins. Keeping processing messages will lead to OOM and crash. The current implementation of the filter does not really have a state. It should accept only certain messages in certain states and if it goes out of sync, it should stop interpreting messages or maybe even reset the connection.

@jianwen612 Thanks for testing it. Making this filter to pass fuzz testing requires more changes. I will address this issue in few weeks, as I will be away.

Thank you for sharing this! Okey, then I will temporarily disable the fuzzing for this filter.

[cpakulski]

/assign @cpakulski

[stale]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or other activity occurs. Thank you for your contributions.

[cpakulski]

WIP.

[stale]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or other activity occurs. Thank you for your contributions.

[cpakulski]

WIP.

[github-actions]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

[rostams-lyft]

Is this still a work-in-progress?

[cpakulski]

@rostams-lyft Yes, I am still working on it.

[cpakulski]

@jianwen612 I opened a PR #16575 to fix this issue. Can you help me verify that fuzz tests do not crash the postgres filter? Thanks!

[jianwen612]

@jianwen612 I opened a PR #16575 to fix this issue. Can you help me verify that fuzz tests do not crash the postgres filter? Thanks!

Sorry... I don't have the environment to test it now. I've moved to another team.