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

The node may have a bug when dealing with unformatted packet and lead to a crash #1951

Closed
fCorleone opened this issue Jun 15, 2021 · 2 comments
Assignees
Labels
Projects

Comments

@fCorleone
Copy link

Describe the bug
A malicious node can send a packet continuously. The packet is in an incorrect format and cannot be decoded by the node correctly. As a result, the node may consume the memory sustainably, as the flowing figure shows:
Figure_1
After 100 seconds, over 4000 MB memory has been consumed. If I continue sending the packet, the node will consume all the memory. At last it be killed by the OS.

In order to analyze the reason for this bug, I try to debug the code of the node. Here is what I found:
First, I found that in the file libp2p/P2PMessageRC2.cpp, at line 109 in the function decode:

ssize_t P2PMessageRC2::decode(const byte* buffer, size_t size)
{
    ...
    m_length = ntohl(*((uint32_t*)&buffer[offset]));
    if (size < m_length)    {        
       return dev::network::PACKET_INCOMPLETE;    
    }
    ...
}

the variable size is 72 and the variable m_length is a very big number under my packet. So the function will return dev::network::PACKET_INCOMPLETE whose value is 0.
The variable which accepts the return value is result in libnetwork/Session.cpp at line 421 in the function doRead:

ssize_t result = message->decode(s->m_data.data(), s->m_data.size());

and the program will enter into a if-else cluse:

if (result > 0){
    ...
}
else if (result == 0)  {                        
    s->doRead();                        
    break;                    
}
else {
    ...
}

Because the value of result is 0, so here the program will call the function doRead recursively. If I delete this call, the problem will not occur anymore.

else if (result == 0)  {                        
    // s->doRead();                        
    break;                    
}

So I think the reason maybe the developers forget to release certain memory before the return statement if the packet is not decoded correctly!

To Reproduce
Steps to reproduce the behavior:

  1. Construct a P2P packet which claims to have a big length (set a big value for variable m_length)
  2. Continuously send the packet to a running node
  3. The node will consume the memory continuously and crash.

Expected behavior
By handling the abnormal packets correctly, the memory cost will not sustainably increase and the node will not crash.

Screenshots
I have give the screenshots of the memory usage of the node in the description part.

Environment (please complete the following information):

  • OS: Ubuntu 16.04
  • FISCO BCOS Version: v2.7.2

Additional context
None!

@cyjseagull
Copy link
Contributor

We will follow up on this issue and resolve it in 2.8.0.

@stale
Copy link

stale bot commented Jan 8, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Jan 8, 2022
@cyjseagull cyjseagull added this to Done in FISCO BCOS Feb 15, 2022
@stale stale bot closed this as completed Mar 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

No branches or pull requests

2 participants