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

Video Decoder not sanitizing IP addresses. #325

Open
cal-pratt opened this issue Jun 16, 2017 · 3 comments
Open

Video Decoder not sanitizing IP addresses. #325

cal-pratt opened this issue Jun 16, 2017 · 3 comments
Assignees
Milestone

Comments

@cal-pratt
Copy link
Contributor

cal-pratt commented Jun 16, 2017

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
Copy link
Contributor Author

cal-pratt commented Jun 16, 2017

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
Copy link
Contributor

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
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants