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

Passive socket leak after failed command #257

Open
andrewwalters opened this issue May 13, 2021 · 2 comments
Open

Passive socket leak after failed command #257

andrewwalters opened this issue May 13, 2021 · 2 comments
Labels
bug A defect or bug that affects the original indended use of the application good first issue Hacktoberfest security

Comments

@andrewwalters
Copy link

When running in passive mode, if a command fails to execute for some reason (such as being blacklisted), the socket that was going to be used for the data connection indefinitely in the LISTENING state.

Repro 1:

  1. Run cli with options: node . ftp://0.0.0.0:2222 --read-only
  2. Using a command-line ftp client, connect to your server and try to 'put' a file. It will fail because the command is blacklisted:
229 EPSV OK (|||1024|)
502 Command blacklisted: STOR
  1. Optionally run another command, such as 'ls' (LIST). Observe that it uses the next passive port (1025) and succeeds.
  2. Run netstat on the server's machine (netstat -nlt on Linux, netstat -nf inet on Mac).

Expected: Node will no longer be listening on the port used for the failed command (1024 in this example).
Actual:

  • netstat shows port in LISTENING (Linux) or CLOSE_WAIT (Mac) state indefinitely, even after ftp client terminates.
  • Turning up log level to 'trace' shows that 'Passive server closed' message for that port never appears.

Repro 2 (lower level):

  1. Run cli: node . ftp://0.0.0.0:2222
  2. telnet localhost 2222 and issue the following commands directly to the server:
USER x
PASS x
EPSV
  1. In separate shell, telnet localhost <port returned by EPSV command> and then close the connection.
  2. Run netstat as above.
@matt-forster matt-forster added bug A defect or bug that affects the original indended use of the application security labels May 13, 2021
@matt-forster
Copy link
Contributor

Thanks for the report! This is a pretty serious issue so we will get to fixing it ASAP. Any help is appreciated, as well.

@matt-forster matt-forster self-assigned this May 13, 2021
@andrewwalters
Copy link
Author

I dug a little more and saw that the passive connector does try to shut down when it sees that the socket has been closed. But it looks like the problem is that Node won't send a close or end event while a socket is in the paused state. As an experiment (not a fix, but just to observe the issue), I added a resume to the blacklist check in commands/index.js:

    if (_.includes(this.blacklist, command.directive)) {
      setTimeout(() => this.connection.connector.socket.resume(), 10000);
      return this.connection.reply(502, `Command blacklisted: ${command.directive}`);
    }

Sure enough, running through Repro 1, the socket gets close ~10 seconds after a STOR command fails.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A defect or bug that affects the original indended use of the application good first issue Hacktoberfest security
Projects
None yet
Development

No branches or pull requests

2 participants