On Wed, Jun 26, 2019 at 12:21:49PM -0400, J. Bruce Fields wrote: > On Mon, Jun 24, 2019 at 05:05:12PM -0400, J. Bruce Fields wrote: > > On Sat, Jun 22, 2019 at 01:22:56PM -0700, Kees Cook wrote: > > > On Sat, Jun 22, 2019 at 03:00:58PM -0400, J. Bruce Fields wrote: > > > > The logic around ESCAPE_NP and the "only" string is really confusing. I > > > > started assuming I could just add an ESCAPE_NONASCII flag and stick " > > > > and \ into the "only" string, but it doesn't work that way. > > > > > > Yeah, if ESCAPE_NP isn't specified, the "only" characters are passed > > > through. It'd be nice to have an "add" or a clearer way to do actual > > > ctype subsets, etc. If there isn't an obviously clear way to refactor > > > it, just skip it for now and I'm happy to ack your original patch. :) > > > > There may well be some simplification possible here.... There aren't > > really many users of "only", for example. I'll look into it some more. > > The printk users are kind of mysterious to me. I did a grep for > > git grep '%[0-9.*]pE' > > which got 75 hits. All of them for pE. I couldn't find any of the > other pE[achnops] variants. pE is equivalent to ESCAPE_ANY|ESCAPE_NP. I saw pEn and pEhp and pEp: drivers/staging/rtl8192e/rtllib.h: snprintf(escaped, sizeof(escaped), "%*pEn", essid_len, essid); drivers/staging/rtl8192u/ieee80211/ieee80211.h: snprintf(escaped, sizeof(escaped), "%*pEn", essid_len, essid); drivers/staging/wlan-ng/prism2sta.c: netdev_info(wlandev->netdev, "Prism2 card SN: %*pEhp\n", drivers/thunderbolt/xdomain.c: return sprintf(buf, "%*pEp\n", (int)strlen(svc->key), svc->key); However, every use was insufficient, AFAICT. This: git grep -2 '\bescape_essid\b' Shows that all the staging uses end up getting logged as: '%s' so their escaping is insufficient. > Confusingly, ESCAPE_NP doesn't mean "escape non-printable", it means > "don't escape printable". So things like carriage returns aren't > escaped. Right -- any they're almost all logged surrounded by ' or " which means those would need to be escaped as well. The prism2 is leaking newlines too, as well as the thunderbolt sysfs printing. So... seems like we should fix this. :P > Of those 57 were in drivers/net/wireless, and from a quick check seemed > mostly to be for SSIDs in debug messages. I *think* SSIDs can be > arbitrary bytes? If they really want them escaped then I suspect they > want more than just nonprintable characters escaped. > > One of the hits outside wireless code was in drm_dp_cec_adap_status, > which was printing some device ID into a debugfs file with "ID: %*pE\n". > If the ID actually needs escaping, then I suspect the meant to escape \n > too to prevent misparsing that output. I think we need to make the default produce "loggable" output. non-ascii, non-printables, \, ', and " need to be escaped. Maybe " " too? -- Kees Cook
