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

Only capture hostname when the request is not blocked because of SSRF #381

Closed
wants to merge 4 commits into from
Closed

Only capture hostname when the request is not blocked because of SSRF #381

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
af6312f
AIK-3347 Only capture hostname when the request is not blocked becaus…
timokoessler Sep 19, 2024
048efd8
Fix fetch tests
timokoessler Sep 19, 2024
2eef0be
Pass port on undici request if context is not set
timokoessler Sep 19, 2024
ef5ec27
Merge branch 'main' into ssrf-capture-hostname
timokoessler Sep 27, 2024
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
8 changes: 6 additions & 2 deletions library/sinks/Fetch.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,13 +83,17 @@ t.test(

t.same(agent.getHostnames().asArray(), [
{ hostname: "aikido.dev", port: 80 },
{ hostname: "www.aikido.dev", port: 443 },
]);
agent.getHostnames().clear();

await fetch(new URL("https://aikido.dev"));
// If we open a connection to the same hostname while the previous one is still open, it will reuse the same connection
// In this case the dns.lookup call will not be called again and the hostname will not be added to the list
// But because the connection gets closed, the hostname will be added to the list for requests in a new heartbeat interval
await fetch(new URL("https://www.aikido.dev"));

t.same(agent.getHostnames().asArray(), [
{ hostname: "aikido.dev", port: 443 },
{ hostname: "www.aikido.dev", port: 443 },
]);
agent.getHostnames().clear();

Expand Down
20 changes: 16 additions & 4 deletions library/sinks/Fetch.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { tryParseURL } from "../helpers/tryParseURL";
import { checkContextForSSRF } from "../vulnerabilities/ssrf/checkContextForSSRF";
import { inspectDNSLookupCalls } from "../vulnerabilities/ssrf/inspectDNSLookupCalls";
import { wrapDispatch } from "./undici/wrapDispatch";
import { isIP } from "net";

export class Fetch implements Wrapper {
private patchedGlobalDispatcher = false;
Expand All @@ -18,21 +19,32 @@ export class Fetch implements Wrapper {
hostname: string,
port: number | undefined
): InterceptorResult {
// Let the agent know that we are connecting to this hostname
// This is to build a list of all hostnames that the application is connecting to
agent.onConnectHostname(hostname, port);
const context = getContext();

if (!context) {
// Add to list of hostnames that the application is connecting to
agent.onConnectHostname(hostname, port);
return undefined;
}

return checkContextForSSRF({
const foundAttack = checkContextForSSRF({
hostname: hostname,
operation: "fetch",
context: context,
port: port,
});

if (foundAttack) {
return foundAttack;
}

if (isIP(hostname)) {
// Add to list of hostnames that the application is connecting to
// Don't add domain names to the list yet, as they might be resolved to a private IP
agent.onConnectHostname(hostname, port);
}

return undefined;
}

inspectFetch(args: unknown[], agent: Agent): InterceptorResult {
Expand Down
40 changes: 23 additions & 17 deletions library/sinks/HTTPRequest.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,34 +49,38 @@ const context: Context = {
route: "/posts/:id",
};

t.test("it works", (t) => {
const agent = new Agent(
true,
new LoggerNoop(),
new ReportingAPIForTesting(),
new Token("123"),
undefined
);
agent.start([new HTTPRequest()]);

t.same(agent.getHostnames().asArray(), []);

const http = require("http");
const https = require("https");

runWithContext(context, () => {
const agent = new Agent(
true,
new LoggerNoop(),
new ReportingAPIForTesting(),
new Token("123"),
undefined
);
agent.start([new HTTPRequest()]);

t.same(agent.getHostnames().asArray(), []);
Copy link
Member

Choose a reason for hiding this comment

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

A bit weird to have an assertion outside a test? Let's move that line?


const http = require("http");
const https = require("https");

t.test("it discovers hostnames", async (t) => {
await runWithContext(context, async () => {
const aikido = http.request("http://aikido.dev");
aikido.end();
// Wait for the request to finish
await new Promise((resolve) => aikido.on("finish", resolve));
});

t.same(agent.getHostnames().asArray(), [
{ hostname: "aikido.dev", port: 80 },
]);
agent.getHostnames().clear();

runWithContext(context, () => {
await runWithContext(context, async () => {
const aikido = https.request("https://aikido.dev");
aikido.end();
// Wait for the request to finish
await new Promise((resolve) => aikido.on("finish", resolve));
});
t.same(agent.getHostnames().asArray(), [
{ hostname: "aikido.dev", port: 443 },
Expand Down Expand Up @@ -132,7 +136,9 @@ t.test("it works", (t) => {
t.throws(() => https.request("invalid url"));
t.same(agent.getHostnames().asArray(), []);
agent.getHostnames().clear();
});

t.test("it blocks SSRF", (t) => {
runWithContext(
{ ...context, ...{ body: { image: "thisdomainpointstointernalip.com" } } },
() => {
Expand Down
12 changes: 9 additions & 3 deletions library/sinks/HTTPRequest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import { getUrlFromHTTPRequestArgs } from "./http-request/getUrlFromHTTPRequestArgs";
import { wrapResponseHandler } from "./http-request/wrapResponseHandler";
import { isOptionsObject } from "./http-request/isOptionsObject";
import { isIP } from "net";

export class HTTPRequest implements Wrapper {
private inspectHostname(
Expand All @@ -20,12 +21,11 @@
port: number | undefined,
module: "http" | "https"
): InterceptorResult {
// Let the agent know that we are connecting to this hostname
// This is to build a list of all hostnames that the application is connecting to
agent.onConnectHostname(url.hostname, port);
const context = getContext();

if (!context) {
// Add to list of hostnames that the application is connecting to
agent.onConnectHostname(url.hostname, port);
return undefined;
}

Expand Down Expand Up @@ -53,6 +53,12 @@
};
}

if (isIP(url.hostname)) {
// Add to list of hostnames that the application is connecting to
// Don't add domain names to the list yet, as they might be resolved to a private IP
agent.onConnectHostname(url.hostname, port);
}

Check warning on line 60 in library/sinks/HTTPRequest.ts

View check run for this annotation

Codecov / codecov/patch

library/sinks/HTTPRequest.ts#L57-L60 

Added lines #L57 - L60 were not covered by tests

return undefined;
}

Expand Down
1 change: 1 addition & 0 deletions library/sinks/Undici.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ t.test(
await fetch("https://aikido.dev");
t.same(agent.getHostnames().asArray(), [
{ hostname: "aikido.dev", port: 443 },
{ hostname: "www.aikido.dev", port: 443 },
]);
agent.getHostnames().clear();

Expand Down
20 changes: 16 additions & 4 deletions library/sinks/Undici.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import { inspectDNSLookupCalls } from "../vulnerabilities/ssrf/inspectDNSLookupCalls";
import { wrapDispatch } from "./undici/wrapDispatch";
import { isOptionsObject } from "./http-request/isOptionsObject";
import { isIP } from "net";

const methods = [
"request",
Expand All @@ -33,21 +34,32 @@
port: number | undefined,
method: string
): InterceptorResult {
// Let the agent know that we are connecting to this hostname
// This is to build a list of all hostnames that the application is connecting to
agent.onConnectHostname(hostname, port);
const context = getContext();

if (!context) {
// Add to list of hostnames that the application is connecting to
agent.onConnectHostname(hostname, port);
return undefined;
}

return checkContextForSSRF({
const foundAttack = checkContextForSSRF({
hostname: hostname,
operation: `undici.${method}`,
context,
port,
});

if (foundAttack) {
return foundAttack;
}

if (isIP(hostname)) {
// Add to list of hostnames that the application is connecting to
// Don't add domain names to the list yet, as they might be resolved to a private IP
agent.onConnectHostname(hostname, port);
}

Check warning on line 60 in library/sinks/Undici.ts

View check run for this annotation

Codecov / codecov/patch

library/sinks/Undici.ts#L57-L60 

Added lines #L57 - L60 were not covered by tests

return undefined;
}

// eslint-disable-next-line max-lines-per-function
Expand Down
20 changes: 11 additions & 9 deletions library/sinks/undici/wrapDispatch.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ export function wrapDispatch(orig: Dispatch, agent: Agent): Dispatch {
return function wrap(opts, handler) {
const context = getContext();

if (!context || !opts || !opts.origin || !handler) {
if (!opts || !opts.origin || !handler) {
return orig.apply(
// @ts-expect-error We don't know the type of this
this,
Expand All @@ -53,16 +53,18 @@ export function wrapDispatch(orig: Dispatch, agent: Agent): Dispatch {
);
}

blockRedirectToPrivateIP(url, context, agent);

const port = getPortFromURL(url);

// Wrap onHeaders to check for redirects
handler.onHeaders = wrapOnHeaders(
handler.onHeaders,
{ port, url },
context
);
if (context) {
blockRedirectToPrivateIP(url, context, agent);

// Wrap onHeaders to check for redirects
handler.onHeaders = wrapOnHeaders(
handler.onHeaders,
{ port, url },
context
);
}

return RequestContextStorage.run({ port, url }, () => {
return orig.apply(
Expand Down
34 changes: 23 additions & 11 deletions library/vulnerabilities/ssrf/inspectDNSLookupCalls.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,10 +83,24 @@ function wrapDNSLookupCallback(

const context = getContext();

// This is set if this resolve is part of an outgoing request that we are inspecting
const requestContext = RequestContextStorage.getStore();

let port: number | undefined;

if (urlArg) {
port = getPortFromURL(urlArg);
} else if (requestContext) {
port = requestContext.port;
}

if (context) {
const matches = agent.getConfig().getEndpoints(context);

if (matches.find((endpoint) => endpoint.forceProtectionOff)) {
// Add to list of hostnames that the application is connecting to
agent.onConnectHostname(hostname, port);

// User disabled protection for this endpoint, we don't need to inspect the resolved IPs
// Just call the original callback to allow the DNS lookup
return callback(err, addresses, family);
Expand All @@ -109,25 +123,20 @@ function wrapDNSLookupCallback(
}

if (!context) {
// Add to list of hostnames that the application is connecting to
agent.onConnectHostname(hostname, port);

// If there's no context, we can't check if the hostname is in the context
// Just call the original callback to allow the DNS lookup
return callback(err, addresses, family);
}

// This is set if this resolve is part of an outgoing request that we are inspecting
const requestContext = RequestContextStorage.getStore();

let port: number | undefined;

if (urlArg) {
port = getPortFromURL(urlArg);
} else if (requestContext) {
port = requestContext.port;
}

const privateIP = resolvedIPAddresses.find(isPrivateIP);

if (!privateIP) {
// Add to list of hostnames that the application is connecting to
agent.onConnectHostname(hostname, port);

// If the hostname doesn't resolve to a private IP address, it's not an SSRF attack
// Just call the original callback to allow the DNS lookup
return callback(err, addresses, family);
Expand Down Expand Up @@ -166,6 +175,9 @@ function wrapDNSLookupCallback(
}

if (!found) {
// Add to list of hostnames that the application is connecting to
agent.onConnectHostname(hostname, port);
Copy link
Member

Choose a reason for hiding this comment

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

Looking for a way to avoid the sprinkled onConnectHostname

Easy to forget when adding a new if statement here or there, right?

undici & fetch

const response = await fetch("url")

When response is returned and the fetch call didn't throw, we know the request succeeded, can we maybe hook into the return value and call agent.onConnectHostname(hostname, port); there?

http/https module

We could wrap the on("response", (...) => {}) handler but we should NEVER add it when no response handler is (See #346)

So I think we have no choice to use the DNS lookup indeed, instead of adding the logic in inspectDNSLookupCalls, can we add a separate wrapper for this?

optionObj.lookup = inspectDNSLookupCalls(
      nativeLookup,
      agent,
      module,
      `${module}.request`,
      url,
      stackTraceCallingLocation
    ) as NonNullable<RequestOptions["lookup"]>;

becomes

optionObj.lookup = addHostnameToAgent(
     agent,
     inspectDNSLookupCalls(
      nativeLookup,
      agent,
      module,
      `${module}.request`,
      url,
      stackTraceCallingLocation
    )
);

function addHostnameToAgent(agent: Agent, lookup: Function) {
  return function addHostnameToAgent(...args: unknown[]) {
    // see if DNS lookup succeeded
    
    if (!error) {
      agent.onConnectHostname(hostname, port);
    }
  }
}


// If we can't find the hostname in the context, it's not an SSRF attack
// Just call the original callback to allow the DNS lookup
return callback(err, addresses, family);
Expand Down
Loading