LinuxLists.cc - [PATCH 3/4] security/selinux: decrement sizeof size in strncmp

[permalink] [raw]

Subject: Re: [PATCH 3/4] security/selinux: decrement sizeof size in strncmp

On Thu, 12 Nov 2009, Serge E. Hallyn wrote:

> Quoting James Morris ([email protected]):
> > On Thu, 12 Nov 2009, Julia Lawall wrote:
> >
> > > From: Julia Lawall <[email protected]>
> > >
> > > As observed by Joe Perches, sizeof of a constant string includes the
> > > trailing 0. If what is wanted is to check the initial characters of
> > > another string, this trailing 0 should not be taken into account. If an
> > > exact match is wanted, strcmp should be used instead.
> >
> > > --- a/security/selinux/hooks.c
> > > +++ b/security/selinux/hooks.c
> > > @@ -448,7 +448,7 @@ static int sb_finish_set_opts(struct sup
> > > sbsec->flags &= ~SE_SBLABELSUPP;
> > >
> > > /* Special handling for sysfs. Is genfs but also has setxattr handler*/
> > > - if (strncmp(sb->s_type->name, "sysfs", sizeof("sysfs")) == 0)
> > > + if (strncmp(sb->s_type->name, "sysfs", sizeof("sysfs") - 1) == 0)
> > > sbsec->flags |= SE_SBLABELSUPP;
> >
> > Shouldn't this be a simple strcmp() ?
>
> Yes I think so.
>
> Julia seems to be arguing that if a module introduces a new fs with
> name 'sysfs_foo' then this check should match that fs too (since
> for sysfs, sb->s_type->name = "sysfs" which also has a trailing \0,
> so for the regular sysfs her patch makes no practical difference).

If strcmp is what is wanted, I can make a patch for that.

julia

2009-11-12 16:22:12

[permalink] [raw]

Subject: Re: [PATCH 3/4] security/selinux: decrement sizeof size in strncmp

Julia Lawall wrote:
> On Thu, 12 Nov 2009, Serge E. Hallyn wrote:
>
>
>> Quoting James Morris ([email protected]):
>>
>>> On Thu, 12 Nov 2009, Julia Lawall wrote:
>>>
>>>
>>>> From: Julia Lawall <[email protected]>
>>>>
>>>> As observed by Joe Perches, sizeof of a constant string includes the
>>>> trailing 0. If what is wanted is to check the initial characters of
>>>> another string, this trailing 0 should not be taken into account. If an
>>>> exact match is wanted, strcmp should be used instead.
>>>>
>>>> --- a/security/selinux/hooks.c
>>>> +++ b/security/selinux/hooks.c
>>>> @@ -448,7 +448,7 @@ static int sb_finish_set_opts(struct sup
>>>> sbsec->flags &= ~SE_SBLABELSUPP;
>>>>
>>>> /* Special handling for sysfs. Is genfs but also has setxattr handler*/
>>>> - if (strncmp(sb->s_type->name, "sysfs", sizeof("sysfs")) == 0)
>>>> + if (strncmp(sb->s_type->name, "sysfs", sizeof("sysfs") - 1) == 0)
>>>> sbsec->flags |= SE_SBLABELSUPP;
>>>>
>>> Shouldn't this be a simple strcmp() ?
>>>
>> Yes I think so.
>>
>> Julia seems to be arguing that if a module introduces a new fs with
>> name 'sysfs_foo' then this check should match that fs too (since
>> for sysfs, sb->s_type->name = "sysfs" which also has a trailing \0,
>> so for the regular sysfs her patch makes no practical difference).
>>
>
> If strcmp is what is wanted, I can make a patch for that.
>

I strongly suggest that this is not what is wanted.
strcmp(x,y)
and
strncmp(x,y,sizeof(y))

are functionally equivalent and strcmp has a bad reputation in
the security community because it is associated with potential
buffer overrun issues.

strncmp(x,y,sizeof(y)-1)

is not the same and is more cluttered as well. This code
does not need to be changed. It does what it is intended
to do in a strait-forward manner.
> julia
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>

2009-11-12 18:27:59

[permalink] [raw]

Subject: Re: [PATCH 3/4] security/selinux: decrement sizeof size in strncmp

Casey Schaufler wrote:
> I strongly suggest that this is not what is wanted.
> strcmp(x,y)
> and
> strncmp(x,y,sizeof(y))
>
> are functionally equivalent and strcmp has a bad reputation in
> the security community because it is associated with potential
> buffer overrun issues.

It does? Hmm, I don't recall hearing of this bad reputation for strcmp().
Is there a justification for why such a reputation would be deserved?
We're not talking strcpy() here. strcmp() is fine as long as its
arguments are properly '\0'-terminated; given that, it doesn't introduce
any new buffer overrun risks.

2009-11-12 21:41:34

by James Morris

[permalink] [raw]

Subject: Re: [PATCH 3/4] security/selinux: decrement sizeof size in strncmp

On Thu, 12 Nov 2009, Casey Schaufler wrote:

> I strongly suggest that this is not what is wanted.
> strcmp(x,y)
> and
> strncmp(x,y,sizeof(y))
>
> are functionally equivalent and strcmp has a bad reputation in
> the security community because it is associated with potential
> buffer overrun issues.

Do you see potential for a buffer overrun in this case?

The strings being compared are "sysfs" and the name field of 'struct
file_system_type'. The kernel code elsewhere assumes the latter string to
be a valid zero-terminated string, and we should, too.

- James
--
James Morris
<[email protected]>

2009-11-12 21:59:54

[permalink] [raw]

Subject: Re: [PATCH 3/4] security/selinux: decrement sizeof size in strncmp

On Fri, 13 Nov 2009, James Morris wrote:

> On Thu, 12 Nov 2009, Casey Schaufler wrote:
>
> > I strongly suggest that this is not what is wanted.
> > strcmp(x,y)
> > and
> > strncmp(x,y,sizeof(y))
> >
> > are functionally equivalent and strcmp has a bad reputation in
> > the security community because it is associated with potential
> > buffer overrun issues.
>
> Do you see potential for a buffer overrun in this case?
>
> The strings being compared are "sysfs" and the name field of 'struct
> file_system_type'. The kernel code elsewhere assumes the latter string to
> be a valid zero-terminated string, and we should, too.

The sizeof only helps for the zero-termination of y, ie "sysfs". Is it
possible for the 0 at the end of an explicit constant string to get
overwritten? If it were the strncmp would be helpful, because the number
of characters to consider would be determined at compile time. If there
is some problem with the name field, the strncmp will look at least to the
end of "sysfs", so the strncmp won't help to keep the character accesses
within the valid characters of name.

julia

2009-11-12 23:57:03

[permalink] [raw]

Subject: Re: [PATCH 3/4] security/selinux: decrement sizeof size in strncmp

Julia Lawall wrote:
> Is it possible for the 0 at the end of an explicit constant string
> to get overwritten?

I don't see any way it could in this case. If there were some other
bug that allowed overwriting the explicit constant string, we'd want
to fix that other bug, but I don't see anything like that in this case.

2009-11-13 02:12:20

[permalink] [raw]

Subject: Re: [PATCH 3/4] security/selinux: decrement sizeof size in strncmp

James Morris wrote:
> On Thu, 12 Nov 2009, Casey Schaufler wrote:
>
>
>> I strongly suggest that this is not what is wanted.
>> strcmp(x,y)
>> and
>> strncmp(x,y,sizeof(y))
>>
>> are functionally equivalent and strcmp has a bad reputation in
>> the security community because it is associated with potential
>> buffer overrun issues.
>>
>
> Do you see potential for a buffer overrun in this case?
>

No, but I hate arguing with people who think that every time
they see strcmp that they have found a security flaw. The
existing code does exactly what it is intended to. Why make
a change that just clutters things up?

> The strings being compared are "sysfs" and the name field of 'struct
> file_system_type'. The kernel code elsewhere assumes the latter string to
> be a valid zero-terminated string, and we should, too.
>
>
> - James
>

2009-11-13 20:32:30

[permalink] [raw]

Subject: Re: [PATCH 3/4] security/selinux: decrement sizeof size in strncmp

Casey Schaufler wrote:
>James Morris wrote:
>> Do you see potential for a buffer overrun in this case?
>
>No, but I hate arguing with people who think that every time
>they see strcmp that they have found a security flaw.

So don't argue with those people, then. Those people are
probably deluded or ill-informed, if that's what they think every
time they see strcmp().

If you feel you absolutely must respond to them, send them here and
let them make the case for their position directly, with a concrete
technical argument -- if they have one (which I doubt). Or, better yet,
ignore those people. If they have a kneejerk reaction that "strcmp()
= security flaw", what makes you think they have anything useful to
contribute anyway?

I don't think this concern should have any weight whatsoever in the
decision on whether to patch the code.

2009-11-13 21:23:23

by Valdis Klētnieks

[permalink] [raw]

Subject: Re: [PATCH 3/4] security/selinux: decrement sizeof size in strncmp

On Thu, 12 Nov 2009 18:11:55 PST, Casey Schaufler said:
> James Morris wrote:
> > Do you see potential for a buffer overrun in this case?

> No, but I hate arguing with people who think that every time
> they see strcmp that they have found a security flaw.

How do you feel about people who think every time they see strcmp()
"Oh crap, something that needs auditing"? ;)

The biggest problem with strcmp() is that even if it got audited when that code
went in, it's prone to unaudited breakage when somebody changes something in
some other piece of code, quite often in some other .c file in some other
directory.

Julia, is there a way to use coccinelle to detect unsafe changes like that? Or
is expressing those semantics too difficult?

Attachments:

(No filename) (227.00 B)

2009-11-13 21:26:24

[permalink] [raw]

Subject: Re: [PATCH 3/4] security/selinux: decrement sizeof size in strncmp

On Fri, 13 Nov 2009, [email protected] wrote:

> On Thu, 12 Nov 2009 18:11:55 PST, Casey Schaufler said:
> > James Morris wrote:
> > > Do you see potential for a buffer overrun in this case?
>
> > No, but I hate arguing with people who think that every time
> > they see strcmp that they have found a security flaw.
>
> How do you feel about people who think every time they see strcmp()
> "Oh crap, something that needs auditing"? ;)
>
> The biggest problem with strcmp() is that even if it got audited when that code
> went in, it's prone to unaudited breakage when somebody changes something in
> some other piece of code, quite often in some other .c file in some other
> directory.
>
> Julia, is there a way to use coccinelle to detect unsafe changes like that? Or
> is expressing those semantics too difficult?

