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

Fix stack data disclosure when returning static files smaller than 16KiB #523

Merged
merged 2 commits into from Aug 22, 2022

Conversation

mrozigor
Copy link
Member

@mrozigor mrozigor commented Aug 21, 2022

It could also cause to return partially incorrect file ending in
case of file not being rounded up to 16KiB.

Thanks to Gynvael Coldwind and hebi for discovering and preparing
report.

@luca-schlecker
Copy link
Collaborator

Is there a reason that the buffers vector is inside the loop?

@mrozigor
Copy link
Member Author

It was already implemented like this. It looks like asio::write expects vector of buffers. Also it looks like asio::buffer doesn't own memory (https://think-async.com/Asio/asio-1.22.1/doc/asio/reference/buffer.html#asio.reference.buffer.buffer_invalidation), so we have to:

luca-schlecker
luca-schlecker previously approved these changes Aug 22, 2022
Copy link
Collaborator

@luca-schlecker luca-schlecker left a comment

Choose a reason for hiding this comment

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

I see, the current solution works fine, no need to add extra copy-operations.

LGTM 👍

@mrozigor
Copy link
Member Author

I'll leave it till tomorrow and then merge. Thanks!

@MichaelSB
Copy link
Contributor

The vector with size 1 could be constructed outside the loop. And then the asio::buffer would be assigned to the first element in the vector inside the loop. Imo. No copy needed and we would save construction/destruction of a vector in a loop (probably alloc/dealloc)

@mrozigor
Copy link
Member Author

Good point. I didn't want to change anything not connected directly with error, but maybe postponing it is a worse idea :P

@crow-clang-format
Copy link

--- include/crow/http_connection.h	(before formatting)
+++ include/crow/http_connection.h	(after formatting)
@@ -389,7 +389,7 @@
             if (res.file_info.statResult == 0)
             {
                 std::ifstream is(res.file_info.path.c_str(), std::ios::in | std::ios::binary);
-                std::vector<asio::const_buffer> buffers {1};
+                std::vector<asio::const_buffer> buffers{1};
                 char buf[16384];
                 is.read(buf, sizeof(buf));
                 while (is.gcount() > 0)

16KiB. It could also cause to return partially incorrect file ending in
case of file not being rounded up to 16KiB.

Thanks to Gynvael Coldwind and hebi for discovering and preparing
report.
@The-EDev The-EDev merged commit c0bfaa7 into master Aug 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants