linux-kernel - Re: [PATCH] locomo.c: convert strncpy(x, y, sizeof(x)) to strlcpy

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] [day] [month] [year] [list]

Message-ID: <fqq73q$21t$1@taverner.cs.berkeley.edu>
Date:	Fri, 7 Mar 2008 01:51:54 +0000 (UTC)
From:	daw@...berkeley.edu (David Wagner)
To:	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] locomo.c: convert strncpy(x, y, sizeof(x)) to strlcpy

Roel Kluin  wrote:
>As I understand it, please correct me if I'm wrong:
>
>Of the three variants: strcpy, strncpy and strlcpy.
>- strcpy does not append \0 (unless the source string already contained it)

No, that's not correct.  If the source string has no \0, then
strcpy keeps reading forever and things go horribly awry.

>In the original code strncpy was used and the size parameter was equal
>to the source string size:
>
>strncpy(dev->dev.bus_id, info->name, sizeof(dev->dev.bus_id));
>
>Since this the size was equal there was no \0 termination. To \0
>terminate using strncpy we could write:
>
>strncpy(dev->dev.bus_id, info->name, sizeof(dev->dev.bus_id) - 1);
>dev->dev.bus_id[sizeof(dev->dev.bus_id) - 1] = '\0';
>
>or using strlcpy, which does the same thing:
>
>strlcpy(dev->dev.bus_id, info->name, sizeof(dev->dev.bus_id));

No, strlcpy() does not do the same thing.  As several people have pointed
out to you, strlcpy() does not fill the rest of the buffer with \0's.
This can create an information leak in some cases, which can create a
security hole.  The difference is important in some cases.

Second, you seem to implicitly assume that it's a bug to leave
dev->dev.bus_id without any \0 termination.  You'd have to look at the API
to determine whether that assumption is accurate.  And then you'd have to
think carefully about what is the proper behavior if info->name is too
long to fit into the buffer.  Is truncating the string the appropriate
behavior?  In some cases prematurely truncating a string is bad news and
can create a security bug, and the proper thing to do in some cases may
be to return an error rather than returning a truncated string.

Rant: Blindly search-and-replacing strncpy() with strlcpy() is not safe,
in general.  You have to actually understand what the code is doing and
what it should be doing.

I'm not claiming that your patch is correct, nor that it is incorrect.
Instead, I'm suggesting that you probably need to do some analysis to
understand the specific requirements and context in each of these cases,
rather than doing a mechanically search-and-replace.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/