Skip to content

Fixes for RF24Gateway - #58

Merged
TMRh20 merged 2 commits into
masterfrom
memFixes
May 29, 2026
Merged

TMRh20 merged 2 commits into
masterfrom
memFixes

Conversation

@TMRh20

@TMRh20 TMRh20 commented May 29, 2026

Copy link
Copy Markdown
Member

Fixes for RF24Gatway w/help from AI:

  1. Potential stack/heap overflow from unchecked RF24 frame size

    File: /tmp/workspace/nRF24/RF24Gateway/RF24Gateway.cpp
    Function: ESBGateway::handleRadioIn
    Location: line 400 (memcpy(&msg.message, &f.message_buffer, f.message_size);)
    Why dangerous: msg.message is fixed-size (MAX_PAYLOAD_SIZE in header), but f.message_size is not bounded before copy.
    Risk: If f.message_size > MAX_PAYLOAD_SIZE, this overflows msg.message. Potentially exploitable if attacker can inject malformed/corrupted radio frames.

  2. Unchecked payload length when packing UDP buffer

    File: /tmp/workspace/nRF24/RF24Gateway/RF24Gateway.cpp
    Function: ESBGateway::sendUDP
    Location: lines 759–766 (uint8_t buffer[MAX_PAYLOAD_SIZE + 11]; ... memcpy(... frame.message_size);)
    Why dangerous: Copies frame.message_size bytes into fixed stack buffer without validating frame.message_size <= MAX_PAYLOAD_SIZE.
    Risk: Stack overflow if oversized frame reaches this function. Could be exploitable depending on call path/input control.

  3. Out-of-bounds reads during packet parsing (short packet not checked)

    File: /tmp/workspace/nRF24/RF24Gateway/RF24Gateway.cpp
    Function: ESBGateway::handleRadioOut
    Location: lines 508–509, 554, 561 (tmp+4, tmp[19], tmp[16])
    Why dangerous: Code reads specific offsets from msgTx->message before verifying minimum packet length.
    Risk: OOB read/crash with truncated packets from TUN/TAP input. Mostly robustness/DoS, but parser bugs are security-relevant.

1. Potential stack/heap overflow from unchecked RF24 frame size
2. Unchecked payload length when packing UDP buffer
3. Out-of-bounds reads during packet parsing (short packet not checked)
@TMRh20
TMRh20 requested a review from 2bndy5 May 29, 2026 12:08

@2bndy5 2bndy5 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm assuming the 6 and 20 are thresholds required to prevent UB. At least the other added bounds checking uses an explanatory const name (MAX_PAYLOAD_SIZE).

@TMRh20

TMRh20 commented May 29, 2026

Copy link
Copy Markdown
Member Author

Yes, the >6 & >20 thresholds would leave us with a 1-byte payload.

@2bndy5

2bndy5 commented May 29, 2026

Copy link
Copy Markdown
Member

Can we add a comment or replace the magic numbers with a named const? I have no problem with the added conditional checks, but I'd like to leave some kind of breadcrumb that doesn't require git blame to explain.

@TMRh20

TMRh20 commented May 29, 2026

Copy link
Copy Markdown
Member Author

Can we add a comment or replace the magic numbers with a named const?

Yes. Do you have a preference? Feel free to modify.

@2bndy5

2bndy5 commented May 29, 2026

Copy link
Copy Markdown
Member

I think a comment would good enough, but I'm not familiar with the context by just looking at the diff.

Did the AGENTS.md help AI in analyzing the RF24 stack for this? I've never used AI for C++.

@TMRh20

TMRh20 commented May 29, 2026

Copy link
Copy Markdown
Member Author

Did the AGENTS.md help AI in analyzing the RF24 stack for this?

I'm not sure, I just asked it to find buffer overflows and memory leaks in RF24Gateway, and pointed it at the gateway repo. Then I asked it to validate my fixes in this branch.
I'll add some comments.

@TMRh20

TMRh20 commented May 29, 2026

Copy link
Copy Markdown
Member Author

Is this good?

        // Ensure that the length of the payload includes at least the MAC address
        if (msgTx->size < 6) {
            txQueue.pop();
            return;
        }
            // Ensure that the length of the payload includes at least the 20-byte TCP/IP header
            if (msgTx->size < 20) {
                txQueue.pop();
                continue;
            }

@2bndy5

2bndy5 commented May 29, 2026

Copy link
Copy Markdown
Member

Perfect. better than I expected.

Looking at your session here, I don't think it needed to deep dive into the stack.

- Add comments to latest fixes
- Modify the return call to continue in the first fix
@TMRh20

TMRh20 commented May 29, 2026

Copy link
Copy Markdown
Member Author

Looking at your session here, I don't think it needed to deep dive into the stack.

Probably not, but it can't hurt to verify things down the stack.

As you can see there are still a couple remaining issues to address, but the fixes here are the main areas of possible external exploits.

@TMRh20
TMRh20 merged commit 1f03273 into master May 29, 2026
6 checks passed
@TMRh20
TMRh20 deleted the memFixes branch May 29, 2026 12:57
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.

2 participants