bugThe nmh Mail Handling System - Bugs: bug #56192, Unreadable mts.conf silently...

 
 

bug #56192: Unreadable mts.conf silently ignored; more general checks required

Submitted by:  Ralph Corderoy <ralph>
Submitted on:  Mon 22 Apr 2019 09:54:26 AM UTC  
 
Category:  Bug Severity:  3 - Normal
Priority:  5 - Normal Status:  None
Assigned to:  None Open/Closed:  Open

Add a New Comment (Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

Wed 24 Apr 2019 12:30:19 AM UTC, comment #4: 

I like it.  A few minor things:  you said it wasn't compiled successfully.  But I couldn't even apply the patch without hacking around a reject of this:
-static char *mtsconf = NMHETCDIR "/mts.conf";
Looks like that should be:
-static char *mtsconf = nmhetcdir(/mts.conf);

The first call to get_optional_env_var() in mts_init() needs its result assigned to `path'.  Or, embed the calls; that might address the following comment, too . . .

The succeeding call to read_mts() isn't protected against a NULL arg, but the following one is.  That corresponds to the second args to get_optional_env_var().  Might that deserve a comment so readers don't have to trace through the called function?

David Levine <levine>
Project Member
Tue 23 Apr 2019 10:55:04 AM UTC, comment #3: 

How about the attached patch, not even compiled successfully here.
It assumes an environment variable set to the empty string is still worthy of attempting to open, i.e. the user will get an error.  I think this is correct as it suggests the user is attempting to set the variable but getting it wrong.  However, it's a change in behaviour.

(file #46812)

Ralph Corderoy <ralph>
Project Member
Tue 23 Apr 2019 12:50:46 AM UTC, comment #2: 

Of course, remove the return after the die().

David Levine <levine>
Project Member
Mon 22 Apr 2019 11:04:56 PM UTC, comment #1: 

Is this sufficient for mts.conf?  The good news is that it's opened in one place, in mts_init().  The bad news is that the function doesn't return a value, so callers can't handle an open failure.

-    if (inited++ || (fp = fopen (get_mtsconf_pathname(), "r")) == NULL)
-       return;
+    if (inited++ || (fp = fopen ((cp = get_mtsconf_pathname()), "r")) == NULL) {
+        die("failed to open %s", cp);
+        return;
+    }

David Levine <levine>
Project Member
Mon 22 Apr 2019 09:54:26 AM UTC, original submission:  

Recent nmh-workers discussion showed a 0600 /etc/nmh/mts.conf,
unreadable by the non-root user, was silently ignored.
http://lists.nongnu.org/archive/html/nmh-workers/2019-04/msg00019.html

In general, nmh needs to tighten up its checking of reading
configuration files.

  • If the file must exist then it is an error if it doesn't.
  • If the file does exist then it is an error if it can't be opened, read

  as required, and closed.

  • If an nmh environment variable is defined to be a path to a file,

  e.g. MHMTSCONF, then the above applies.

Ralph Corderoy <ralph>
Project Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #46812:  nmh-sbr-mts.c.patch added by ralph (3KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by levine (Posted a comment)
  • -email is unavailable- added by ralph (Submitted the item)
  •  

     

     

    Follows 1 latest change.

    Date Changed by Updated Field Previous Value => Replaced by
    2019-04-23 ralph Attached File- => Added nmh-sbr-mts.c.patch, #46812

    Back to the top


    Powered by Savane 3.5