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

Add MQTT exception for private IP address server #5072

Merged
merged 6 commits into from
Oct 16, 2024

Conversation

JohnathonMohr
Copy link
Contributor

Related to issue #5000 and implementing what was suggested in #5023.

Allowing MQTT publishing regardless of the flag setting if the server being published to is local (for local automation scenarios).

I have not completed testing yet, but wanted to get early feedback.

@CLAassistant
Copy link

CLAassistant commented Oct 15, 2024

CLA assistant check
All committers have signed the CLA.

@thebentern thebentern requested a review from jp-bennett October 15, 2024 09:43
@thebentern thebentern requested a review from fifieldt October 15, 2024 15:28
Copy link
Contributor

@fifieldt fifieldt left a comment

Choose a reason for hiding this comment

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

Thanks for working on this and congratulations on your first patch to Meshtastic :)

@fifieldt fifieldt merged commit ad214ea into meshtastic:master Oct 16, 2024
47 checks passed
Copy link
Collaborator

@jp-bennett jp-bennett left a comment

Choose a reason for hiding this comment

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

Does this catch something sneaky like 10.mydomain.com? I started working on this a couple days ago, and went down that rabbit hole. I think we should check that we only have dots and decimals in the address.

@Talie5in
Copy link
Contributor

Talie5in commented Oct 16, 2024

@jp-bennett
Quick fuzzy check and your right, would not grab 10.something.com

@JohnathonMohr

Might be able to do that with some regex to make sure it's an IP first?

#include <regex>

bool MQTT::isValidIpAddress(const char ip[])
{
    // Use a simple regular expression to check for valid IPv4 addresses.
    const std::regex ipPattern("^([0-9]{1,3}\\.){3}[0-9]{1,3}$");
    return std::regex_match(ip, ipPattern);
}

bool MQTT::isPrivateIpAddress(const char ip[]) 
{
    // First, check if it's a valid IPv4 address.
    if (!isValidIpAddress(ip)) {
        return false;
    }

    // Check for common private IP address ranges.
    if (strcmp(ip, "127.0.0.1") == 0 || 
        strncmp(ip, "10.", 3) == 0 || 
        strncmp(ip, "192.168", 7) == 0) {
        return true;
    }

    // Early exit if it's not a 172 address.
    if (strncmp(ip, "172.", 4) != 0) {
        return false;
    }

    // Ensure the format is correct (checking that the IP has at least two octets and no buffer overflows).
    if (strlen(ip) < 7 || ip[6] != '.') {
        return false;
    }

    // Parse the second octet.
    char octet2[3] = { ip[4], ip[5], '\0' };
    int octet2Num = atoi(octet2);

    // Return true if the second octet is within the private range for 172.x.x.x.
    return octet2Num >= 16 && octet2Num <= 31;
}

@jp-bennett
Copy link
Collaborator

Might be able to do that with some regex to make sure it's an IP first?

I can only imagine how much flash space adding regex to the firmware is going to cost. We only need to check once per boot. Just make a char array of "0123456789." and a nested for loop.

@Talie5in
Copy link
Contributor

Talie5in commented Oct 16, 2024

@jp-bennett Yup, didnt think of that, how about cstring as this is already loaded in many places in the firmware?

#include <cstring> // for strlen

bool MQTT::isValidIpAddress(const char ip[])
{
    int numDots = 0;
    int length = strlen(ip);  // Get the length of the string

    // IPv4 addresses should be between 7 and 15 characters (e.g., "0.0.0.0" to "255.255.255.255").
    if (length < 7 || length > 15) {
        return false;
    }

    const char *ptr = ip;

    // Check if the string is empty or null-terminated properly.
    if (*ptr == '\0') {
        return false;
    }

    while (*ptr) {
        int octet = 0;
        int digitCount = 0;

        // Parse each octet (up to three digits).
        while (*ptr >= '0' && *ptr <= '9') {
            octet = octet * 10 + (*ptr - '0');
            ptr++;
            digitCount++;

            // Ensure no octet exceeds 255 or is longer than three digits.
            if (octet > 255 || digitCount > 3) {
                return false;
            }
        }

        // After each octet, we expect either a dot (.) or the end of the string.
        if (*ptr == '.') {
            numDots++;
            ptr++;  // Move past the dot.
            if (numDots > 3) {
                return false;  // Too many dots.
            }
        } else if (*ptr != '\0') {
            return false;  // Invalid character found.
        }
    }

    // We should have exactly 3 dots and 4 octets (number of dots = 3).
    return numDots == 3;
}

Just trying to keep memory safe and keep things in bounds (avoid buffer overflows etc)

@JohnathonMohr
Copy link
Contributor Author

Good catch on decimal prefixes of domains. I'll include the check in the next iteration (avoiding regex).

@fifieldt
Copy link
Contributor

Thanks for the additional review folks.

@JohnathonMohr
Copy link
Contributor Author

JohnathonMohr commented Oct 16, 2024

@Talie5in, it's great to see your idea of confirming a valid IPv4, but for the simple purpose of determining whether to consider the server as a private address or not, I think it's more than what's required.

As long as we can determine it's not a valid domain (i.e. it has only digits and dots) I think that's sufficient. If we determine it's a private address after that, even if it's not a valid address, I don't think it matters since a connection will never be able to be made anyway.

@Talie5in
Copy link
Contributor

@JohnathonMohr All goodly!

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.

5 participants