EasternEdgeRobotics / Software_2017

The control software for 2017
MIT License
3 stars 0 forks source link

Video Decoder not sanitizing IP addresses. #325

Open cal-pratt opened 7 years ago

cal-pratt commented 7 years ago

Was using the topside software yesterday and noticed that a "/" is appended to the front of the IP Address being sent to the Pi Camera's.

@krs158 I think you meant to remove a letter from the start of the ip string:

final int lastLocation = newAddress.lastIndexOf('.');
if (lastLocation >= 0) {
    final String testAddress = newAddress.substring(1, lastLocation - 1);
    if (broadcastIP.equals(testAddress)) {
        eventPublisher.emit(new VideoValueA(newAddress, config.portA()));
        eventPublisher.emit(new VideoValueB(newAddress, config.portB()));

should maybe be:

final int lastLocation = newAddress.lastIndexOf('.');
if (lastLocation >= 0) {
    final String testAddress = newAddress.substring(1, lastLocation - 1);
    if (broadcastIP.equals(testAddress)) {
        eventPublisher.emit(new VideoValueA(newAddress.substring(1), config.portA()));
        eventPublisher.emit(new VideoValueB(newAddress.substring(1), config.portB()));

Using a Regular Expression A safer way to handle this would be to use a Regular Expression. These are one of the coolest tools you have when it comes to working with strings. With a regular expression you can extract complicated patterns from a string without having to worry about these off by one errors.

You can use the Pattern class to come up with a pattern for your address, and match it against that. For example, this is how you can get an IP out of this string asdasASD/192.168.88.123@!asdasd

Pattern p = Pattern.compile("[^1-9]*([1-9]{1,3}[.][1-9]{1,3}[.][1-9]{1,3}[.][1-9]{1,3})[^1-9]*");
Matcher m = p.matcher("asdasASD/192.168.88.123@!asdasd");
boolean b = m.matches();
if (b) {
    System.out.println(m.group(1));
}
// prints:
192.168.88.123

What is pattern compile? This is a Regular Expression to match an IP address. It was created to match the following pattern:

[^1-9]*([1-9]{1,3}\\.[1-9]{1,3}\\.[1-9]{1,3}\\.[1-9]{1,3})[^1-9]*

[^1-9]* is any character that is not 1 to 9, and * means 0 or more times [1-9]{1,3} is a character from 1 to 9 and {1,3} means 1 to 3 times repeating \\. is the literal . character. (whereas . without \\ means any character) ( ... ) means a group. anything in () is a group and can be extracted from the pattern

So in plain english, the long regex is: Some amount of non numerical characters followed by a group, followed by some amount of non numerical characters. The group is 1-3 numbers . 1-3 numbers . 1-3 numbers . 1-3 numbers

Making a regex for the subnet In our case you need to make a regex to match our subnet. That might look something like this:

[^1-9]*(192\\.168\\.88\\.[1-9]{1,3})[^1-9]*

In the existing code you have a subnet where the . characters are not escaped properly 192.168.88.. You can get the pattern tool to do this calling Pattern.quote("192.168.88.");

Final result

String subnet = "192.168.88.";
String testIP = "asdasASD/192.168.88.123@!asdasd";

Pattern p = Pattern.compile("[^1-9]*(" + Pattern.quote(subnet) + "[1-9]{1,3})[^1-9]*");
Matcher m = p.matcher(testIP);
boolean b = m.matches();
if (b) {
    System.out.println(m.group(1));
}
// prints:
192.168.88.123
cal-pratt commented 7 years ago

Further on this; I'd like you to create this in a new helper class so we can write a test for the method. Place this class in the io package, and create a JUnit class for it aswell:

public class NetworkUtil {
    public Optional<String> findMatchingSubnetIP(final String subnet, final String ipText) {
        ...
    }
}

Given a subnet IP subnet (This should be the full string including the ".255" portion) and a text with an ip address in it ipText, return the properly formatted ip address in an Optional using Optional#of. If there is no matching IP address return Optional#empty. In the video decoder class you can then call:

for (final String newAddress: ipAddresses) {
    final Optional<String> ip = NetworkUtil.findMatchingSubnetIP(subnet, newAddress);
    if (ip.isPresent()) {
        eventPublisher.emit(new VideoValueA(ip.get(), config.portA()));
        eventPublisher.emit(new VideoValueB(ip.get(), config.portB()));
        initialized = true;
        break;
    }
}
k-sutherland commented 7 years ago

I am curious about why you suggest using [1-9] in he regex instead of \d. Won't using the former exclude the possibility of the number being zero? The broadcast ip should be constant, so I don't think it would really come up in this case anyway, but ip addresses can contain zeros, so could that throw off a generalized version?

cal-pratt commented 7 years ago

Yup, good point. [1-9] is incorrect, you need to count zero [0-9]. And \d would work in almost all cases.

As for the difference, see this

\d checks all Unicode digits, while [0-9] is limited to these 10 characters. For example, Persian digits, ۱۲۳۴۵۶۷۸۹, are an example of Unicode digits which are matched with \d, but not [0-9].

I think either are \d or [0-9] fine.