linux-kernel - Re: [PATCH 08/16] nfsd: escape high characters in binary data

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <201906262100.00C1C22@keescook>
Date:   Wed, 26 Jun 2019 21:16:44 -0700
From:   Kees Cook <keescook@...omium.org>
To:     "J. Bruce Fields" <bfields@...ldses.org>
Cc:     linux-nfs@...r.kernel.org, linux-kernel@...r.kernel.org,
        Rasmus Villemoes <linux@...musvillemoes.dk>
Subject: Re: [PATCH 08/16] nfsd: escape high characters in binary data

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