linux-kernel - Re: [V9fs-developer] [PATCH v2] net/9p/trans_virtio.c: add null terminal for mount tag

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]

Date:   Fri, 3 Aug 2018 04:06:08 +0200
From:   Dominique Martinet <asmadeus@...ewreck.org>
To:     piaojun <piaojun@...wei.com>
Cc:     Greg Kurz <groug@...d.org>, Latchesar Ionkov <lucho@...kov.net>,
        Eric Van Hensbergen <ericvh@...il.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        v9fs-developer@...ts.sourceforge.net,
        Ron Minnich <rminnich@...dia.gov>
Subject: Re: [V9fs-developer] [PATCH v2] net/9p/trans_virtio.c: add null
 terminal for mount tag

piaojun wrote on Fri, Aug 03, 2018:
> We'd better reach an agreement about the patch fix.

The way I read Greg's comment was that he agreed to the proposed changes
and is waiting for a new version.

I'm writing a longer reply than I should because I don't like people
saying strncmp is safe just because it's strncmp, feel free to skim
through the rest as it's just ranting.

> In my opinion, replacing strlen(chan->tag) with a local variable
> sounds reasonable,

So we agree here

> and changing strncmp to strcmp may be little beneficial, as strcmp is more
> dangerours such as buffer-flow.

strcmp is more dangerous for buffer-overflow if you're comparing
"unsafe" non-null terminated strings.
This isn't the case here as you've constructed chan->tag yourself and
you rely on it being null-terminated by calling strlen() on it yourself.

strncmp(x, y, strlen(y)+1) is at best awkward, but it's a false sense of
security if you think this is any better than strcmp here. It implies that:
 - y is null-terminated (for strlen() to work)
 - x is either null-terminated or longer than y

Here, x is the devname argument to p9_virtio_create, which comes
straight from the mount syscall with "copy_mount_string", using
strndup_user, which returns a null-terminated string or an error.

(the code is currently not safe if it returns an error, I'm sending
another mail about it right after this one as we already have a partial
fix)

strcmp(x, y) on the other hand assumes that x and y are null-terminated
in this case, which is the same assumptions you have, so is strictly as
"safe" as strncmp used that way.
(it could also assume that one is null terminated and the other one is
at least as long as that, e.g. when comparing a "char buf[42]" to the
constant "foo" then we don't care if buf is null-terminated)

Thanks,
-- 
Dominique