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

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

[Julia Lawall]

From: Julia Lawall <julia@diku.dk>

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.

The semantic patch that makes this change is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
@@
expression foo;
constant char *abc;
@@

strncmp(foo, abc, 
- sizeof(abc)
+ sizeof(abc)-1
 )
// </smpl>

Signed-off-by: Julia Lawall <julia@diku.dk>

---
 security/selinux/hooks.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff -u -p a/security/selinux/hooks.c b/security/selinux/hooks.c
--- 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;
 
 	/* Initialize the root inode. */

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

[James Morris]

On Thu, 12 Nov 2009, Julia Lawall wrote:

> From: Julia Lawall <julia@diku.dk>
> 
> 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() ?


-- 
James Morris
<jmorris@namei.org>

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

[Serge E. Hallyn]

Quoting James Morris (jmorris@namei.org):
> On Thu, 12 Nov 2009, Julia Lawall wrote:
> 
> > From: Julia Lawall <julia@diku.dk>
> > 
> > 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).

-serge

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

[Julia Lawall]

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

> Quoting James Morris (jmorris@namei.org):
> > On Thu, 12 Nov 2009, Julia Lawall wrote:
> > 
> > > From: Julia Lawall <julia@diku.dk>
> > > 
> > > 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

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

[Casey Schaufler]

Julia Lawall wrote:
> On Thu, 12 Nov 2009, Serge E. Hallyn wrote:
>
>   
>> Quoting James Morris (jmorris@namei.org):
>>     
>>> On Thu, 12 Nov 2009, Julia Lawall wrote:
>>>
>>>       
>>>> From: Julia Lawall <julia@diku.dk>
>>>>
>>>> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
>   

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

[David Wagner]

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.

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

[James Morris]

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
<jmorris@namei.org>

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

[Julia Lawall]

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

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

[David Wagner]

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.

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

[Casey Schaufler]

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
>   

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

[David Wagner]

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.

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

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 743 bytes --]

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?



[-- Attachment #2: Type: application/pgp-signature, Size: 227 bytes --]

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

[Julia Lawall]

On Fri, 13 Nov 2009, Valdis.Kletnieks@vt.edu 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

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

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 1669 bytes --]

On Fri, 13 Nov 2009 22:26:20 +0100, Julia Lawall said:
> On Fri, 13 Nov 2009, Valdis.Kletnieks@vt.edu 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...)


[-- Attachment #2: Type: application/pgp-signature, Size: 227 bytes --]

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

[David Wagner]

>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.

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

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 1629 bytes --]

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.


[-- Attachment #2: Type: application/pgp-signature, Size: 227 bytes --]

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

[Julia Lawall]

On Fri, 13 Nov 2009, Valdis.Kletnieks@vt.edu wrote:

> On Fri, 13 Nov 2009 22:26:20 +0100, Julia Lawall said:
> > On Fri, 13 Nov 2009, Valdis.Kletnieks@vt.edu 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

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

[David Wagner]

> 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"))?

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

[Casey Schaufler]

Valdis.Kletnieks@vt.edu 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?
>
>
>   

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

[David Wagner]

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.

Valdis.Kletnieks@vt.edu 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?

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

[Joe Perches]

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.

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

[Casey Schaufler]

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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>
>
>   

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

[Joe Perches]

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

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

[Casey Schaufler]

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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>
>
>   

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

[Raja R Harinath]

Hi,

Casey Schaufler <casey@schaufler-ca.com> 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

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

[Casey Schaufler]

Raja R Harinath wrote:
> Hi,
>
> Casey Schaufler <casey@schaufler-ca.com> 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.
>   

The point is that the code is correct as written. The change
suggested, changing the sizeof to strlen, would result in
incorrect behavior in the case where foo is "BAR-BAR-BA-RAN".
The other change suggested, which is to change strncmp to
strcmp, would be functionally correct but offers no value,
might be considered less safe in certain circles, and requires
a change that, like any change, adds a trivial amount of risk.

> 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
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>
>
>