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.

Sign up for GitHub

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

Jump to bottom

TLS: Memory corruption #1217

Closed
avbelov23 opened this issue Mar 21, 2019 · 6 comments
Closed

TLS: Memory corruption #1217

avbelov23 opened this issue Mar 21, 2019 · 6 comments
Assignees
@avbelov23
Labels
bug crucial
Milestone
0.7 - HTTP/2

Comments

@avbelov23
Copy link
Contributor

avbelov23 commented Mar 21, 2019 •
edited
Loading

Tempesta config:

server 127.0.0.1:80; #nginx
listen 443 proto=https;
tls_certificate /home/avb/Projects/tempesta/tfw-root.crt;
tls_certificate_key /home/avb/Projects/tempesta/tfw-root.key;

Log sh:

$ cat /var/www/html/index.html 
<!DOCTYPE html>
<html>
<body>
Test
</body>
</html>
$ curl -k https://localhost
<!DOCTYPE html>
<html>
<body>
Test
</body>
</html>
$ cat /var/www/html/index.html 
�\>�(���e�����m�#����O̬�����r��(���t�����Zl���7�>+ɩ�����4�k�!�/$QS��N���
$ echo 3 | sudo tee /proc/sys/vm/drop_caches
3
$ cat /var/www/html/index.html 
<!DOCTYPE html>
<html>
<body>
Test
</body>
</html>
@avbelov23
Copy link
Contributor Author

Blame sendfile on;

@avbelov23
Copy link
Contributor Author

It turns out that thanks to the in-place implementation of TLS, we encrypt data pages without allocating additional memory and copying, but because sendfile links the socket with the file cache, we overwrite the file cache.

If several clients request the same file, we will overwrite it in different places at once and each client will receive a unique file.

Then the problem can be reproduced not only with the help of sendfile. If the file is given from the cache, then the result will be the same. When sending from our cache, we reuse the pages of memory from the cache.

@krizhanovsky krizhanovsky mentioned this issue Mar 22, 2019
Closed
17 tasks
@krizhanovsky
Copy link
Contributor

@avb good catch - this the fundamental problem of out TLS encryption!

The fix should be straightforward enough - we need to copy skb data before encryption if an skb is formed from splice() (sendfile() for file cache or splice() for user-space buffer, but the core is the same) or our web cache. We do something similar in ss_send(). It's easy to do for our cache, but I'm not sure whether we know the nature of skb data in tcp_write_xmit() - probably some kernel patching is required. However, maybe SKBTX_SHARED_FRAG flag will help us, see do_tcp_sendpages().

@vankoven
Copy link
Contributor

Also note, that only payload part of the response served from cache reuses pages. The headers are created from scratch for every new response. Same can happen to sendfile() responses. But in the same time backend server may have its own cache and use zero-copy TCP transmissions. So it's can be very tricky.

Probably the copying of skb data should be configurable option:

  • avoid - only for necessary cases, such as serving from splice()/webcache
  • always - always create copy of the skb for the encrypted data.

@krizhanovsky
Copy link
Contributor

We have #634 to reuse HTTP response headers kept in cache. For current kernel there is only one zero-copy transmission mechanism - splice, which is used by both sendfile() and vmsplice(). However, newer kernels introduce MSG_ZEROCOPY, so the final problem solution must consider MSG_ZEROCOPY and at least contain TODO comment what we need to do to move to the newer kernels.

@avbelov23 avbelov23 mentioned this issue May 29, 2019
Merged
@krizhanovsky
Copy link
Contributor

Please also don’t forget to backport the fix to 0.6.

@krizhanovsky krizhanovsky mentioned this issue Jul 1, 2019
Closed
This was referenced Jul 1, 2019
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@avbelov23 avbelov23

Labels
bug crucial
Projects
None yet
Milestone
0.7 - HTTP/2
Development

No branches or pull requests
3 participants
@krizhanovsky @vankoven @avbelov23