Bug 25598

I forgot to patch the man page accordingly; I'm submitting the man page
diff here:

--- libexec/ftpd/ftpd.8.original        Thu Mar  8 03:36:41 2001
+++ libexec/ftpd/ftpd.8 Thu Mar  8 04:45:16 2001
@@ -236,6 +236,11 @@
 .It XRMD Ta "remove a directory (deprecated) [RW]"
 .El
 .Pp
+After a successful CWD command, a file
+.Pa .message
+is looked up in the new current directory and submitted to the client
+as part of the ok-message, if available.
+.Pp
 The following non-standard or
 .Tn UNIX
 specific commands are supported

On Thu, Mar 08, 2001 at 03:08:58AM +0100, bdluevel@heitec.net wrote:
> 
> >Number:         25598
> >Category:       bin
> >Synopsis:       patch to let ftpd output message when changing directory
> >Originator:     Bernd Luevelsmeyer
> >Release:        FreeBSD 4.3-BETA i386
> >Organization:
> >Environment:
> System: FreeBSD 4.3-BETA #0: Wed Mar 7 04:44:56 CET 2001
> 
> >Description:
> The patch lets the FTP daemon (after a successful 'cwd' command)
> look for a file '.message' in the current directory and, if
> readable, send the contents to the client, preceding the
> "250 CWD successful." message. The intent is to let
> the FTP site admin announce directory descriptions, copyright
> notices or the like.
[snip the patch itself]

Hmm I wonder if this should not stat() the file beforehand, to make
sure it's a regular file; otherwise, problems might arise with a local
user creating a FIFO or something, and then pointing a couple of clients
there.. or just letting the FIFO lie dormant until some unsuspecting
soul connects and CWD's :)

Of course, then there's the issue of a race condition between a stat()
and the actual opening.. this might be resolved with a fstat(fileno(fp))
right after the fopen(), before the first read from the file.

Or should FIFO's be considered an issue at all?  I believe yes, since
something similar has been done to inetd recently..

G'luck,
Peter

-- 
Nostalgia ain't what it used to be.

Peter Pentchev wrote:
> 
> On Thu, Mar 08, 2001 at 03:08:58AM +0100, bdluevel@heitec.net wrote:
[...]
> > >Description:
> > The patch lets the FTP daemon (after a successful 'cwd' command)
> > look for a file '.message' in the current directory and, if
> > readable, send the contents to the client, preceding the
> > "250 CWD successful." message. The intent is to let
> > the FTP site admin announce directory descriptions, copyright
> > notices or the like.
> [snip the patch itself]
> 
> Hmm I wonder if this should not stat() the file beforehand, to make
> sure it's a regular file; otherwise, problems might arise with a local
> user creating a FIFO or something, and then pointing a couple of clients
> there.. or just letting the FIFO lie dormant until some unsuspecting
> soul connects and CWD's :)

You are right, of course. Thanks for wording it so politely :-)
There's also the issue of uploads, e.g. in public upload directories;
someone might create a directory (or find one without a .message) and
put a megabyte of whatever there, called '.message' .


> Of course, then there's the issue of a race condition between a stat()
> and the actual opening.. this might be resolved with a fstat(fileno(fp))
> right after the fopen(), before the first read from the file.

I think that's the way to go. To care for unwanted 3rd-party-.messages,
one might perhaps check that it's owned by root, or owned by the
directory owner? Not world-writeable? And limit the output to max. 20
lines of max. 50 characters each, filtered to printable ASCII (checked
with isprint())?


> Or should FIFO's be considered an issue at all?  I believe yes, since
> something similar has been done to inetd recently..

It is certainly an issue. I'm afraid I didn't consider this topic at
all. I'll implement the fstat() and a size limitation and/or other
sanity checks, and re-submit the patch.

I suggest that this ill-conceived PR should be closed.

On Fri, Mar 09, 2001 at 09:10:14AM +0100, Bernd Luevelsmeyer wrote:
> Peter Pentchev wrote:
> 
> > Of course, then there's the issue of a race condition between a stat()
> > and the actual opening.. this might be resolved with a fstat(fileno(fp))
> > right after the fopen(), before the first read from the file.
> 
> I think that's the way to go. To care for unwanted 3rd-party-.messages,
> one might perhaps check that it's owned by root, or owned by the
> directory owner? Not world-writeable? And limit the output to max. 20
> lines of max. 50 characters each, filtered to printable ASCII (checked
> with isprint())?

I think most of these checks are reasonable, esp. the owned-by-root-or-owner
check (I think that would be the best way to go - allow root to drop .message
files all over the place, and let owners put their own), which would also
take care of the problem you mentioned earlier, .message files uploaded
to public incoming directories.

> > Or should FIFO's be considered an issue at all?  I believe yes, since
> > something similar has been done to inetd recently..
> 
> It is certainly an issue. I'm afraid I didn't consider this topic at
> all. I'll implement the fstat() and a size limitation and/or other
> sanity checks, and re-submit the patch.
> 
> I suggest that this ill-conceived PR should be closed.

Wouldn't it be better to leave this PR open, so you can post your patches
as follow-ups?

G'luck,
Peter

-- 
I am the thought you are now thinking.

Peter Pentchev wrote:
> 
> On Fri, Mar 09, 2001 at 09:10:14AM +0100, Bernd Luevelsmeyer wrote:
[...]
> > I suggest that this ill-conceived PR should be closed.
> 
> Wouldn't it be better to leave this PR open, so you can post your patches
> as follow-ups?

If it's still open, I'll follow-up; if not I'll submit a new one and
point to this PR in the description.

Comment 6 Kris Kennaway 2003-07-13 02:24:07 UTC

Responsible Changed
From-To: freebsd-bugs->yar

Assign to ftpd maintainer

Comment 7 Yar Tikhiy 2003-07-21 16:39:00 UTC

State Changed
From-To: open->suspended

We are going to have lukemftpd instead of BSD-ftpd in our 
tree in near future, so it's not a good time to introduce 
more divergence between the two ftpd variants by adding 
new features.

Comment 8 Eitan Adler 2012-07-10 04:30:45 UTC

State Changed
From-To: suspended->closed

per yar@