Re: Implement C11 annex K?

["David A. Wheeler" <dwheeler at dwheeler dot com>]

> David A. Wheeler wrote:
> > Could you please post your empirical data?

On Fri, 15 Aug 2014 15:04:00 -0700, Paul Eggert <eggert@cs.ucla.edu> graciously replied:
> I'm afraid I long ago discarded the raw data, but you should be able to 
> reproduce it by following the recipe outlined in 
> <https://www.sourceware.org/ml/libc-alpha/2002-01/msg00159.html> and by 
> looking at the first few grep hits.  Sorry, I didn't note down the 
> working environment, but it was either C or en_US (either Latin-1 or 
> UTF-8) and it was run on either GNU/Linux or Solaris at the time.

Thanks very much for the URL!!
I downloaded "openssh-6.6p1" (the current version) and ran:
  egrep -n 'strlcpy|strlcat' `find * -type f -name '*.[ch]' -print

If strlcpy/strlcat were truly security disasters, or unhelpful,
you'd expect their use in OpenSSH to have disappeared by now (12 years later).
It has not.  There are 140 instances of the terms strlcpy or strlcat, but that includes
5 inside comments.  Thus, there were 135 active uses of strlcpy or strlcat.
So... there are clearly people who care about security and
*specifically* use strlcpy/strlcat.

A few instances and discussion are below.  I don't have time for
a serious security analysis (and I don't think anyone else did so earlier),
so these are more "quick impressions" than anything else.
In many cases the lengths were checked; in many others
they were not (allowing truncation but still countering buffer overflow).

But first, a bottom line from my viewpoint.
If you are *checking* the results of simple
copies or concats, strlcpy/strlcat win easily (same for strcpy_s/strcat_s).
If you aren't checking and using strlcpy, snprintf isn't too bad compared to strlcpy,
but it obscures the purpose.  snprintf is no substitute for strlcat in general,
though in some cases it works okay.
If strlcpy encourages sloppy programming, we ought to ban snprintf too :-).


addrmatch.c:321:
 if (p == NULL || strlcpy(addrbuf, p, sizeof(addrbuf)) >= sizeof(addrbuf))
   return -1;

 This strlcpy use seems reasonable.  If the buffer is exceeded, the routine
 returns a parse error.  You *do* want to check for buffer excess here,
 since it's user input.  Nice and simple. Great!

 The one-line snprintf version is this horror:
 if (p == NULL || (len = snprintf(addrbuf, sizeof(addrbuf), "%s", p), len < 0 || len >= sizeof(addrbuf)))
   return -1;

 So the likely result of eliminating strlcpy is many more lines & complexity:
 if (p == NULL)
   return -1;
 len = snprintf(addrbuf, sizeof(addrbuf), "%s", p);
 if (len < 0 || len >= sizeof(addrbuf))
   return -1;

 You have to define "int len;" for both snprintf uses, in addition.

 Strlcpy wins.



auth.c:486:
 strlcpy(buf, cp, sizeof(buf));

 This is part of auth_secure_path(); it's processing a dirname() of a buf
 of length MAXPATHLEN that was originally filled by realpath().
 The realpath() function itself is broken by design (you can't really
 know how long a buffer to allocate yet it provides no control over
 its length). Thus, I'm going to assume that this is passed
 in by the user who invoked this in the first
 place (and thus we're not leaving a trust boundary, we're just
 doing sanity-checking).

 So.. do you really believe that MAXPATHLEN really is the max length?
 If you believe that, then it can't be exceeded, and thus there's no
 need to worry about truncation (and indeed, this code doesn't try to
 detect truncation).  The real problem is that MAXPATHLEN is not
 necessarily a constant, but you'd hit problems earlier in that case.
 So the strlcpy is probably not strictly necessary (if you believe
 in MAXPATHLEN) but harmless.

 But here's a philosophical difference. I'd rather people use limiting
 functions instead of requiring deep analysis to figure out if there's
 a potential buffer overflow.  It's belt-and-suspenders; adding an
 extra check costs nearly nothing in performance time, but now the analyst
 no longer has to determine if the buffer can be overwritten (it can't).
 If someone later replaces realpath() with a sensible function, the
 strlcpy() will prevent an easy buffer overflow... making debugging
 much simpler.

 Strlcpy slightly wins.


authfd.c:107:
 strlcpy(sunaddr.sun_path, authsocket, sizeof(sunaddr.sun_path));

 Here strlcpy is used to copy a path to sun_path.
 Truncation isn't checked... but it's not clear what else you
 could do when truncation occurs.  sun_path is defined
 to be a fixed length that the application program cannot change.
 Chopping it to a fixed length is a reasonable approach, since
 you really can't do much else.  An snprintf wouldn't be too bad
 here, but it would obscure the fact that it's a simple string copy:
 snprintf(sunaddr.sun_path, sizeof(sunaddr.sun_path), "%s", authsocket);

 Strlcpy slightly wins, but snprintf is reasonable enough.


authfile.c:1179-1180:
  if ((strlcpy(file, filename, sizeof file) < sizeof(file)) &&
      (strlcat(file, ".pub", sizeof file) < sizeof(file)) &&
      (key_try_load_public(pub, file, commentp) == 1))
      return pub;

  Here we have the first use of strlcat. 
  The obvious snprintf alternative is:

  len = snprintf(file, sizeof(file), "%s.pub", filename);
  if (len >= 0 && len < sizeof(file) &&
      (key_try_load_public(pub, file, commentp) == 1))
      return pub;

  I think the snprint version is cleaner to read, so I'd prefer it.
  However, note that in this case the "Schlemiel argument" is basically
  irrelevant. Yes, the strlcat version invokes Schlemiel,
  but we're talking filenames, so Schlemiel will be pretty quick,
  while setting up format processing is heavyweight.

  I would use snprintf in this case... but that's not an
  argument against strlcat itself.


auth-pam.c:742:
  mlen = strlen(msg);
  ...
  len = plen + mlen + 1;
  **prompts = xrealloc(**prompts, 1, len);
  strlcpy(**prompts + plen, msg, len - plen);
  plen += mlen;

  On a quick look, this appears to (re)allocate the necessary space,
  and then copy a string in.  It *might* be possible to substitute strcpy
  instead... but why in the world would I *WANT* to do that!?

  I think there is a philosophical difference here.  If you make a mistake,
  what is the ramification?  If you use functions that are designed to
  limit damage, then the mistakes you inevitably make are less likely
  to hurt your users.

  Advantage strlcpy, due to a philosophical preference for code that
  self-defends.


I completely agree that silent truncation can cause serious problems.
But silent truncation is typically *way* better than buffer overflows.

Below is the complete output from:
 egrep -n 'strlcpy|strlcat' `find * -type f -name '*.[ch]' -print` | sort


--- David A. Wheeler


==================================================


addrmatch.c:321:	if (p == NULL || strlcpy(addrbuf, p, sizeof(addrbuf)) >= sizeof(addrbuf))
auth.c:486:		strlcpy(buf, cp, sizeof(buf));
authfd.c:107:	strlcpy(sunaddr.sun_path, authsocket, sizeof(sunaddr.sun_path));
authfile.c:1179:	if ((strlcpy(file, filename, sizeof file) < sizeof(file)) &&
authfile.c:1180:	    (strlcat(file, ".pub", sizeof file) < sizeof(file)) &&
auth-pam.c:742:			strlcpy(**prompts + plen, msg, len - plen);
auth-pam.c:752:			strlcpy(**prompts + plen, msg, len - plen);
auth-pam.c:754:			strlcat(**prompts + plen, "\n", len - plen);
auth-rhosts.c:110:			strlcpy(userbuf, server_user, sizeof(userbuf));
channels.c:1084:	strlcpy(username, p, sizeof(username));
channels.c:3542:	strlcpy(addr.sun_path, pathname, sizeof addr.sun_path);
channels.c:3614:	strlcpy(buf, display, sizeof(buf));
clientloop.c:412:		strlcpy(proto, SSH_X11_PROTO, sizeof proto);
entropy.c:101:		strlcpy(addr_un->sun_path, socket_path,
key.c:437:		strlcat(retval, hex, dgst_raw_len * 3 + 1);
loginrec.c:1230:			strlcpy(li->hostname, ut.ut_host,
loginrec.c:1392:			strlcpy(li->hostname, utx.ut_host,
loginrec.c:1489:		strlcpy(lastlog_file, LASTLOG_FILE, sizeof(lastlog_file));
loginrec.c:1543:		strlcpy(last.ll_host, li->hostname,
loginrec.c:1578:	strlcpy(li->hostname, ll->ll_host,
loginrec.c:1603:		strlcpy(li->hostname, last.ll_host,
loginrec.c:1640:	strlcpy(li->hostname, utx->ut_host,
loginrec.c:1691:	strlcpy(ut.ut_line, "ssh:notty", sizeof(ut.ut_line));
loginrec.c:313:	if (strlcpy(li->username, pw->pw_name, sizeof(li->username)) >=
loginrec.c:380:		strlcpy(li->username, username, sizeof(li->username));
loginrec.c:390:		strlcpy(li->hostname, hostname, sizeof(li->hostname));
loginrec.c:564:		strlcpy(dst, src, dstsize);
loginrec.c:566:		strlcpy(dst, "/dev/", dstsize);
loginrec.c:567:		strlcat(dst, src, dstsize);
loginrec.c:578:		strlcpy(dst, src + 5, dstsize);
loginrec.c:580:		strlcpy(dst, src, dstsize);
loginrec.c:614:		/* note: _don't_ change this to strlcpy */
logintest.c:101:	strlcpy(username, pw->pw_name, sizeof(username));
logintest.c:109:	strlcpy(li1->progname, "OpenSSH-logintest", sizeof(li1->progname));
logintest.c:123:		strlcpy(li1->hostname, "localhost", sizeof(li1->hostname));
logintest.c:144:	strlcpy(s_t0, ctime(&t0), sizeof(s_t0));
logintest.c:146:	strlcpy(s_t1, ctime(&t1), sizeof(s_t1));
logintest.c:155:	strlcpy(s_logintime, ctime(&logintime), sizeof(s_logintime));
logintest.c:171:	strlcpy(s_logouttime, ctime(&logouttime), sizeof(s_logouttime));
logintest.c:186:	strlcpy(s_t2, ctime(&t2), sizeof(s_t2));
md5crypt.c:145:	strlcat(passwd, to64(l, 4), sizeof(passwd));
md5crypt.c:147:	strlcat(passwd, to64(l, 4), sizeof(passwd));
md5crypt.c:149:	strlcat(passwd, to64(l, 4), sizeof(passwd));
md5crypt.c:151:	strlcat(passwd, to64(l, 4), sizeof(passwd));
md5crypt.c:153:	strlcat(passwd, to64(l, 4), sizeof(passwd));
md5crypt.c:155:	strlcat(passwd, to64(l, 2), sizeof(passwd));
misc.c:608:				i = strlcat(buf, keys[j].repl, sizeof(buf));
misc.c:754:		strlcat(r, b, hl);
monitor_wrap.c:736:	strlcpy(namebuf, p, namebuflen); /* Possible truncation */
mux.c:1171:	if (strlcpy(addr.sun_path, options.control_path,
mux.c:2029:	if (strlcpy(addr.sun_path, path,
openbsd-compat/bsd-cray.c:226:	strlcpy(hostname,
openbsd-compat/bsd-cray.c:322:		strlcpy(ue.ue_name, "root", sizeof(ue.ue_name));
openbsd-compat/bsd-cray.c:323:		strlcpy(ue.ue_dir, "/", sizeof(ue.ue_dir));
openbsd-compat/bsd-cray.c:324:		strlcpy(ue.ue_shell, "/bin/sh", sizeof(ue.ue_shell));
openbsd-compat/bsd-cray.c:504:					strlcpy(acct_name, acid2nam(valid_acct), MAXACID);
openbsd-compat/fake-rfc2553.c:58:		if (strlcpy(serv, tmpserv, servlen) >= servlen)
openbsd-compat/fake-rfc2553.c:64:			if (strlcpy(host, inet_ntoa(sin->sin_addr),
openbsd-compat/fake-rfc2553.c:75:			if (strlcpy(host, hp->h_name, hostlen) >= hostlen)
openbsd-compat/fmt_scaled.c:232:		strlcpy(result, "0B", FMT_SCALED_STRSIZE);
openbsd-compat/glob.c:988:		strlcpy(buf, ".", sizeof buf);
openbsd-compat/inet_ntop.c:207:	strlcpy(dst, tmp, size);
openbsd-compat/inet_ntop.c:97:	strlcpy(dst, tmp, size);
openbsd-compat/openbsd-compat.h:75:size_t strlcpy(char *dst, const char *src, size_t siz);
openbsd-compat/openbsd-compat.h:80:size_t strlcat(char *dst, const char *src, size_t siz);
openbsd-compat/port-aix.c:420:			strlcpy(host, "::", hostlen);
openbsd-compat/port-linux.c:210:	strlcpy(newctx + len, newname, newlen - len);
openbsd-compat/port-linux.c:212:		strlcat(newctx, cx, newlen);
openbsd-compat/realpath.c:133:		resolved_len = strlcat(resolved, next_token, PATH_MAX);
openbsd-compat/realpath.c:179:				left_len = strlcat(symlink, left, sizeof(left));
openbsd-compat/realpath.c:185:			left_len = strlcpy(left, symlink, sizeof(left));
openbsd-compat/realpath.c:69:		left_len = strlcpy(left, path + 1, sizeof(left));
openbsd-compat/realpath.c:72:			strlcpy(resolved, ".", PATH_MAX);
openbsd-compat/realpath.c:76:		left_len = strlcpy(left, path, sizeof(left));
openbsd-compat/setproctitle.c:140:	strlcpy(buf, __progname, sizeof(buf));
openbsd-compat/setproctitle.c:145:		len = strlcat(buf, ": ", sizeof(buf));
openbsd-compat/setproctitle.c:161:	len = strlcpy(argv_start, ptitle, argv_env_len);
openbsd-compat/strlcat.c:1:/*	$OpenBSD: strlcat.c,v 1.13 2005/08/08 08:05:37 espie Exp $	*/
openbsd-compat/strlcat.c:19:/* OPENBSD ORIGINAL: lib/libc/string/strlcat.c */
openbsd-compat/strlcat.c:35:strlcat(char *dst, const char *src, size_t siz)
openbsd-compat/strlcpy.c:1:/*	$OpenBSD: strlcpy.c,v 1.11 2006/05/05 15:27:38 millert Exp $	*/
openbsd-compat/strlcpy.c:19:/* OPENBSD ORIGINAL: lib/libc/string/strlcpy.c */
openbsd-compat/strlcpy.c:33:strlcpy(char *dst, const char *src, size_t siz)
progressmeter.c:185:	strlcat(buf, " ", win_size);
progressmeter.c:190:	strlcat(buf, "/s ", win_size);
progressmeter.c:199:		strlcat(buf, "- stalled -", win_size);
progressmeter.c:201:		strlcat(buf, "  --:-- ETA", win_size);
progressmeter.c:221:			strlcat(buf, " ETA", win_size);
progressmeter.c:223:			strlcat(buf, "    ", win_size);
readconf.c:1118:			strlcpy(fwdarg, arg, sizeof(fwdarg));
readconf.c:526:			strlcpy(shorthost, thishost, sizeof(shorthost));
servconf.c:1359:			strlcat(p, " ", len);
servconf.c:1360:			strlcat(p, arg, len);
session.c:1470:			strlcpy(component, path, sizeof(component));
session.c:1859:		if (strlcpy(argv0 + 1, shell0, sizeof(argv0) - 1)
session.c:222:	strlcpy(sunaddr.sun_path, auth_sock_name, sizeof(sunaddr.sun_path));
session.c:2620:				strlcat(buf, ",", sizeof buf);
session.c:2621:			strlcat(buf, cp, sizeof buf);
session.c:2625:		strlcpy(buf, "notty", sizeof buf);
sftp.c:2103:			if (strlcpy(cmd, line, sizeof(cmd)) >= sizeof(cmd)) {
sftp.c:955:		strlcpy(s_used, "error", sizeof(s_used));
sftp.c:956:		strlcpy(s_avail, "error", sizeof(s_avail));
sftp.c:957:		strlcpy(s_root, "error", sizeof(s_root));
sftp.c:958:		strlcpy(s_total, "error", sizeof(s_total));
sftp-client.c:1728:	strlcpy(ret, p1, len);
sftp-client.c:1730:		strlcat(ret, "/", len);
sftp-client.c:1731:	strlcat(ret, p2, len);
sftp-glob.c:84:	strlcpy(ret->d_name, od->dir[od->offset++]->filename, MAXPATHLEN);
sftp-glob.c:86:	strlcpy(ret->d_name, od->dir[od->offset++]->filename,
sftp-server.c:255:			strlcat(ret, ",", sizeof(ret));	\
sftp-server.c:256:		strlcat(ret, str, sizeof(ret));		\
ssh.c:265:		if (strlcpy(cname, res->ai_canonname, clen) >= clen) {
ssh.c:981:	strlcpy(shorthost, thishost, sizeof(shorthost));
ssh-add.c:360:	strlcpy(prompt, "Enter lock password: ", sizeof(prompt));
ssh-add.c:363:		strlcpy(prompt, "Again: ", sizeof prompt);
ssh-agent.c:1138:		strlcpy(socket_name, agentsocket, sizeof socket_name);
ssh-agent.c:1153:	strlcpy(sunaddr.sun_path, socket_name, sizeof(sunaddr.sun_path));
sshconnect.c:1175:			strlcat(msg, "\nAre you sure you want "
sshconnect.c:1303:	strlcpy(padded, password, size);
sshconnect2.c:129:			strlcat(to, ",", maxlen); \
sshconnect2.c:130:		strlcat(to, from, maxlen); \
ssh-keygen.c:1024:		if (strlcpy(identity_file, cp, sizeof(identity_file)) >=
ssh-keygen.c:1038:		if (strlcpy(tmp, identity_file, sizeof(tmp)) >= sizeof(tmp) ||
ssh-keygen.c:1039:		    strlcat(tmp, ".XXXXXXXXXX", sizeof(tmp)) >= sizeof(tmp) ||
ssh-keygen.c:1040:		    strlcpy(old, identity_file, sizeof(old)) >= sizeof(old) ||
ssh-keygen.c:1041:		    strlcat(old, ".old", sizeof(old)) >= sizeof(old))
ssh-keygen.c:1394:		strlcpy(new_comment, identity_comment, sizeof(new_comment));
ssh-keygen.c:1421:	strlcat(identity_file, ".pub", sizeof(identity_file));
ssh-keygen.c:2320:			if (strlcpy(identity_file, optarg, sizeof(identity_file)) >=
ssh-keygen.c:2413:			if (strlcpy(out_file, optarg, sizeof(out_file)) >=
ssh-keygen.c:2419:			if (strlcpy(out_file, optarg, sizeof(out_file)) >=
ssh-keygen.c:252:		strlcpy(identity_file, buf, sizeof(identity_file));
ssh-keygen.c:2648:		strlcpy(comment, identity_comment, sizeof(comment));
ssh-keygen.c:2672:	strlcat(identity_file, ".pub", sizeof(identity_file));
ssh-keygen.c:560:		strlcat(encoded, line, sizeof(encoded));
ssh-keygen.c:931:		strlcpy(identity_file, key_types[i].path, sizeof(identity_file));
ssh-keygen.c:952:		strlcat(identity_file, ".pub", sizeof(identity_file));
sshlogin.c:78:	strlcpy(buf, li.hostname, bufsize);
sshpty.c:79:	strlcpy(namebuf, name, namebuflen);	/* possible truncation */
xmalloc.c:84:	strlcpy(cp, str, len);