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.

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

Support IPv6 in in_private_net function, reduce noise on errors #157

Merged
merged 3 commits into from
Oct 14, 2020
Merged

Support IPv6 in in_private_net function, reduce noise on errors #157

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
abc08c0
reduce error log noise by suppressing the stacktrace
kroepke Mar 23, 2020
6c3f617
extend `in_private_net` to check for unique local addresses in IPv6
kroepke Oct 9, 2020
91d4f64
use Graylog's IpSubnet class instead of ancient netty one
kroepke Oct 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/main/java/org/graylog/plugins/threatintel/functions/misc/PrivateNetLookupFunction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import org.graylog.plugins.pipelineprocessor.ast.functions.FunctionDescriptor;
import org.graylog.plugins.pipelineprocessor.ast.functions.ParameterDescriptor;
import org.graylog.plugins.threatintel.tools.PrivateNet;
import org.graylog2.shared.utilities.ExceptionUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand Down Expand Up @@ -64,7 +65,7 @@ public Boolean evaluate(FunctionArgs args, EvaluationContext context) {

return result;
} catch (Exception e) {
LOG.error("Could not run private net lookup for IP [{}].", ip, e);
LOG.error("Could not run private net lookup for IP [{}]: {}", ip, ExceptionUtils.getRootCauseMessage(e));
return null;
}
}
Expand Down
19 changes: 9 additions & 10 deletions src/main/java/org/graylog/plugins/threatintel/tools/PrivateNet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,26 +17,25 @@
package org.graylog.plugins.threatintel.tools;

import com.google.common.net.InetAddresses;
import org.apache.commons.net.util.SubnetUtils;

public class PrivateNet {
import java.net.Inet6Address;
import java.net.InetAddress;

public static final SubnetUtils.SubnetInfo TEN = new SubnetUtils("10.0.0.0/8").getInfo();
public static final SubnetUtils.SubnetInfo ONE_HUNDRED_SEVENTY_TWO = new SubnetUtils("172.16.0.0/12").getInfo();
public static final SubnetUtils.SubnetInfo ONE_HUNDRED_NINETY_TWO = new SubnetUtils("192.168.0.0/16").getInfo();
public class PrivateNet {

/**
* Checks if an IPv4 address is part of a private network as defined in RFC 1918.
/**
* Checks if an IPv4 address is part of a private network as defined in RFC 1918. This ignores IPv6 addresses for now and always returns false for them.
*
* @param ip The IPv4 address to check
* @return
*/
public static boolean isInPrivateAddressSpace(String ip) {
if(!InetAddresses.isInetAddress(ip)) {
InetAddress inetAddress = InetAddresses.forString(ip);
if (inetAddress instanceof Inet6Address) {
// we don't deal with IPv6 unique local addresses currently.
return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we should be doing this, since it changes the behavior of the lookup function.
If we can't or won't answer the question if a v6 address is private, we shouldn't return a boolean value.

Copy link
Contributor

Choose a reason for hiding this comment

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

How about we return a boxed Boolean here, and keep returning null for v6 addresses?

Copy link
Member Author

Choose a reason for hiding this comment

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

null could be an option, but I'd like to avoid doing it if possible.
An alternative is to use https://www.rfc-editor.org/rfc/rfc4193.txt ranges (FC00::/7), which I think is technically correct, but probably rarely used in practice.

Copy link
Contributor

Choose a reason for hiding this comment

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

that's what isSiteLocalAddress would be doing for v6. But I'm wondering if it wouldn't be more practical to also include link local addresses fe80::/10 as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think link-local addresses are out of scope for this function: the original version also didn't consider link-local IPv4 address, because they shouldn't be routed anyway.

For the purposes of this function, I'll now update the PR to include the unique local addresses in IPv6 and properly return true for those.

}

return ONE_HUNDRED_SEVENTY_TWO.isInRange(ip) || TEN.isInRange(ip) || ONE_HUNDRED_NINETY_TWO.isInRange(ip);
return inetAddress.isSiteLocalAddress();
}

}
Expand Down
2 changes: 2 additions & 0 deletions src/test/java/org/graylog/plugins/threatintel/tools/PrivateNetTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ public void testIsInPrivateAddressSpace() throws Exception {
assertTrue(PrivateNet.isInPrivateAddressSpace("172.16.20.50"));
assertTrue(PrivateNet.isInPrivateAddressSpace("192.168.1.1"));
assertFalse(PrivateNet.isInPrivateAddressSpace("99.42.44.219"));
assertFalse(PrivateNet.isInPrivateAddressSpace("ff02:0:0:0:0:0:0:fb"));
assertThrows(IllegalArgumentException.class, () -> PrivateNet.isInPrivateAddressSpace("this is not an IP address"));
}

}