Re: Alternative method for supporting TLS and latest patch set

>It tooks some reading thru the docs, but I did get it to work.
>Delegate runs as a real proxy server with protocol awareness,
>so nmh using smtp mode can be used with it.  To get complete,
>expected functionality, modifications to nmh was required, mainly
>with whom.c so it can support the -port option so address
>verification can work.
>[...]

I looked over your patches; I say commit 'em (I'd personally prefer that
nmh had inline TLS support, but we've already beaten that horse into the
ground, shot it a few times for good measure, ground it up into sausage,
eaten that sausage, shat out the sausage, and buried the aforementioned
excrement in the farthest reaches of the Gobi Desert).  These changes
are certainly worthwhile.

--Ken

_______________________________________________
Nmh-workers mailing list
Nmh-workers <at> nongnu.org
http://lists.nongnu.org/mailman/listinfo/nmh-workers

Re: Alternative method for supporting TLS and latest patch set

On February 1, 2010 at 22:06, Ken Hornstein wrote:

> I looked over your patches; I say commit 'em (I'd personally prefer that
> nmh had inline TLS support,...

I agree, but the changes are also applicable for any non-TLS mail
server that requires SASL.

The horse has definitely been pounded to death.  Maybe some
day nmh will have direct support for it.  I may look at it again
in the future.

BTW, I'm working on removing all uses of mktemp from the code
and trying to clean-up annoying warnings from gcc when compiling
code.

Hopefully, I do not break anything.  I'll try things it out for awhile
to see if anything breaks, but I do not use all the features of nmh,
so some changes will not really get tested.  I've taken the approach
of creating new functions that use mkstemp() and replacing all calles
to m_scratch() and m_tmpfil() with the new functions.

I'm unsure what the policy is for how stable the trunk is supposed
to be, so I hesitant to check in what I have done.

--ewh

_______________________________________________
Nmh-workers mailing list
Nmh-workers <at> nongnu.org

Re: Alternative method for supporting TLS and latest patch set

Earl Hood wrote:
>BTW, I'm working on removing all uses of mktemp from the code
>and trying to clean-up annoying warnings from gcc when compiling
>code.

Ah, good. I had a bit of a go at that a while ago but ran aground
on some of the less trivial bits.

>Hopefully, I do not break anything.  I'll try things it out for awhile
>to see if anything breaks, but I do not use all the features of nmh,
>so some changes will not really get tested.  I've taken the approach
>of creating new functions that use mkstemp() and replacing all calles
>to m_scratch() and m_tmpfil() with the new functions.
>
>I'm unsure what the policy is for how stable the trunk is supposed
>to be, so I hesitant to check in what I have done.

Well, the trunk shouldn't be deliberately broken, but we don't
have a "policy" as such  Send the diffs to the mailing list
(particularly useful since this is a security-related change)
and if nobody yells about them you can commit, I guess.

-- PMM

_______________________________________________
Nmh-workers mailing list
Nmh-workers <at> nongnu.org
http://lists.nongnu.org/mailman/listinfo/nmh-workers

Re: Alternative method for supporting TLS and latest patch set

>I'm unsure what the policy is for how stable the trunk is supposed
>to be, so I hesitant to check in what I have done.

I'm with Peter; we don't really have any formal policy as to the stability
of HEAD.  I've committed code that broke HEAD; it wasn't intentional, and
I fixed it as soon as I was aware that it was broken, but I didn't lose
any sleep over it.  I don't think a working HEAD is a substitute for
a real release now and then (note to self: we should really think about
doing a 1.4/2.0).

--Ken

_______________________________________________
Nmh-workers mailing list
Nmh-workers <at> nongnu.org
http://lists.nongnu.org/mailman/listinfo/nmh-workers

Diffs for replacing mktemp() usage

Attached are the changes I have currently done in my development
environment.  The changes also include minor mods to deal
with "may be uninitialized" warnings from gcc.

I added a new file, sbr/m_mktemp.c, to include the new functions for
replacing m_scratch() and m_tmpfil().

The goal was to minimize the amount of re-coding, so the new functions
attempt to provide the basic capabilities of the older functions,
but using mkstemp() under the hood vs mktemp().  It seems the much
code has a heavy reliance on being able to access the actual pathnames
of temporary files vs just having an open handle to it.

Index: h/prototypes.h
===================================================================
RCS file: /cvsroot/nmh/nmh/h/prototypes.h,v
retrieving revision 1.26
diff -u -r1.26 prototypes.h
--- h/prototypes.h	16 Jan 2009 02:28:54 -0000	1.26
+++ h/prototypes.h	2 Feb 2010 19:36:04 -0000
 <at>  <at>  -81,6 +81,8  <at>  <at> 
 int m_putenv (char *, char *);
 char *m_scratch (char *, char *);
 char *m_tmpfil (char *);
+char *m_mktemp(const char *, int *, FILE **);
+char *m_mktemp2(const char *, const char *, int *, FILE **);
 void m_unknown(FILE *);
 int makedir (char *);