Could you give a concrete example of something that would be a problem?
If something like alias analysis is required, to know what strings a
variable might be bound to, that might be difficult. Coccinelle works
better when there is some concrete codeto match against. But it is
possible to eg match functions that have a certain property of their
return value, or to collect all of the values that are stored in a
structure field.

julia

2009-11-13 23:06:29

[permalink] [raw]

Subject: Re: [PATCH 3/4] security/selinux: decrement sizeof size in strncmp

> The biggest problem with strcmp() is that even if it got audited when
> that code went in, it's prone to unaudited breakage when somebody changes
> something in some other piece of code, quite often in some other .c file
> in some other directory.

I don't understand what concern you are ferring to. Could you explain?
What is special about strcmp() that requires auditing? What kind of
breakage are you talking about?

Are you just referring to the fact that strcmp() assumes its strings
are '\0'-terminated? Do you have the same concern about every library
function that handles '\0'-terminated strings? Does your concern apply
to this particular code snippet, where the call is (or would be) of the
form strcmp(s, "string constant")? Does your concern apply equally to
strncmp(s, "string constant", sizeof("string constant"))?

2009-11-13 23:08:20

by Valdis Klētnieks

[permalink] [raw]

Subject: Re: [PATCH 3/4] security/selinux: decrement sizeof size in strncmp

On Fri, 13 Nov 2009 22:26:20 +0100, Julia Lawall said:
> On Fri, 13 Nov 2009, [email protected] wrote:
> > Julia, is there a way to use coccinelle to detect unsafe changes like that? Or
> > is expressing those semantics too difficult?
>
> Could you give a concrete example of something that would be a problem?
> If something like alias analysis is required, to know what strings a
> variable might be bound to, that might be difficult. Coccinelle works
> better when there is some concrete codeto match against.

Here's a concrete example of how a previously audited strcmp() can go bad...

struct foo {
char[16] a; /* old code allows 15 chars and 1 more for the \0 */
int b;
int c;
}

bzero(foo,sizeof(foo));

Now code can pretty safely mess with the first 15 bytes of foo->a and
we know we're OK if we call strcmp(foo->a,....) because that bzero()
nuked a[15] for us. It's safe to strncpy(foo->a,bar,15); and not worry
about the fact that if bar is 15 chars long, a trailing \0 won't be put in.

Now somebody comes along and does:

struct foo {
char *a; /* we need more than 15 chars for some oddball hardware */
int b;
int c;
}

bzero(foo,sizeof(foo));
foo->a = kmalloc(32); /* whoops should have been kzmalloc */

Now suddenly, strncpy(foo->a,bar,31); *isn't* safe....

