From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27934 invoked by alias); 18 Aug 2014 00:15:45 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Received: (qmail 27867 invoked by uid 89); 18 Aug 2014 00:15:36 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,SPF_PASS,UNPARSEABLE_RELAY autolearn=ham version=3.3.2 X-HELO: aibo.runbox.com Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 From: "David A. Wheeler" Reply-To: dwheeler@dwheeler.com To: "libc-alpha" Subject: Re: Implement C11 annex K? Date: Mon, 18 Aug 2014 00:15:00 -0000 In-Reply-To: <53EE83D0.1070703@cs.ucla.edu> Message-Id: X-SW-Source: 2014-08/txt/msg00274.txt.bz2 > David A. Wheeler wrote: > > Could you please post your empirical data? On Fri, 15 Aug 2014 15:04:00 -0700, Paul Eggert gracio= usly replied: > I'm afraid I long ago discarded the raw data, but you should be able to=20 > reproduce it by following the recipe outlined in=20 > and by=20 > looking at the first few grep hits. Sorry, I didn't note down the=20 > working environment, but it was either C or en_US (either Latin-1 or=20 > 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 late= r). It has not. There are 140 instances of the terms strlcpy or strlcat, but t= hat 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 t= o strlcpy, but it obscures the purpose. snprintf is no substitute for strlcat in gene= ral, though in some cases it works okay. If strlcpy encourages sloppy programming, we ought to ban snprintf too :-). addrmatch.c:321: if (p =3D=3D NULL || strlcpy(addrbuf, p, sizeof(addrbuf)) >=3D sizeof(addr= buf)) 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 =3D=3D NULL || (len =3D snprintf(addrbuf, sizeof(addrbuf), "%s", p),= len < 0 || len >=3D sizeof(addrbuf))) return -1; So the likely result of eliminating strlcpy is many more lines & complexit= y: if (p =3D=3D NULL) return -1; len =3D snprintf(addrbuf, sizeof(addrbuf), "%s", p); if (len < 0 || len >=3D 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) =3D=3D 1)) return pub; Here we have the first use of strlcat.=20 The obvious snprintf alternative is: len =3D snprintf(file, sizeof(file), "%s.pub", filename); if (len >=3D 0 && len < sizeof(file) && (key_try_load_public(pub, file, commentp) =3D=3D 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 =3D strlen(msg); ... len =3D plen + mlen + 1; **prompts =3D xrealloc(**prompts, 1, len); strlcpy(**prompts + plen, msg, len - plen); plen +=3D 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 =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D addrmatch.c:321: if (p =3D=3D NULL || strlcpy(addrbuf, p, sizeof(addrbuf)) = >=3D 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)= ) >=3D 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->pr= ogname)); 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_logoutt= ime)); 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 =3D 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_na= me)); 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.u= e_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) >=3D= servlen) openbsd-compat/fake-rfc2553.c:64: if (strlcpy(host, inet_ntoa(sin->sin_ad= dr), openbsd-compat/fake-rfc2553.c:75: if (strlcpy(host, hp->h_name, hostlen) = >=3D 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 *sr= c, size_t siz); openbsd-compat/openbsd-compat.h:80:size_t strlcat(char *dst, const char *sr= c, size_t siz); openbsd-compat/port-aix.c:420: strlcpy(host, "::", hostlen); openbsd-compat/port-linux.c:210: strlcpy(newctx + len, newname, newlen - le= n); openbsd-compat/port-linux.c:212: strlcat(newctx, cx, newlen); openbsd-compat/realpath.c:133: resolved_len =3D strlcat(resolved, next_tok= en, PATH_MAX); openbsd-compat/realpath.c:179: left_len =3D strlcat(symlink, left, sizeo= f(left)); openbsd-compat/realpath.c:185: left_len =3D strlcpy(left, symlink, sizeof= (left)); openbsd-compat/realpath.c:69: left_len =3D strlcpy(left, path + 1, sizeof(= left)); openbsd-compat/realpath.c:72: strlcpy(resolved, ".", PATH_MAX); openbsd-compat/realpath.c:76: left_len =3D strlcpy(left, path, sizeof(left= )); openbsd-compat/setproctitle.c:140: strlcpy(buf, __progname, sizeof(buf)); openbsd-compat/setproctitle.c:145: len =3D strlcat(buf, ": ", sizeof(buf)); openbsd-compat/setproctitle.c:161: len =3D strlcpy(argv_start, ptitle, argv= _env_len); openbsd-compat/strlcat.c:1:/* $OpenBSD: strlcat.c,v 1.13 2005/08/08 08:05:3= 7 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:3= 8 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)) >=3D 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, MAXPA= THLEN); 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) >=3D 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)) >= =3D ssh-keygen.c:1038: if (strlcpy(tmp, identity_file, sizeof(tmp)) >=3D sizeo= f(tmp) || ssh-keygen.c:1039: strlcat(tmp, ".XXXXXXXXXX", sizeof(tmp)) >=3D sizeo= f(tmp) || ssh-keygen.c:1040: strlcpy(old, identity_file, sizeof(old)) >=3D sizeo= f(old) || ssh-keygen.c:1041: strlcat(old, ".old", sizeof(old)) >=3D sizeof(old)) ssh-keygen.c:1394: strlcpy(new_comment, identity_comment, sizeof(new_comme= nt)); ssh-keygen.c:1421: strlcat(identity_file, ".pub", sizeof(identity_file)); ssh-keygen.c:2320: if (strlcpy(identity_file, optarg, sizeof(identity_fil= e)) >=3D ssh-keygen.c:2413: if (strlcpy(out_file, optarg, sizeof(out_file)) >=3D ssh-keygen.c:2419: if (strlcpy(out_file, optarg, sizeof(out_file)) >=3D 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(identit= y_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);