This patch adds some paranoia checks for the -c flag. It also fixes a
bug that could cause sftpd to get into an infinite loop (a possible
DoS).
To apply the patch:
% cd sftpd*
% patch -p1 < req.enc.paranoia.patch.txt
% make distclean ; ./configure && make
diff -ub --recursive sftpd-1.45p4/doc/sftpd.html sftpd/doc/sftpd.html
--- sftpd-1.45p4/doc/sftpd.html Fri Sep 15 15:55:05 2000
+++ sftpd/doc/sftpd.html Fri Sep 22 03:25:00 2000
@@ -115,7 +115,9 @@
-c
-Require data encryption to be used for all file transfers.
+Require data encryption to be used for all file transfers. This option
+implies -3 (force data relay) and -9 (disallow unencrypted
+connections).
-rlow-high
diff -ub --recursive sftpd-1.45p4/sc/mkdist sftpd/sc/mkdist
--- sftpd-1.45p4/sc/mkdist Fri Aug 25 18:02:21 2000
+++ sftpd/sc/mkdist Fri Sep 22 03:35:46 2000
@@ -2,20 +2,31 @@
# make a tarball for distribution
if [ "$1" = "" ]; then
- echo "usage: $0 version-number"
+ echo "usage: $0 version-number [pre-tag]"
+ echo " e.g.: $0 1.45 p1"
exit
fi
cd misc/rel || exit 2
cvs export -D now sftpd || exit 2
-mv sftpd sftpd-$1 || exit 2
-if [ -f sftpd-$1.tar.gz ]; then
+mv sftpd "sftpd-$1" || exit 2
+if [ -f "sftpd-$1.tar.gz" ]; then
# already exists.. replace it
- rm -f sftpd-$1.tar.gz
+ rm -f "sftpd-$1.tar.gz"
fi
-targz sftpd-$1 || exit 2
-rm -rf sftpd-$1 || exit 2
+targz "sftpd-$1" || exit 2
+rm -rf "sftpd-$1" || exit 2
-echo "tarball created as misc/rel/sftpd-$1.tar.gz"
+# rename based on pre
+now="sftpd-$1.tar.gz"
+if [ "$2" = "" ]; then
+ fnl="$now"
+else
+ fnl="sftpd-$1$2.tar.gz"
+ mv "$now" "$fnl" || exit 2
+fi
+
+echo "tarball created as misc/rel/$fnl"
+md5 "$fnl"
exit
diff -ub --recursive sftpd-1.45p4/sftpc.cpp sftpd/sftpc.cpp
--- sftpd-1.45p4/sftpc.cpp Mon Sep 18 12:01:24 2000
+++ sftpd/sftpc.cpp Fri Sep 22 03:19:28 2000
@@ -213,7 +213,7 @@
Reply reply = readReply();
// do DIGT and ADAT processing
- handleReply(reply);
+ handleAdatAndDigt(reply);
// look at reply code
if (first(reply.getCode()) != RCF_POSITIVE_PRELIMINARY) {
@@ -247,6 +247,16 @@
Reply reply = readFinalReply();
// confirm is ok
+ checkReplyCode(reply);
+
+ // return the code itself
+ return reply.getCode();
+}
+
+
+// throw xReply if there is a problem
+void SFTPC::checkReplyCode(Reply const &reply) const
+{
ReplyCodeFirst f = first(reply.getCode());
if (!( f == RCF_POSITIVE_COMPLETION ||
f == RCF_POSITIVE_INTERMEDIATE )) {
@@ -258,13 +268,10 @@
// doesn't appear to be a possible return from readFinalReply.
// 6/8/99: doh! it's "preliminary" which can't be returned;
// "intermediate" can be (and is; e.g., "user" response is 331)
-
- // return the code itself
- return reply.getCode();
}
-void SFTPC::handleReply(Reply &reply)
+void SFTPC::handleAdatAndDigt(Reply &reply)
{
// the reply may go into the DIGT calculation
if (isDigtActive()) {
@@ -620,6 +627,7 @@
// get reply
Reply reply = readReply();
+ checkReplyCode(reply); // minor fix: w/o this, errors produce a confusing message
// parse the address and port
IPAddress addr;
@@ -2228,7 +2236,7 @@
Reply reply = readReply();
// DIGT and ADAT processing
- handleReply(reply);
+ handleAdatAndDigt(reply);
// since we don't know what this reply was for,
// there isn't anything intelligent to do with
@@ -2348,8 +2356,9 @@
// if we're turning on data encryption for the first time,
// negotiate a PBSZ
- if (newLevel != DSL_CLEAR &&
- !negotiatedPBSZ()) {
+ if (!negotiatedPBSZ()) {
+ // fix: was checking that newLevel != DSL_CLEAR, but 2228
+ // specifies that PBSZ must come first even for "PROT C"
requestPBSZ();
}
diff -ub --recursive sftpd-1.45p4/sftpc.h sftpd/sftpc.h
--- sftpd-1.45p4/sftpc.h Sat Sep 16 02:27:57 2000
+++ sftpd/sftpc.h Fri Sep 22 03:19:28 2000
@@ -103,8 +103,11 @@
// get the reply, throw exception on error reply code
ReplyCode readAndCheckFinalReplyCode();
- // print, and otherwise respond to reply
- void handleReply(Reply &reply);
+ // throw xReply if not success
+ void checkReplyCode(Reply const &reply) const;
+
+ // deal with ADATs, possibly adding them to DIGT checksum
+ void handleAdatAndDigt(Reply &reply);
// --------- request stuff ----------
diff -ub --recursive sftpd-1.45p4/sftpd.cpp sftpd/sftpd.cpp
--- sftpd-1.45p4/sftpd.cpp Mon Sep 18 12:01:24 2000
+++ sftpd/sftpd.cpp Fri Sep 22 03:19:28 2000
@@ -448,7 +448,13 @@
break;
case 'x':
+ if (requireDataEncryption) { // handle -c before -x
+ log(LE_ERROR, "-x is incompatible with -c");
+ errors = true;
+ }
+ else {
allowCleartext = true;
+ }
break;
case '9':
@@ -491,6 +497,14 @@
case 'c':
requireDataEncryption = true;
+
+ // 9/22/00 02:07: tightening-down -c even more: make it
+ // imply -3, so the direct-to-ftpd route is completely
+ // closed, and -9 so we never accept unencrypted conns
+ forceDataRelay = true;
+ allowRfc959 = false;
+ allowRfc959_anon = false;
+ allowCleartext = false; // in case -c after -x
break;
case 'r': {
@@ -1344,6 +1358,9 @@
// it worked; no big deal
diagnostic("port command was accepted");
+ // 9/22/00 02:01 bugfix: wasn't resetting this
+ serverPassivePort = NONPASV_PORT;
+
// but must send ftpd's reply! this was a bug in 1.10 ... :(
clientReply(reply);
}
@@ -1353,6 +1370,10 @@
// this fn is called in both 959 interop and normal SafeTP mode
void SFTPD::handlePortCommand(Request const &request)
{
+ if (failsRequireEncTest()) {
+ return;
+ }
+
if (doingDataRelay()) {
// bugfix: clear any prior data-channel socket state
closeDataSockets();
@@ -1856,6 +1877,7 @@
if (level == DSL_NONE) {
clientReply(RC_PARAMETER_SYNTAX_ERROR,
"Unknown PROT command code.");
+ break; // 9/22/00 00:28 bugfix: 'break' was missing
}
// verify we can support it (should never fail, but now is as good
@@ -1909,15 +1931,19 @@
// with the *attempt*, so at the earliest moment we see that
// the client sent a data-transmission command, we inc
if (security && dataChannelProtected()) {
+ if (requireDataEncryption) {
+ // paranioa rule: never call newFile with anything other
+ // than DSL_PRIVATE when requireDataEncryption==true
+ security->data().newFile(DSL_PRIVATE);
+ }
+ else {
security->data().newFile(dataSecLevel);
}
+ }
- // policy option
- if ( requireDataEncryption &&
- !(security && (dataSecLevel == DSL_PRIVATE)) ) {
- clientReply(RC_REQUEST_DENIED,
- "This server requires data encryption to be enabled -- turn on "
- "data encryption in your SafeTP software.");
+ // policy option; test this *after* newFile to obey file-number
+ // increment semantics
+ if (failsRequireEncTest()) {
break;
}
@@ -1929,6 +1955,7 @@
if (!pasvMode() &&
doingDataRelay() &&
server_listen == INVALID_SOCKET) {
+ diagnostic("doing extra PORT for default-client-port transfer");
listenServerDataPort();
}
@@ -2040,6 +2067,23 @@
}
+// if the transfer isn't allowed, reply to the user and return true;
+// otherwise return false
+bool SFTPD::failsRequireEncTest()
+{
+ if ( requireDataEncryption &&
+ !(security && (dataSecLevel == DSL_PRIVATE)) ) {
+ clientReply(RC_REQUEST_DENIED,
+ "This server requires data encryption to be enabled -- turn on "
+ "data encryption in your SafeTP software.");
+ return true;
+ }
+ else {
+ return false;
+ }
+}
+
+
// check the login name for special processing; return true if
// action was taken, such that the calling code should not do
// any more processing
@@ -2140,8 +2184,20 @@
// the client has just issued a PASV command; we must relay it to the
// server, but instead of passing on the server's response, substitute
// our own newly-opened port
+//
+// I do not implement the direct-to-ftpd optimization for PASV
+// transfers.. this originally was simply an oversight, but as I
+// consider it more, the problem is (unlike PORT), I can't detect
+// (before it causes a problem) whether the server is going to balk at
+// the address the client connects from. So my decision now is if
+// someone really wants the direct-to-ftpd for performance, they can
+// use active mode. -- sm, 9/22/00 02:07
void SFTPD::handlePassiveMode()
{
+ if (failsRequireEncTest()) {
+ return;
+ }
+
// bugfix: clear any prior data-channel socket state
closeDataSockets();
@@ -2376,7 +2432,9 @@
// read from client, decrypt, forward to server
void SFTPD::decryptDataBlock()
{
- // read the block size, 32 bits in network-byte-order
+ // read the block size, 32 bits in network-byte-order; if the
+ // connection has been closed, this will throw xSocket, and
+ // it will be caught and handled in the caller
unsigned long blockSize = recvNBO32(client_data);
if (blockSize > maxBlockSize) {
xsecurity(stringb("Block size was " << blockSize <<
@@ -2441,6 +2499,11 @@
maxCleartextBlockSize);
if (cleartextBlockSize == 0) {
diagnostic("ftpd just closed the data connection");
+
+ // 9/22/00 01:06 bugfix: shut down the socket now; if something
+ // fails in the code below, we don't want the conn-closed condition
+ // to keep causing us to come here and fail over and over
+ closeSocket(server_data);
}
if (printDataChannel) {
@@ -2480,8 +2543,8 @@
// response to STOR, RETR, etc.)
// shut down data sockets
+ // (update: moved the close(server_data) up)
closeSocket(client_data);
- closeSocket(server_data);
}
doArtificialDelay();
@@ -2490,6 +2553,9 @@
void SFTPD::relaySocketData(SOCKET &dest, SOCKET &source)
{
+ // paranoia
+ xassert(!requireDataEncryption);
+
// just use a local buffer for simplicity
enum { BUFSIZE = 1024 };
char buf[BUFSIZE];
diff -ub --recursive sftpd-1.45p4/sftpd.h sftpd/sftpd.h
--- sftpd-1.45p4/sftpd.h Fri Sep 15 15:55:03 2000
+++ sftpd/sftpd.h Fri Sep 22 03:19:28 2000
@@ -175,6 +175,8 @@
void encryptedClientReply(Reply const &reply);
void unencryptedClientReply(Reply const &reply);
+ bool failsRequireEncTest();
+
// automatically determine whether to encrypt the reply or not
void clientReply(Reply const &reply);
void clientReply(ReplyCode code, char const *msg);
diff -ub --recursive sftpd-1.45p4/sftpd.todo.txt sftpd/sftpd.todo.txt
--- sftpd-1.45p4/sftpd.todo.txt Tue Sep 19 02:08:07 2000
+++ sftpd/sftpd.todo.txt Tue Sep 19 03:29:25 2000
@@ -54,6 +54,8 @@
- HUP to reopen log file
- actual configuration file instead of endless cmdline options..
+
+ - misc/testinstall doesn't work on OSF because of chown thing
Can't reproduce
Only in sftpd: sktsrvr
Only in sftpd: socktest