(Yes, I know there's plenty of blame to go around in this example - the failure
to use kzmalloc, the use of strncpy() without an explicit \0 being assigned
someplace, the use of strcmp() rather than strncmp()... But our tendency to
intentionally omit several steps of this to produce more efficient code means
it's easier to shoot ourselves in the foot...)

Attachments:

(No filename) (227.00 B)

2009-11-14 00:41:11

[permalink] [raw]

Subject: Re: [PATCH 3/4] security/selinux: decrement sizeof size in strncmp

>Here's a concrete example of how a previously audited strcmp() can go bad...
>
>struct foo {
> char[16] a; /* old code allows 15 chars and 1 more for the \0 */
> int b;
> int c;
>}
>
>bzero(foo,sizeof(foo));
>
>Now code can pretty safely mess with the first 15 bytes of foo->a and
>we know we're OK if we call strcmp(foo->a,....) because that bzero()
>nuked a[15] for us. It's safe to strncpy(foo->a,bar,15); and not worry
>about the fact that if bar is 15 chars long, a trailing \0 won't be put in.
>
>Now somebody comes along and does:
>
>struct foo {
> char *a; /* we need more than 15 chars for some oddball hardware */
> int b;
> int c;
>}
>
>bzero(foo,sizeof(foo));
>foo->a = kmalloc(32); /* whoops should have been kzmalloc */
>
>Now suddenly, strncpy(foo->a,bar,31); *isn't* safe....
>
>(Yes, I know there's plenty of blame to go around in this example - the failure
>to use kzmalloc, the use of strncpy() without an explicit \0 being assigned
>someplace, the use of strcmp() rather than strncmp()... But our tendency to
>intentionally omit several steps of this to produce more efficient code means
>it's easier to shoot ourselves in the foot...)

I'm not persuaded by your arguments against strcmp(): not even
a little bit.

1) The particular code snippets under discussion here were of the
form
strncmp(foo, "constant", sizeof("constant"))
And the proposal is to replace this code with
strcmp(foo, "constant")
Do you really mean to object to this proposal?

Please note that the '\0'-termination issues you mention do not
come up when comparing to a string constant. strcmp(, "constant")
is not going to run past the end of your 16-byte buffer. Moreover,
even if there was an issue with '\0'-termination, it would be present
to exactly the same degree with strncmp(), since the two code snippets
are behaviorally identical (in this case, it is not an issue for either
one, but if there was some context where it was an issue for strcmp(),
it would be an issue for the strncmp() equivalent, too). So none
of your arguments are a good reason to prefer
strncmp(foo, "constant", sizeof("constant"))
to
strcmp(foo, "constant")

2) More generally, you seem to be concerned about bugs where one
piece of code fails to '\0'-terminate a string, and another piece of
code invokes some library function that relies upon '\0'-termination.
That is not specifically a strcmp() issue; this is an issue with using
any string library that assumes '\0'-termination, where the string is not
'\0'-terminated. That's a much broader issue that should be addressed by
other means. Saying "strcmp() is bad" is a poor response to this risk.
For every string, you should decide whether it must be '\0'-terminated
or not, and then act appropriately.

If the string is intended to be '\0'-terminated, then you have
an obligation to ensure that all code consistently maintains this
invariant, and in return you may call string libraries that assume
'\0'-termination. Or if the string isn't intended to be '\0'-terminated,
then you should never be calling any string library function that
assumes '\0'-termination. This is pretty elementary stuff: if you do
much C programming, you're hopefully used to this. There's nothing
wrong with using strcmp(), if it is used appropriately. A kneejerk
"never use strcmp()" seems like poor policy to me.

In summary, I think your arguments against strcmp() are unpersuasive in
this context. Perhaps in some other context they are relevant, but not
to this thread. Focusing on strcmp() is misplaced.

If you want to do something useful, I think it would be more useful
to focus on checking that strings are properly '\0'-terminated where
this is necessary -- but realize that this is a much more challenging
property, and it's going to require something much more sophisticated
than just "don't use strcmp()". Most likely, this is not something that
Cocinelle will be sophisticated enough to handle usefully, at least in
the general case.

2009-11-14 03:06:43

[permalink] [raw]

Subject: Re: [PATCH 3/4] security/selinux: decrement sizeof size in strncmp

[email protected] wrote:
> On Thu, 12 Nov 2009 18:11:55 PST, Casey Schaufler said:
>
>> James Morris wrote:
>>
>>> Do you see potential for a buffer overrun in this case?
>>>
>
>
>> No, but I hate arguing with people who think that every time
>> they see strcmp that they have found a security flaw.
>>
>
> How do you feel about people who think every time they see strcmp()
> "Oh crap, something that needs auditing"? ;)
>

They have my deep sympathy. Which is why I'm advocating leaving
the perfectly functional and correct use of strncmp() as it is.

> The biggest problem with strcmp() is that even if it got audited when that code
> went in, it's prone to unaudited breakage when somebody changes something in
> some other piece of code, quite often in some other .c file in some other
> directory.
>
> Julia, is there a way to use coccinelle to detect unsafe changes like that? Or
> is expressing those semantics too difficult?
>
>
>

2009-11-14 03:44:10

[permalink] [raw]

Subject: Re: [PATCH 3/4] security/selinux: decrement sizeof size in strncmp

Casey Schaufler wrote:
> No, but I hate arguing with people who think that every time
> they see strcmp that they have found a security flaw.

[email protected] wrote:
> How do you feel about people who think every time they see strcmp()
> "Oh crap, something that needs auditing"? ;)

Casey Schaufler wrote:
> They have my deep sympathy. Which is why I'm advocating leaving
> the perfectly functional and correct use of strncmp() as it is.

Personally, I think this is catering to irrational kneejerk responses,
from hypothetical people who may or may not exist. I have yet to see a
single person stand up on the linux-kernel mailing list, endorse such
a position, and put their own name behind it. That's no surprise,
because it looks to me like an illogical and unjustified position that
would reflect poorly on anyone who adopted such a position in this case.
I do not think it makes sense to make a decision based upon the hypothesis
that maybe unidentified others will think that way, particularly when
no one can put forward a convincing argument that "thinking that way"
is reasonable in light of the technical facts.

I personally don't find
strncmp(foo, "constant", sizeof("constant")) // first snippet
to be more readable, auditable, or obviously correct than
strcmp(foo, "constant"). // second snippet
Do you? Does anyone?

Of course, those two code snippets have exactly the same behavior, so
there is no difference in their actual security properties. The only
possible difference I can imagine would be if one code snippet is
more readable, intuitive, or obviously correct than the other -- but I
would think strcmp() is no worse under those criteria, and if anything
modestly better. Is there a technical basis for arguing that the first
snippet is better than the second snippet?

2009-11-14 03:48:09

by Joe Perches

[permalink] [raw]

Subject: Re: [PATCH 3/4] security/selinux: decrement sizeof size in strncmp

On Sat, 2009-11-14 at 03:44 +0000, David Wagner wrote:
> I personally don't find
> strncmp(foo, "constant", sizeof("constant")) // first snippet
> to be more readable, auditable, or obviously correct than
> strcmp(foo, "constant"). // second snippet
> Is there a technical basis for arguing that the first
> snippet is better than the second snippet?

I don't think there is.

2009-11-14 05:08:25

by Valdis Klētnieks

[permalink] [raw]

Subject: Re: [PATCH 3/4] security/selinux: decrement sizeof size in strncmp

On Sat, 14 Nov 2009 00:41:15 GMT, David Wagner said:

> 1) The particular code snippets under discussion here were of the
> form
> strncmp(foo, "constant", sizeof("constant"))
> And the proposal is to replace this code with
> strcmp(foo, "constant")
> Do you really mean to object to this proposal?

No, in that particular case strcmp() is Obviously Safe.

> 2) More generally, you seem to be concerned about bugs where one
> piece of code fails to '\0'-terminate a string, and another piece of
> code invokes some library function that relies upon '\0'-termination.

Exactly.

> That is not specifically a strcmp() issue; this is an issue with using
> any string library that assumes '\0'-termination, where the string is not
> '\0'-terminated. That's a much broader issue that should be addressed by
> other means.

I know that - which is why I asked Julia if Cocinelle is able to do anything
in this area. An awful lot of our \0-terminated strings end up that way
implicitly because somebody does a kzalloc() or bzero() of an entire
structure, which can be fragile if code is refactored.

By the same token, that implicit behavior means that it's probably quite
difficult for any programmatic correctness checkers to follow the behavior.

> Saying "strcmp() is bad" is a poor response to this risk.

I didn't say "strcmp() is bad". I said it needs auditing. The strn-
versions of functions have a guaranteed termination condition right there
in the call. For strcmp() and strcpy(), the termination guarantee is
often elsewhere, which is why code using them tends to be brittle and
is harder to audit.

Attachments:

(No filename) (227.00 B)

2009-11-14 05:13:13

[permalink] [raw]

Subject: Re: [PATCH 3/4] security/selinux: decrement sizeof size in strncmp

Joe Perches wrote:
> On Sat, 2009-11-14 at 03:44 +0000, David Wagner wrote:
>
>> I personally don't find
>> strncmp(foo, "constant", sizeof("constant")) // first snippet
>> to be more readable, auditable, or obviously correct than
>> strcmp(foo, "constant"). // second snippet
>> Is there a technical basis for arguing that the first
>> snippet is better than the second snippet?
>>
>
> I don't think there is.
>

And you're exactly correct. Now please go convince all the whingers
who think that even though because their tool found a "bad" thing
there is nothing to worry about. But that's beside the point. There
really is no point here. This whole discussion is around a gratuitous
change that has no net effect on the behavior of the system. Unless
you are talking about the original change proposal, which would have
broken certain cases.

I am advocating that the code be left as is. It works fine (for what it
is intended to do, of course) and the "corrected" change is just plain
unnecessary. It is no clearer and no less clear than the original. Leave
it alone unless there is a good reason to change it. What, are y'all
getting paid by the patch or something?

> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
>
>

2009-11-14 05:26:18

by Joe Perches

[permalink] [raw]

Subject: Re: [PATCH 3/4] security/selinux: decrement sizeof size in strncmp

On Fri, 2009-11-13 at 21:12 -0800, Casey Schaufler wrote:
> Joe Perches wrote:
> > On Sat, 2009-11-14 at 03:44 +0000, David Wagner wrote:
> >> I personally don't find
> >> strncmp(foo, "constant", sizeof("constant")) // first snippet
> >> to be more readable, auditable, or obviously correct than
> >> strcmp(foo, "constant"). // second snippet
> >> Is there a technical basis for arguing that the first
> >> snippet is better than the second snippet?
> > I don't think there is.
> And you're exactly correct.
> This whole discussion is around a gratuitous
> change that has no net effect on the behavior of the system.

It has relatively little or no effect on a
running system, but does effect code
readability.

> I am advocating that the code be left as is.

I assert that code should be made as readable
as possible and that the code used fit the
reader's expectations.

strcmp(foo, "BAR") is natural.
strncmp(foo, "BAR", sizeof("BAR")) is unnatural
and should not be used.

cheers, Joe

2009-11-14 07:20:22

[permalink] [raw]

Subject: Re: [PATCH 3/4] security/selinux: decrement sizeof size in strncmp

Joe Perches wrote:
> On Fri, 2009-11-13 at 21:12 -0800, Casey Schaufler wrote:
>
>> Joe Perches wrote:
>>
>>> On Sat, 2009-11-14 at 03:44 +0000, David Wagner wrote:
>>>
>>>> I personally don't find
>>>> strncmp(foo, "constant", sizeof("constant")) // first snippet
>>>> to be more readable, auditable, or obviously correct than
>>>> strcmp(foo, "constant"). // second snippet
>>>> Is there a technical basis for arguing that the first
>>>> snippet is better than the second snippet?
>>>>
>>> I don't think there is.
>>>
>> And you're exactly correct.
>> This whole discussion is around a gratuitous
>> change that has no net effect on the behavior of the system.
>>
>
> It has relatively little or no effect on a
> running system, but does effect code
> readability.
>
>
>> I am advocating that the code be left as is.
>>
>
> I assert that code should be made as readable
> as possible and that the code used fit the
> reader's expectations.
>
> strcmp(foo, "BAR") is natural.
> strncmp(foo, "BAR", sizeof("BAR")) is unnatural
> and should not be used.
>
>

Oh good gravy. I've been writing C code since the 1970's and
have seen enough "unnatural" code to make most people think that
PASCAL was a good idea. This is not unnatural code. This is an
argument over which side of the head of the pin the odd angel
should dance on. Give it up. You're advocating a gratuitous
change. Can't y'all go find some questionable casts to expunge?
That might actually be useful.

> cheers, Joe
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
>
>

2009-11-14 15:22:43

[permalink] [raw]

Subject: Re: [PATCH 3/4] security/selinux: decrement sizeof size in strncmp

On Fri, 13 Nov 2009, [email protected] wrote:

> On Fri, 13 Nov 2009 22:26:20 +0100, Julia Lawall said:
> > On Fri, 13 Nov 2009, [email protected] wrote:
> > > Julia, is there a way to use coccinelle to detect unsafe changes like that? Or
> > > is expressing those semantics too difficult?
> >
> > Could you give a concrete example of something that would be a problem?
> > If something like alias analysis is required, to know what strings a
> > variable might be bound to, that might be difficult. Coccinelle works
> > better when there is some concrete codeto match against.
>
> Here's a concrete example of how a previously audited strcmp() can go bad...
>
> struct foo {
> char[16] a; /* old code allows 15 chars and 1 more for the \0 */
> int b;
> int c;
> }
>
> bzero(foo,sizeof(foo));
>
> Now code can pretty safely mess with the first 15 bytes of foo->a and
> we know we're OK if we call strcmp(foo->a,....) because that bzero()
> nuked a[15] for us. It's safe to strncpy(foo->a,bar,15); and not worry
> about the fact that if bar is 15 chars long, a trailing \0 won't be put in.
>
> Now somebody comes along and does:
>
> struct foo {
> char *a; /* we need more than 15 chars for some oddball hardware */
> int b;
> int c;
> }
>
> bzero(foo,sizeof(foo));
> foo->a = kmalloc(32); /* whoops should have been kzmalloc */
>
> Now suddenly, strncpy(foo->a,bar,31); *isn't* safe....
>
> (Yes, I know there's plenty of blame to go around in this example - the failure
> to use kzmalloc, the use of strncpy() without an explicit \0 being assigned
> someplace, the use of strcmp() rather than strncmp()... But our tendency to
> intentionally omit several steps of this to produce more efficient code means
> it's easier to shoot ourselves in the foot...)

Thanks for the example. Coccinelle only finds patterns of code in one
version, while this would require considering two versions at once. Such
a thing could be interesting though.

julia

2009-11-15 08:39:16

by Raja R Harinath

[permalink] [raw]

Subject: Re: [PATCH 3/4] security/selinux: decrement sizeof size in strncmp

Hi,

Casey Schaufler <[email protected]> writes:

> Joe Perches wrote:
[snip]
>> I assert that code should be made as readable
>> as possible and that the code used fit the
>> reader's expectations.
>>
>> strcmp(foo, "BAR") is natural.
>> strncmp(foo, "BAR", sizeof("BAR")) is unnatural
>> and should not be used.
>
> Oh good gravy. I've been writing C code since the 1970's and
> have seen enough "unnatural" code to make most people think that
> PASCAL was a good idea. This is not unnatural code. This is an
> argument over which side of the head of the pin the odd angel
> should dance on. Give it up. You're advocating a gratuitous
> change. Can't y'all go find some questionable casts to expunge?
> That might actually be useful.

I think the point is that

strncmp(foo, "BAR", sizeof("BAR"))

is exceedingly similar to

strncmp(foo, "BAR", strlen("BAR"))

which mean different things. The point of this series was the suspicion
that people who intended the "strlen" variant might have used the
"sizeof" variant.

And, since this confusion exists, it is probably better to use two
canonical forms for the two different meanings

strcmp(foo, "BAR")
strncmp(foo, "BAR", strlen("BAR"))

and avoid other equivalent formulations.

- Hari

2009-11-15 18:44:32