Re: Diffs for replacing mktemp() usage

Earl Hood wrote:
>The goal was to minimize the amount of re-coding, so the new functions
>attempt to provide the basic capabilities of the older functions,
>but using mkstemp() under the hood vs mktemp().  It seems the much
>code has a heavy reliance on being able to access the actual pathnames
>of temporary files vs just having an open handle to it.

Yes, this is why it's difficult to fix . Unfortunately, if you
use mkstemp() but still allow the rest of the code to reopen
the temporary file by name, you've shut the linker up but
not completely closed the security hole. See
http://www.mail-archive.com/nmh-workers <at> nongnu.org/msg01380.html

So I would vote against (the tempfile related parts of) this patch.

-- PMM

_______________________________________________
Nmh-workers mailing list
Nmh-workers <at> nongnu.org
http://lists.nongnu.org/mailman/listinfo/nmh-workers

Re: Diffs for replacing mktemp() usage

peter wrote:
 > use mkstemp() but still allow the rest of the code to reopen
 > the temporary file by name, you've shut the linker up but
 > not completely closed the security hole. See
 > http://www.mail-archive.com/nmh-workers <at> nongnu.org/msg01380.html

huh.  i was just about to suggest that.  replacing mktemp with a
version that uses a user-only directory (and the routine could
check the permissions) seems like the best solution.  or, such a
directory could be created in /tmp when the command starts -- but
cleanup might be more of an issue in that case.

paul
=---------------------
 paul fox, pgf <at> foxharp.boston.ma.us (arlington, ma, where it's 30.6 degrees)

_______________________________________________
Nmh-workers mailing list
Nmh-workers <at> nongnu.org
http://lists.nongnu.org/mailman/listinfo/nmh-workers

Re: Diffs for replacing mktemp() usage

On February 2, 2010 at 22:26, Peter Maydell wrote:

> Yes, this is why it's difficult to fix . Unfortunately, if you
> use mkstemp() but still allow the rest of the code to reopen
> the temporary file by name, you've shut the linker up but
> not completely closed the security hole. See
> http://www.mail-archive.com/nmh-workers <at> nongnu.org/msg01380.html
> 
> So I would vote against (the tempfile related parts of) this patch.

I do not think the problem applies here.  With mkstemp(), the
file is actually created at the time, so no race condition.
If you look at the new functions, if no fd and no FILE return
is requested, I only close the file.  I do NOT unlink it.

There are parts of the code where the file "sits" there before
it is opened, but in that case, it is actually re-opening an
existing file vs creating a new file.

The only way the file becomes a vulnerability is of the sys admin
has misconfigured the tmp directory to allow anyone to clobber
anyone elses file.

As for temporary files under ~/Mail, some operations in the
code do that since the files end up being renamed.  It
is possible to modify the new functions I've added to default
to the user's mail directory for creating temporary files
vs /tmp.

--ewh

Re: Diffs for replacing mktemp() usage

> Yes, this is why it's difficult to fix . Unfortunately, if you
> use mkstemp() but still allow the rest of the code to reopen
> the temporary file by name, you've shut the linker up but
> not completely closed the security hole. See
> http://www.mail-archive.com/nmh-workers <at> nongnu.org/msg01380.html
> 
> So I would vote against (the tempfile related parts of) this patch.

Having an MH-private namespace for scratch files is certainly the way
to go here.  These aren't 'temp files' in the traditional sense, and
none of the usual APIs suit the task at hand.

There are license-compatible mkstemp() implementations out there that
can serve as a base for a code import, upon which a suitable
replacement can be built.

--lyndon

_______________________________________________
Nmh-workers mailing list
Nmh-workers <at> nongnu.org
http://lists.nongnu.org/mailman/listinfo/nmh-workers

Re: Diffs for replacing mktemp() usage

The changes I've made do not contain the same security flaw
that has been noted, from what I understand.

The old, insecure, mktemp() does NOT create a file.  All
it does is provide a temporary filename, that (at-the-time)
does not exist.  Therefore, when using it, there is a
race condition between the function returns and when the
caller decides to create the file.

mkstemp() actually creates the file and returns a file descriptor
for it (along with modifying template to the pathname of the file).
Therefore, there is no race condition and the reason one should
use mkstemp().

Where I noticed that calling code was to use the open descriptor
(either the unix fd or the standard C FILE *), I modified the code
to use what is returned from the new functions (the new functions
will call fdopen() on the descriptor if FILE* is desired).

If the calling code did not immediately use the temp file,
the new functions close the descriptor returned from mkstemp(),
but it does NOT delete the file.

Since the file still exists, an external (different uid) process
cannot create one in its place, so the race condition vulnerability
does not exist.  The file is just sitting there.

When the nmh code gets around to using the temporary file, the call
to open/fopen basically re-opens the file.  This does increase the
window that if the main program is terminated abnormally, a temporary