Skip to content

Revert "Don't record private IP literals as outbound hostnames (Zen alert flood)" #309

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

Open
bitterpanda63 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Revert "Don't record private IP literals as outbound hostnames (Zen alert flood)" #309

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
23 changes: 6 additions & 17 deletions agent_api/src/main/java/dev/aikido/agent_api/collectors/DNSRecordCollector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
import dev.aikido.agent_api.vulnerabilities.ssrf.SSRFException;
import dev.aikido.agent_api.helpers.logging.LogManager;
import dev.aikido.agent_api.helpers.logging.Logger;
import dev.aikido.agent_api.vulnerabilities.ssrf.IsPrivateIP;
import dev.aikido.agent_api.vulnerabilities.ssrf.StoredSSRFDetector;
import dev.aikido.agent_api.vulnerabilities.ssrf.StoredSSRFException;

Expand All @@ -38,23 +37,13 @@ public static void report(String hostname, InetAddress[] inetAddresses) {
// Consume pending ports recorded by URLCollector for this hostname.
// Removing them here ensures each (hostname, port) pair is counted exactly once.
Set<Integer> ports = PendingHostnamesStore.getAndRemove(hostname);

// The outbound-domains list is meant for hostnames, not raw IP literals.
// A private/internal IP literal passed straight to getAllByName (DNS-resolver
// bootstrap, service discovery connecting by IP, libraries building a private-IP
// matcher, ...) is not an outbound domain and would otherwise flood the
// "new outbound connection" feature. Skip recording those; SSRF/stored-SSRF and
// outbound-domain blocking below are unaffected.
boolean isPrivateIpLiteral = IsPrivateIP.isPrivateIp(hostname);
if (!isPrivateIpLiteral) {
if (!ports.isEmpty()) {
for (int port : ports) {
HostnamesStore.incrementHits(hostname, port);
}
} else {
// We still need to report a hit to the hostname for outbound domain blocking
HostnamesStore.incrementHits(hostname, 0);
if (!ports.isEmpty()) {
for (int port : ports) {
HostnamesStore.incrementHits(hostname, port);
}
} else {
// We still need to report a hit to the hostname for outbound domain blocking
HostnamesStore.incrementHits(hostname, 0);
}

// Block if the hostname is in the blocked domains list
Expand Down
55 changes: 0 additions & 55 deletions agent_api/src/test/java/collectors/DNSRecordCollectorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,59 +223,4 @@ public void testStoredSSRFWithNoContext() throws InterruptedException {
DNSRecordCollector.report("metadata.google.internal", new InetAddress[]{imdsAddress1, inetAddress2});
});
}

@Test
public void testPrivateIpLiteralNotRecordedAsOutboundHostname() throws UnknownHostException {
// Regression: a private/internal IP literal passed straight to getAllByName
// (e.g. Reactor Netty resolver bootstrap resolving nameserver/gateway addresses,
// service discovery connecting by IP, a library building a private-IP matcher)
// must NOT be recorded as an outbound hostname, otherwise it floods the
// "new outbound connection" feature on port 0.
Context.set(null);
DNSRecordCollector.report("10.0.0.0", new InetAddress[]{InetAddress.getByName("10.0.0.0")});
DNSRecordCollector.report("172.16.0.0", new InetAddress[]{InetAddress.getByName("172.16.0.0")});
DNSRecordCollector.report("192.168.0.0", new InetAddress[]{InetAddress.getByName("192.168.0.0")});
DNSRecordCollector.report("169.254.0.0", new InetAddress[]{InetAddress.getByName("169.254.0.0")});
DNSRecordCollector.report("10.20.11.143", new InetAddress[]{InetAddress.getByName("10.20.11.143")});
DNSRecordCollector.report("127.0.0.1", new InetAddress[]{inetAddress2});

assertEquals(0, HostnamesStore.getHostnamesAsList().length);
}

@Test
public void testPrivateIpLiteralWithPendingPortStillConsumedButNotRecorded() {
// The pending port must still be consumed (so it can't leak into a later
// lookup of a different hostname), but nothing is recorded for the IP literal.
PendingHostnamesStore.add("10.20.11.143", 443);
Context.set(mock(ContextObject.class));

DNSRecordCollector.report("10.20.11.143", new InetAddress[]{inetAddress2});

assertEquals(0, HostnamesStore.getHostnamesAsList().length);
assertTrue(PendingHostnamesStore.getPorts("10.20.11.143").isEmpty());
}

@Test
public void testHostnameResolvingToPrivateIpStillRecorded() {
// Only raw IP literals are filtered. A real DNS name that resolves to a private
// IP (the customer's internal services) is still recorded by NAME.
Context.set(null);
DNSRecordCollector.report("keycloak.internal.example.com", new InetAddress[]{inetAddress2});

Hostnames.HostnameEntry[] entries = HostnamesStore.getHostnamesAsList();
assertEquals(1, entries.length);
assertEquals("keycloak.internal.example.com", entries[0].getHostname());
assertEquals(0, entries[0].getPort());
}

@Test
public void testPublicIpLiteralStillRecorded() {
// Scope: only PRIVATE IP literals are dropped; public IP literals remain visible.
Context.set(null);
DNSRecordCollector.report("1.1.1.1", new InetAddress[]{inetAddress1});

Hostnames.HostnameEntry[] entries = HostnamesStore.getHostnamesAsList();
assertEquals(1, entries.length);
assertEquals("1.1.1.1", entries[0].getHostname());
}
}
Loading