Skip to content

Incomplete file when uploaded over TLS data connection #123

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

Open
jkryl wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Incomplete file when uploaded over TLS data connection #123

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 2 additions & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,9 +163,10 @@ The user is not able to escape this directory.
- `tlsOptions`: _(default: undefined)_
- If this is set, the server will allow explicit TLS authentication.
- Value should be a dictionary which is suitable as the `options` argument of `tls.createServer`.
- `tlsOptions.requestCert` can be set to true to require a certificate from clients.
- `tlsOptions.rejectUnauthorized` is ignored unless `tlsOptions.requestCert` is true and if it is set then only clients with certificates signed by trusted CA are allowed to connect.
- `tlsOnly`: _(default: false)_
- If this is set to `true`, and `tlsOptions` is also set, then the server will not allow logins over non-secure connections.
- `allowUnauthorizedTls`: ?? I obviously set this to true when tlsOnly is on -someone needs to update this.
- `pasvPortRangeStart`: _(default: random?)_
- Integer, specifies the lower-bound port (min port) for creating PASV connections
- `pasvPortRangeEnd`: _(default: random?)_
Expand Down
20 changes: 14 additions & 6 deletions lib/FtpConnection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,15 +87,20 @@ FtpConnection.prototype._createPassiveServer = function() {
self._closeSocket(psocket, true);
self.dataConfigured = false;
} else if (!cleartext.authorized) {
if (self.server.options.allowUnauthorizedTls) {
self._logIf(LOG.INFO, 'Allowing unauthorized passive connection (allowUnauthorizedTls is on)');
if (!self.server.options.tlsOptions.rejectUnauthorized) {
self._logIf(LOG.INFO, 'Allowing unauthorized passive connection (tlsOptions.rejectUnauthorized is off)');
switchToSecure();
} else {
self._logIf(LOG.INFO, 'Closing unauthorized passive connection (allowUnauthorizedTls is off)');
self._logIf(LOG.INFO, 'Closing unauthorized passive connection (tlsOptions.rejectUnauthorized is on)');
self._closeSocket(self.socket, true);
self.dataConfigured = false;
}
} else {
// TODO: Verify that cert used for data connection is the same as
// the one used for control connection.
// TODO: Implement callback for client identity check based on its
// certificate, which would verify that client's certificate is not
// only valid, but also appropriate for that particular client.
switchToSecure();
}

Expand Down Expand Up @@ -317,14 +322,17 @@ FtpConnection.prototype._command_AUTH = function(commandArg) {
self._closeSocket(self.socket, true);
} else if (!cleartext.authorized) {
self._logIf(LOG.INFO, 'Secure socket not authorized: ' + util.inspect(cleartext.authorizationError));
if (self.server.options.allowUnauthorizedTls) {
self._logIf(LOG.INFO, 'Allowing unauthorized connection (allowUnauthorizedTls is on)');
if (!self.server.options.tlsOptions.rejectUnauthorized) {
self._logIf(LOG.INFO, 'Allowing unauthorized connection (tlsOptions.rejectUnauthorized is off)');
switchToSecure();
} else {
self._logIf(LOG.INFO, 'Closing unauthorized connection (allowUnauthorizedTls is off)');
self._logIf(LOG.INFO, 'Closing unauthorized connection (tlsOptions.rejectUnauthorized is on)');
self._closeSocket(self.socket, true);
}
} else {
// TODO: Implement callback for client identity check based on its
// certificate, which would verify that client's certificate is not
// only valid, but also appropriate for that particular client.
switchToSecure();
}

Expand Down
6 changes: 6 additions & 0 deletions lib/FtpServer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,12 @@ function FtpServer(host, options) {
if (!options.getRoot) {
throw new Error("'getRoot' option of FtpServer must be set");
}
if (options.tlsOptions) {
// rejectUnauthorized is meaningful only if there is a client certificate
if (!options.tlsOptions.requestCert) {
options.tlsOptions.rejectUnauthorized = false;
}
}
self.getInitialCwd = options.getInitialCwd;
self.getRoot = options.getRoot;

Expand Down
117 changes: 17 additions & 100 deletions lib/starttls.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,25 +2,7 @@
// https://github.com/andris9/rai/blob/master/lib/starttls.js
// (This code is MIT licensed.)

//
// Target API:
//
// var s = require('net').createStream(25, 'smtp.example.com');
// s.on('connect', function() {
// require('starttls')(s, options, function() {
// if (!s.authorized) {
// s.destroy();
// return;
// }
//
// s.end("hello world\n");
// });
// });
//
//

var tls = require('tls');
var crypto = require('crypto');

// From Node docs for TLS module.
var RECOMMENDED_CIPHERS = 'ECDHE-RSA-AES256-SHA:AES256-SHA:RC4-SHA:RC4:HIGH:!MD5:!aNULL:!EDH:!AESGCM';
Expand All @@ -33,111 +15,46 @@ function starttlsClient(socket, options, callback) {
}

function starttls(socket, options, callback, isServer) {
var sslcontext;

var opts = {};

Object.keys(options).forEach(function(key) {
opts[key] = options[key];
});
if (!opts.ciphers) {
opts.ciphers = RECOMMENDED_CIPHERS;
}
opts.isServer = isServer;
opts.secureContext = tls.createSecureContext(opts);

socket.removeAllListeners('data');
if (tls.createSecureContext) {
sslcontext = tls.createSecureContext(opts);
} else {
sslcontext = crypto.createCredentials(opts);
}
var pair = tls.createSecurePair(sslcontext, isServer);
var cleartext = pipe(pair, socket);

var secureSocket = new tls.TLSSocket(socket, opts);
var erroredOut = false;
pair.on('secure', function() {

// NodeJS documentation bug: secure vs secureConnect
// https://github.com/nodejs/node/issues/10555

@sam-github sam-github Jan 16, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

:-(. I will document the API.

secureSocket.on('secure', function() {
if (erroredOut) {
pair.end();
secureSocket.end();
return;
}

var verifyError = (pair._ssl || pair.ssl).verifyError();

if (verifyError) {
cleartext.authorized = false;
cleartext.authorizationError = verifyError;
var authError = secureSocket.ssl.verifyError();
if (authError) {
secureSocket.authorized = false;
secureSocket.authorizationError = authError;
} else {
cleartext.authorized = true;
secureSocket.authorized = true;

@sam-github sam-github Jan 22, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Its not authorized yet. You have connected to a valid certificate, but you don't know if its a certificate for the host you want to connect to! It See https://github.com/sam-github/node/blob/39f39189ee6d4ef3c6d20033721ae515a744267f/doc/api/tls.md#event-secure

@jkryl jkryl Jan 23, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I'm not sure I can do that as I don't know the hostname of client. This is server side of FTP connection, so client is connecting to us. All we can know about the client is perhaps its IP address and even that one does not have to be correct (if client is behind NAT or so). So making sure that the certificate presented by the client is valid is probably the best we can do here. It looks that neither the previous version of the code called checkServerIdentity(). Since it's not a regression I would prefer to leave the code as it is.

@mk-pmb mk-pmb Jan 23, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this code even special for the case where clients do present their own certificates, or do we have to assume the client authenticated with username and password? In the latter case, as far as I understand it, there would be no client certificate at all that we could verify.
In that case we should still put a comment that this were the place to put certificate verification if we knew which cert to expect, to help future additions and for better example code.

@jkryl jkryl Jan 26, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I agree. By skipping server identity check we can make people to believe that the connection is fully secure while it is not. IMHO it is really a corner case as I doubt that any one runs FTP server configured to validate client certificates but nevertheless we can either:

  1. put a comment into the code explaining how the things are (as you suggest) or
  2. throw exception from FTP server constructor if anyone tries to use requestCert option to avoid false impression of security or
  3. allow user to specify his own checkServerIdentity method in options and throw exception if it is not specified (and requestCert is true)

@mk-pmb: What would be the preferred solution in your opinion? I'm open to other suggestions as well. thanks

@mk-pmb mk-pmb Jan 26, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I skimmed the README again to see what behavior people might expect from our config documentation. Took me a while to find the TLS options, we should add some keyword-y hints like "Configure your server certificate and private key here."

The best I could find was allowUnauthorizedTls, that sounds like it could be used to determine the security level required, but I'd prefer something more like requireTlsCert with at least four values:

  • no = no cert required,
  • any = .requestCert, any cert will do (e.g. learning phase for whitelist),
  • ca = .requestCert + .rejectUnauthorized (cert must be CA-signed),
  • custom = .requestCert + fire a validation event where I can plug-in my whitelist of which self-signed user certs I accept.

The validation event should provide:

  • TLS info on an abstraction level near the node API.
    • I wouldn't want to work with that directly, instead I'd want a module that makes validation more admin-friendly. However, this module should work for any node TLS, not just FTP, thus out of project scope here.
    • Our docs should have a (link to a) list of what modules we can recommend for admin-friendly TLS validation. Even if that list might start empty, adding an item might feel easier if the collection already exists. (It assures that yes it is a good idea to collect this kind of item.)
    • A comment near the source line that fires the validation event should remind that the docs offer a list of recommended modules.
  • easy access to some session object where information can be shared between at least these events:
    • TLS validation on control channel
      • USER/PASS on control channel
      • TLS validation on data channel
        • People might want to verify the cert is not just valid for this user but also the same one used on the control channel.
        • I'm undecided whether we should even make this the default.
  • a decider function with one argument that we treat as boolean flag whether to accept the connection.
    • Whichever event handler calls the decider first, wins. If people want more sophisticated decision strategies like priorities, they can hand out their own decider function, count ballots, and provide our decider with the final result.

@jkryl jkryl Jan 31, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

yea, you are right. checkServerIdentity is used only on client side of TLS connection. So my proposal regarding checkServerIdentity was not correct. I looked at what general TLS server implementation in nodejs does and it is surprising: https://github.com/nodejs/node/blob/master/lib/_tls_wrap.js#L814 . It does not bother to verify hostname to match a certificate at all. Valid certificate used by client is sufficient. I got confused by Sam's initial comment about missing checkServerIdentity call:

... you don't know if its a certificate for the host you want to connect to!

Sam thought that we deal with client side of the connection, but that's wrong. So if we want to provide more sophisticated validation of client certificates, it is possible but we should pick a different name for such method than checkServerIdentity (i.e. checkClientIdentity :-) ) and it would get called only if client's certificate was successfully verified. Regardless how it is named implementation of this new option sounds exactly like feature which could be implemented in future as extension of what we have now?

@sam-github sam-github Jan 31, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yes, sorry, I thought it was client side code.

@mk-pmb mk-pmb Jan 31, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should think of what exactly the client's cert shall certify. The remote IP or its hostname, probably not. I could think of a username disguised as a hostname, or email address, my CA software could do both. Those would make sense to verify.

@jkryl jkryl Feb 1, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I agree that it is not very clear what should be subject to verification. We should keep in mind that when TLS handshake happens on control connection we don't even know client's login credentials. I'm sure it will be much easier to add this feature when there is a need for it, rather than if one has to guess what might be needed without any particular use case. I have updated my PR to include documentation changes clarifying how tlsOptions should be used and inserted a couple of TODOs to source code - hints on possible future improvements. Thanks!

@mk-pmb mk-pmb Feb 1, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

when TLS handshake happens on control connection we don't even know client's login credentials

Right, so we'd have to save which name the cert claimed to certify (if we believe it) so we can remember it when a USER or PASS command arrives.

}

callback(null, cleartext);
callback(null, secureSocket);
});
pair.once('error', function(err) {
secureSocket.once('error', function(err) {
if (!erroredOut) {
erroredOut = true;
callback(err);
}
});

cleartext._controlReleased = true;
pair;
}

function forwardEvents(events, emitterSource, emitterDestination) {
var map = [];

for (var i = 0, len = events.length; i < len; i++) {
var name = events[i];

var handler = forwardEvent.bind(emitterDestination, name);

map.push(name);
emitterSource.on(name, handler);
}

return map;
}

function forwardEvent() {
this.emit.apply(this, arguments);
}

function removeEvents(map, emitterSource) {
for (var i = 0, len = map.length; i < len; i++) {
emitterSource.removeAllListeners(map[i]);
}
}

function pipe(pair, socket) {
pair.encrypted.pipe(socket);
socket.pipe(pair.encrypted);

pair.fd = socket.fd;

var cleartext = pair.cleartext;

cleartext.socket = socket;
cleartext.encrypted = pair.encrypted;
cleartext.authorized = false;

function onerror(e) {
if (cleartext._controlReleased) {
cleartext.emit('error', e);
}
}

var map = forwardEvents(['timeout', 'end', 'close', 'drain', 'error'], socket, cleartext);

function onclose() {
socket.removeListener('error', onerror);
socket.removeListener('close', onclose);
removeEvents(map, socket);
}

socket.on('error', onerror);
socket.on('close', onclose);

return cleartext;
}

exports.starttlsServer = starttlsServer;
exports.starttlsClient = starttlsClient;
exports.RECOMMENDED_CIPHERS = RECOMMENDED_CIPHERS;