search

mickael-kerjean / filestash

🦄 A file manager / web client for SFTP, S3, FTP, WebDAV, Git, Minio, LDAP, CalDAV, CardDAV, Mysql, Backblaze, ...
https://www.filestash.app/
GNU Affero General Public License v3.0
10.53k stars 781 forks source link

[bug] skip tls verify in ftps #710

Open nyxfqq opened 4 months ago

nyxfqq commented 4 months ago

Description of the bug

In the Init function of index.go located in github.com/mickael-kerjean/filestash/server/plugin/plg_backend_ftp, the FTPS (FTPs over TLS) connections are being established with the InsecureSkipVerify flag set to true. This configuration instructs the client to bypass the validation of the server's TLS certificate, which is inherently insecure and can expose the connection to man-in-the-middle attacks.

Step by step instructions to reproduce the bug

  1. Navigate to the server/plugin/plg_backend_ftp/index.go file. 2 Within the Init function, locate the conditional statement that configures the tls.Config when withTLS is true.
  2. You'll find the line that sets InsecureSkipVerify to true.
  3. Run the application and connect to an FTPS server. Note that the TLS connection will not validate the server's certificate.

Observed behavior

The FTPS connection is established without validating the server's TLS certificate, which means the connection is susceptible to interception and tampering by third parties.

Expected behavior

The TLS configuration should include a secure validation of the server's certificate. At minimum, there should be an option to enable or disable certificate verification based on the user's preference or security requirements. This would ensure that the FTPS connections are properly secured and not vulnerable to security threats due to misconfigured TLS settings.

mickael-kerjean commented 3 months ago

Like a lot of things in Filestash, there's a compromise between "ease of use" and "security" and you might have a different opinion about where the bar is in various plugins: hence the plugin based approach for all the storage that are supported so you can put your own opinion in if you want to, pick the thing you want and discard the rest because most companies will not see kindly a software that can speak all protocols and all flavors of those protocols.

The plugin you cite is made for the layman who have no idea what a certificate is and won't have the level of understanding to know why his FTP doesn't work properly when it does work fine somewhere else and will think that's a bug in Filestash. The goal of plg_backend_ftp is to support the most FTP server I could find in the wild and this has a hidden cost, InsecureSkipVerify is the prime example of such hidden cost as tons of ftps server are either missconfigured or are link to root certificates that aren't known to any of us (very typical in enterprise settings and intranets).

If I were to blindly apply the suggested change, tons of people will lose access to their FTP and that's not ok. The real question is how do we solve this? The most realistic answer is to have a "hardened ftps" version that instead of favoring compatibility with all possible FTP server would provide a hardened version of it that work without any of the drawback of the existing implementation and at the cost of compatibility. I'm happy to accept a PR to whoever make one up as it would make for a great addition. On my end I'm way to busy working on this

nyxfqq commented 3 months ago

We are very sorry for the inconvenience caused to you, but you did not edit the security bulletin page on GitHub. Our issue has been raised for more than 3 weeks and we have not received a response before we chose to make CVE public. We have now closed the bulletin page and contacted the CVE team to revoke the public release