denoland / deno

A modern runtime for JavaScript and TypeScript.
https://deno.com
MIT License
94.63k stars 5.25k forks source link

`--allow-net=0.0.0.0:port` allows both `fetch` and `Deno.listen` #16532

Open mcodik opened 1 year ago

mcodik commented 1 year ago

deno 1.24.2 (release, aarch64-apple-darwin) v8 10.4.132.20 typescript 4.7.4

If your code uses Deno.listen, it requires you to give permission via --allow-net=0.0.0.0:port

In my testing on Mac 12.6.1, calling fetch("http://0.0.0.0:8080") ends up hitting localhost on port 8080. Apparently this is true for Macs generally as curl http://0.0.0.0:8080 works too. Some discussion here: https://github.com/whatwg/fetch/issues/1117

A consequence of this is that --allow-net cant distinguish the two flows: if you allow listening to http requests, you also allow code to hit localhost on those ports. The same permission allows both.

This isn't a problem for us, but since the behavior was surprising to me I thought I'd post an issue to share. Example code snippet below.

const startServer = async function (port: number) {
  const server = Deno.listen({ port });
  for await (const conn of server) {
    serveHttp(conn);
  }

  async function serveHttp(conn: Deno.Conn) {
    const httpConn = Deno.serveHttp(conn);
    for await (const requestEvent of httpConn) {
      const url = new URL(requestEvent.request.url);
      console.log("GET " + url.search);
      if (url.search == "?a=1") {
        await fetch("http://0.0.0.0:8080/foo?a=2");
      }

      await requestEvent.respondWith(
        new Response("OK", {
          status: 200,
        }),
      ).catch((e) => console.log(`Uncaught exception: ${e}`));
    }
  }
};

if (import.meta.main) {
  const port = 8080;
  await startServer(port);
}

Run this with deno run --no-prompt --allow-net=0.0.0.0:8080 ./localhost.ts in one terminal, then if you curl "localhost:8080/foo?a=1 in another terminal, you can see see both ?a=1 (from curl) and ?a=2 (from fetch(0.0.0.0)) logged

teleclimber commented 1 year ago

Yes, it's a problem that --allow-net does not differentiate between listen and dial. If you want togive liberal permissions to a script to make requests to a any host, you also give the script the ability to listen on any port. This can have huge consequences on a server environment. I wrote an issue long ago but it got stale-botted into oblivion:

https://github.com/denoland/deno/issues/2705

bartlomieju commented 1 year ago

CC @crowlKats I believe this is something you wanted to look into for Deno 2.0. The question is what the flag names would be

jtoppine commented 1 year ago

It seems a sensible option would be to change the meaning of --allow-net for Deno 2.0 to only allow making requests / dial. This would break the least amount of existing code, as most often the intention is to allow some code to act as client to whatever service.

Then introduce --allow-listen or such for Deno 2.0. Which allows listening for ports / creating servers. Server apps are not as numerous, and are more security sensitive. So it would make sense to err on the side of caution, let existing code fail by permission denied, and expect server apps/bootstrap code to adapt to the changed semantics of --allow-net vs --allow-listen.

Aloso commented 1 year ago

How about adding two new flags: allow-request and --allow-listen. Then --allow-net would not change, and just be an abbreviation for --allow-listen --allow-request.

carragom commented 10 months ago

Just stumbled upon this and IMHO I agree that leaving --allow-net as it's today would be great since it does not break anything, maybe deprecate it and remove it later. As for the other two flags: --allow-listen and --allow-connect would be great names.

Finally the new parameters could use a URL like syntax. A few examples come to mind.

General purpose using transport protocol as scheme, should fit all needs:

Some shortcuts could exist for well known application protocols using application protocol as scheme:

jtoppine commented 8 months ago

Since we are getting closer to Deno 2.0 and some mildly breaking changes can be expected, should we reopen this discussion?

Now would be great opportunity to introduce separate permission for listening on a port versus dialing to somewhere else.

From a user's point of view, as in what permission a user might want to give to a script.. this distinction of creating a server (listen) vs allowing external fetch/connect (dial) is quite clear typically. It is quite confusing and unexpected that these are lumped together in to a singular permission flag.

The worst thing I can imagine a malicious module could attempt is to open up port to for a longlived server listening for whatever actions an attacker might want to execute on the machine. To me an explicit permission flag for binding to a port would make so much more sense.