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

Update proxy-protocol.cc #9320

Merged
merged 1 commit into from Jul 16, 2020
Merged

Update proxy-protocol.cc #9320

merged 1 commit into from Jul 16, 2020

Conversation

ihsinme
Copy link
Contributor

@ihsinme ihsinme commented Jul 10, 2020

get rid of integer overflow

Short description

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)
  • checked that this code was merged to master

get rid of integer overflow
@@ -65,10 +65,10 @@ std::string makeProxyHeader(bool tcp, const ComboAddress& source, const ComboAdd

size_t valuesSize = 0;
for (const auto& value : values) {
if (value.content.size() > std::numeric_limits<uint16_t>::max()) {
valuesSize += sizeof(uint8_t) + sizeof(uint8_t) * 2 + value.content.size();
if (valuesSize > std::numeric_limits<uint16_t>::max()) {
Copy link
Member

@rgacogne rgacogne Jul 10, 2020

Choose a reason for hiding this comment

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

Just a small comment, changing that check means that trying to add a single value larger than 65k will trigger the same error message than the total length of several values exceeding 65k. Perhaps we could have both checks, like in https://github.com/PowerDNS/pdns/pull/9319/files, so we have different error messages?

Copy link
Contributor Author

@ihsinme ihsinme Jul 10, 2020

Choose a reason for hiding this comment

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

I apologize for the short comment.

I propose to consider from the perspective that a change in this place will provide protection against integer overflow in the addProxyProtocol function as well, since a limited value will come there.

Copy link
Member

@rgacogne rgacogne Jul 10, 2020

Choose a reason for hiding this comment

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

I agree, but do you also agree that with your proposed fix, trying to add ONE value larger than 2¹⁶ bytes will trigger the same error message than adding for example two values of (2¹⁶/2)+1 bytes?

Copy link
Contributor Author

@ihsinme ihsinme Jul 10, 2020

Choose a reason for hiding this comment

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

i agree, but "adding for example two values of (2¹⁶ / 2) +1 bytes" will cause an error while processing the second value and display its contents in the error message "of size" + std :: to_string (value.content.size ())); "
but taking into account the check "if (total> std :: numeric_limits <uint16_t> :: max ())", the logic of the function seems to me, including to prevent data overstating in the total value.

Copy link
Contributor Author

@ihsinme ihsinme Jul 10, 2020

Choose a reason for hiding this comment

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

could you tell me why "Travis CI - Pull Request Failing after 45m - Build Failed" and what can I do to fix this

Copy link
Member

@rgacogne rgacogne Jul 13, 2020

Choose a reason for hiding this comment

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

i agree, but "adding for example two values of (2¹⁶ / 2) +1 bytes" will cause an error while processing the second value and display its contents in the error message "of size" + std :: to_string (value.content.size ())); "
but taking into account the check "if (total> std :: numeric_limits <uint16_t> :: max ())", the logic of the function seems to me, including to prevent data overstating in the total value.

Right, I'm not saying having one check will not handle all the issues, I'm just trying to have a more specific message displayed to the user that will make it possible to them to know if they are trying to add a too large value, or if it's the sum of values that is too large.

could you tell me why "Travis CI - Pull Request Failing after 45m - Build Failed" and what can I do to fix this

This was a transient failure, it's gone now.

Copy link
Contributor Author

@ihsinme ihsinme Jul 13, 2020

Choose a reason for hiding this comment

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

Thanks for the clarifications.
In this case, I can suggest shortening the error text to the following.
throw std :: runtime_error ("The size of proxy protocol values is limited to" + std :: to_string (std :: numeric_limits <uint16_t> :: max ()));
I think it will be correct and quite informative.

Copy link
Contributor Author

@ihsinme ihsinme Jul 13, 2020

Choose a reason for hiding this comment

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

Although, if you want to inform the user in such detail, part of the changes in your request pool is ideal https://github.com/PowerDNS/pdns/pull/9319/files#diff-22c24a7a10de6e3fd3f14125d553f772.
But I am very interested in my own successful PR, so I ask you to suggest how to do this correctly.

Copy link
Member

@rgacogne rgacogne Jul 16, 2020

Choose a reason for hiding this comment

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

I'll merge this pull request to fix the possible overflow, and I'll make a second one to improve the user experience later.

Copy link
Member

@rgacogne rgacogne left a comment

Thanks a lot!

@rgacogne rgacogne merged commit c22723c into PowerDNS:master Jul 16, 2020
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants