Skip to content

Fix #72764. ftps:// opendir - Negotiate data channel encryption after NLST command #2058

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

Closed
wants to merge 1 commit into from

Conversation

vhuk
Copy link
Contributor

@vhuk vhuk commented Aug 5, 2016

This is to prevent FTPS issues with IIS, ProFTPD and essentially fixes the mistake I did on PR #2032.

Sorry about that.

… NLST command.

This is to prevent issues with IIS and ProFTPD.
@nikic
Copy link
Member

nikic commented Aug 5, 2016

This is what RFC 4217 has to say on the topic:

              Client                                 Server
     control          data                   data               control
   ====================================================================

     PASV -------------------------------------------------------->
                                             socket()
                                             bind()
         <------------------------------------------ 227 (w,x,y,z,a,b)
                      socket()
     STOR file --------------------------------------------------->
                      connect()  ----------> accept()
         <-------------------------------------------------------- 150
                      TLSneg()   <---------> TLSneg()
                      TLSwrite()  ---------> TLSread()
                      TLSshutdown() -------> TLSshutdown()
                      close()     ---------> close()
         <-------------------------------------------------------- 226

So yeah, it looks like TLS negotiation is supposed to occur after receiving the NLST response.

What I find peculiar however, is that this diagram seems to match what we were doing originally (i.e. first NLST, then open data connection always and only then read response).

@vhuk
Copy link
Contributor Author

vhuk commented Aug 5, 2016

That's interesting indeed. It basically means to revert PR #2033 and leave it up to the non-compliant servers to fix the issue (IIS FTP 7.5+). I can update the test cases if we go that route.

@nikic
Copy link
Member

nikic commented Aug 5, 2016

Merged as 65056e9 into 5.6+.

If the implementation as it is now works with IIS (and nobody else complains) I think we should keep it... (Also we're now really in line with ext/ftp -- I did not notice before that it does the TLS initialization only after getting the response as well.)

@nikic nikic closed this Aug 5, 2016
@nikic
Copy link
Member

nikic commented Aug 5, 2016

Am I understanding it correctly that prior to the s/stream/datastream patch opendir over ftps did not work at all, so we do not actually regress anything if a version without the change in this PR is released?

@vhuk
Copy link
Contributor Author

vhuk commented Aug 6, 2016

That's correct, opendir over ftps didn't work at all prior to fixing bug 54431. opendir over ftp work correctly on 5.6.25RC1, at least against ProFTPD, vsftpd. Will test additional platforms at Monday.

There is a slight change in the failure mode but that's about it. Before 5.6.25RC1 opendir over ftps failed immediately, now it'll fail after the negotiation times out.

@vhuk
Copy link
Contributor Author

vhuk commented Aug 8, 2016

I ran some additional test for this to confirm 5.6.25RC1 works fine over ftp:// without this PR, but will fail using ftps://.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants