From grdetil@scrc.umanitoba.ca Wed Dec 23 09:15:35 1998 Date: Tue, 22 Dec 1998 12:30:42 -0600 (CST) From: Gilles Detillieux To: Geoff Hutchison Cc: grdetil@scrc.umanitoba.ca, htdig3-dev@wso.williams.edu Subject: Re: htdig3-dev Security hole in htnotify Hi, Geoff & company... > > Another problem I can see with this patch is that characters like "%", > > ":", and "!" are valid in e-mail addresses, but get changed to "_" by > > the patch. I think as long as you use "-t" on the command line, and > > DON'T put the e-mail addresses on the command line, you don't need to > > worry about what's valid and what's not - sendmail should be able to > > safely parse the "To:" header on its own. > > Fair enough. Send me a patch that you like better and I'll use that one. > While I can certainly see the problem, I'm not versed in sending commands > through C/C++ so I can't see easy fixes. I clearly need to read up on > spawning processes. OK, my patch is below. I wasn't sure what to diff against, so I diff'ed against the original 3.1.0b3 source. So, the patch also includes the memory leak fix, which I'm sure you've already put in. There was actually another bug in the old code. The strtok() function would break up the "to" string, so only the first e-mail address appeared in the "To:" header, when multiple recipients were specified. I now use an intermediate string to avoid that, and build a "To:" header with the separators that sendmail expects. I left the -F & -f arguments, because I like having those for the envelope as well as the headers, but now I quote the htnotify_sender argument in case it has characters the shell doesn't like. This isn't something the document can override, so it's relatively safe. I left in your ok_chars code, with a slightly expanded list of valid characters, but I left it commented out as I really don't see the need if the addresses are only passed to sendmail via the "To:" header. > > for the same security issue. Too bad I didn't figure that out back then > > and tell you about it. Now, I can't even find his source files anywhere, > > so I don't know exactly what his patch did. I've e-mailed him about it, > > Groan. I also looked for this problem in the CVS tree. (CVSweb from > is very useful.) It seemed to be there from the > beginning, despite a massive patch from 3.0.8b2 -> 3.1.0b1. The change from 3.0.8 to 3.1.0 is what made it much less secure. Before, it exec'ed sendmail directly, passing it each address as a separate argument. There was a lot less chance of doing anything nasty back then. With the switch to popen(), which uses the shell to interpret the command string, it opened the door to the possibility of tricking the shell into interpretting other commands, especially because the e-mail address arguments weren't quoted. > > The 3.0.8 version of htnotify didn't use popen() to run sendmail, though, > > so it wasn't as vulnerable to monkey business with the e-mail addresses. > > It called execv directly, after building up the argument vector. With > > popen(), at the very least you should quote any arguments that you can't > > personally vouch for. > > OK, then I'd say we probably need to poke around a few other files looking > for similar problems. I doubt there's anything quite so bad, but I'd look > at htmerge/words.cc, htdig/PDF.cc, and htcommon/DocumentDB.cc. I've > already looked through the cgi.cc file and that seems OK (I'm going to > print out a copy and go through with a fine-tooth comb tonight.) htdig/ExternalParser.cc calls popen, and passes some unquoted arguments, some of which come from the HTTP server and its documents. I'd take a closer look there. (I'm running out of time to do it myself, though.) The main thing to remember is that system() and popen() pass their string to the shell, so anything that the user provides that goes into that string should be scrutinized and/or quoted. You don't want to allow the user to specify unintended (by you) file redirections or extra commands. Another possible security concern: htsearch lets the user use config=../../whatever.conf, which could potentially allow access to stuff you don't want the user to look at. It may be worth disallowing or stripping out all "../" occurrences. > P.S. I'm turning up a variety of memory leaks too. I haven't hit htdig/* > but that's my next area. Here's my patch: --- htnotify/htnotify.cc.security Tue Dec 15 10:58:13 1998 +++ htnotify/htnotify.cc Tue Dec 22 11:51:45 1998 @@ -138,6 +138,7 @@ { ref = docdb[str->get()]; htnotify(*ref); + delete ref; } delete docs; docdb.Close(); @@ -211,27 +212,41 @@ // void send_notification(char *date, char *email, char *url, char *subject) { - /* Currently unused int fildes[2]; */ - String to = email; - - String command = SENDMAIL; - command << " -F \"ht://Dig Notification Service\" -f "; - command << config["htnotify_sender"]; - - char *token = strtok(to, " ,\t\r\n"); + String command = SENDMAIL; + command << " -t -F \"ht://Dig Notification Service\" -f \""; + command << config["htnotify_sender"] << '"'; + + String em = email; + String to = ""; + char *token = strtok(em.get(), " ,\t\r\n"); while (token) { - command << " " << token; - token = strtok(0, " ,\t\r\n"); + if (*token) + { + if (to.length()) + to << ", "; + to << token; + } + token = strtok(0, " ,\t\r\n"); } + + // Before we use the email address string, we may want to sanitize it. +// static char ok_chars[] = "abcdefghijklmnopqrstuvwxyz\ +//ABCDEFGHIJKLMNOPQRSTUVWXYZ\ +//1234567890_-.@/=:%!, "; +// char *cursor; // cursor into email address + +// for (cursor = to.get(); *(cursor += strspn(cursor, ok_chars));) +// *cursor = '_'; // Set it to something harmless FILE *fileptr; - if( (fileptr = popen(command.get(), "w")) != NULL ) { + if ( (fileptr = popen(command.get(), "w")) != NULL ) { if (!subject || !*subject) - subject = "notification"; + subject = "page expired"; String out; - out << "From: " << config["htnotify_sender"] << "\n"; + out << "From: ht://Dig Notification Service <" + << config["htnotify_sender"] << ">\n"; out << "Subject: WWW notification: " << subject << '\n'; out << "To: " << to.get() << '\n'; out << "Reply-To: " << config["htnotify_sender"] << "\n"; -- Gilles R. Detillieux E-mail: Spinal Cord Research Centre WWW: http://www.scrc.umanitoba.ca/~grdetil Dept. Physiology, U. of Manitoba Phone: (204)789-3766 Winnipeg, MB R3E 3J7 (Canada) Fax: (204)789-3930