checkpolicy: the " is not part of the filename for

OpenPGP: *Attachments to this message have not been signed or encrypted*

********* *BEGIN ENCRYPTED or SIGNED PART* *********

This patch looks good to me. acked.

********** *END ENCRYPTED or SIGNED PART* **********

0001-checkpolicy-the-is-not-part-of-the-filename-for-tran.patchFrom c3ba40d2e17186d702a6ea2b83e185603dafa06f Mon Sep 17 00:00:00 2001
From: Dan Walsh <[email protected]>
Date: Tue, 20 Sep 2011 09:52:57 -0400
Subject: [PATCH 01/63] checkpolicy: the " is not part of the filename for
trans rules

Policy decided that all filenames needed to be wrapped in " in the filename trans rules. But we weren't doing anything with those in the language syntax and instead just passed the " to the kernel as if the filename in question were actually \"file\". Add the " to the policy grammer.

Signed-off-by Eric Paris <[email protected]>

---
 checkpolicy/policy_parse.y |    4 ++--
 checkpolicy/policy_scan.l  |    2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

checkpolicy/policy_parse.y +2 -2

index 49ac15f..1e3ef6f 100644
@@ -353,7 +353,7 @@ cond_rule_def           : cond_transition_def
 			| require_block
 			{ $$ = NULL; }
                         ;
-cond_transition_def	: TYPE_TRANSITION names names ':' names identifier filename ';'
+cond_transition_def	: TYPE_TRANSITION names names ':' names identifier '\"' filename '\"' ';'
                         { $$ = define_cond_filename_trans() ;
                           if ($$ == COND_ERR) return -1;}
 			| TYPE_TRANSITION names names ':' names identifier ';'

@@ -391,7 +391,7 @@ cond_dontaudit_def	: DONTAUDIT names names ':' names names ';'
 			{ $$ = define_cond_te_avtab(AVRULE_DONTAUDIT);
                           if ($$ == COND_ERR) return -1; }
 		        ;
-transition_def		: TYPE_TRANSITION  names names ':' names identifier filename ';'
+transition_def		: TYPE_TRANSITION  names names ':' names identifier '\"' filename '\"' ';'
 			{if (define_filename_trans()) return -1; }
 			| TYPE_TRANSITION names names ':' names identifier ';'
                         {if (define_compute_type(AVRULE_TRANSITION)) return -1;}

checkpolicy/policy_scan.l +0 -1

index a61e0db..2ba5971 100644
@@ -227,7 +227,6 @@ PERMISSIVE			{ return(PERMISSIVE); }
 {digit}{1,3}(\.{digit}{1,3}){3}    { return(IPV4_ADDR); }
 {hexval}{0,4}":"{hexval}{0,4}":"({hexval}|[:.])*  { return(IPV6_ADDR); }
 {digit}+(\.({alnum}|[_.])*)?    { return(VERSION_IDENTIFIER); }
-\"({alnum}|[_\.\-])+\"		{ return(FILENAME); }
 {alnum}*                        { return(FILENAME); }
 \.({alnum}|[_\.\-])*	        { return(FILENAME); }
 {letter}+([-_\.]|{alnum})+      { return(FILENAME); }

Quoted 53 lines · Daniel J Walsh

OpenPGP: *Attachments to this message have not been signed or encrypted*

********* *BEGIN ENCRYPTED or SIGNED PART* *********

This patch looks good to me. acked.

********** *END ENCRYPTED or SIGNED PART* **********

0001-checkpolicy-the-is-not-part-of-the-filename-for-tran.patchFrom c3ba40d2e17186d702a6ea2b83e185603dafa06f Mon Sep 17 00:00:00 2001 From: Dan Walsh <[email protected]> Date: Tue, 20 Sep 2011 09:52:57 -0400 Subject: [PATCH 01/63] checkpolicy: the " is not part of the filename for trans rules

Policy decided that all filenames needed to be wrapped in " in the filename trans rules. But we weren't doing anything with those in the language syntax and instead just passed the " to the kernel as if the filename in question were actually \"file\". Add the " to the policy grammer.

Signed-off-by Eric Paris <[email protected]> ---

checkpolicy/policy_parse.y | 4 ++-- checkpolicy/policy_scan.l
| 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)

(quoted diff) +0 -0

b/checkpolicy/policy_parse.y index 49ac15f..1e3ef6f 100644 --- a/checkpolicy/policy_parse.y +++ b/checkpolicy/policy_parse.y @@
-353,7 +353,7 @@ cond_rule_def : cond_transition_def |
require_block { $$ = NULL; } ; -cond_transition_def :
TYPE_TRANSITION names names ':' names identifier filename ';'
+cond_transition_def : TYPE_TRANSITION names names ':' names
identifier '\"' filename '\"' ';' { $$ = define_cond_filename_trans() ; if ($$ == COND_ERR) return -1;} | TYPE_TRANSITION names names ':' names identifier ';' @@ -391,7
+391,7 @@ cond_dontaudit_def : DONTAUDIT names names ':' names
names ';' { $$ = define_cond_te_avtab(AVRULE_DONTAUDIT); if ($$
== COND_ERR) return -1; } ; -transition_def : TYPE_TRANSITION
names names ':' names identifier filename ';' +transition_def :
TYPE_TRANSITION names names ':' names identifier '\"' filename '\"' ';' {if (define_filename_trans()) return -1; } | TYPE_TRANSITION names names ':' names identifier ';' {if (define_compute_type(AVRULE_TRANSITION)) return -1;} diff --git a/checkpolicy/policy_scan.l b/checkpolicy/policy_scan.l index a61e0db..2ba5971 100644 --- a/checkpolicy/policy_scan.l +++
b/checkpolicy/policy_scan.l @@ -227,7 +227,6 @@ PERMISSIVE {
return(PERMISSIVE); } {digit}{1,3}(\.{digit}{1,3}){3} {
return(IPV4_ADDR); } {hexval}{0,4}":"{hexval}{0,4}":"({hexval}|[:.])* {
return(IPV6_ADDR); } {digit}+(\.({alnum}|[_.])*)? {

Quoted 8 lines

return(VERSION_IDENTIFIER); } -\"({alnum}|[_\.\-])+\" {
return(FILENAME); } {alnum}* {
return(FILENAME); } \.({alnum}|[_\.\-])* {
return(FILENAME); } {letter}+([-_\.]|{alnum})+ {
return(FILENAME); } @@ -253,6 +252,7 @@ PERMISSIVE {
return(PERMISSIVE); } "-" | "." | "]" | +"\"" | "~" | "*" {
return(yytext[0]); } . {
yywarn("unrecognized character");} -- 1.7.7

I believe this shouldn't be necessary, and it looks like that's because a patch was committed that shouldn't have been.

This was the original filename commit:

commit d4c230386653db49d8e8116b603efcce4423df70 Author: Daniel J
Walsh <[email protected]> Date: Fri Apr 29 15:29:48 2011 -0400

checkpolicy: use a better identifier for filenames

That commit was reverted and changed to require a quote around filenames (which did the quote stripping) in this commit:

commit b42e15ffd5163effe3b2cb910685a5956a00defc Author: Steve
Lawrence <[email protected]> Date: Mon May 16 08:40:00 2011
-0400

checkpolicy: wrap file names in filename trans with quotes

Then, recnetly, this patch was committed, which looks to be the same as the commit that was reverted:

commit d72a9ec825ef2a8723510f62292cf2adfd4a2a6c Author: Dan Walsh
<[email protected]> Date: Tue Apr 12 09:54:46 2011 -0400

checkpolicy: Redo filename/filesystem syntax to support filename trans rules

The comment for that commit said:

In order to support filenames, which might start with "." or filesystems that start with a number we need to rework the matching rules a little bit. Since the new filename rule is so permissive it must be moved to the bottom of the matching list to not cover other definitions.

Both of those cases should have been supported by the "wrap in quotes" commit.

Was this just a mistake of something getting committed that shouldn't have been? Should d72a9ec825ef2a8723510f62292cf2adfd4a2a6c be reverted?

- Steve

-- This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to [email protected] with the words "unsubscribe selinux" without quotes as the message.

Quoted 64 lines · Daniel J Walsh

OpenPGP: *Attachments to this message have not been signed or encrypted*

********* *BEGIN ENCRYPTED or SIGNED PART* *********

This patch looks good to me. acked.

********** *END ENCRYPTED or SIGNED PART* **********

0001-checkpolicy-the-is-not-part-of-the-filename-for-tran.patchFrom c3ba40d2e17186d702a6ea2b83e185603dafa06f Mon Sep 17 00:00:00 2001
From: Dan Walsh <[email protected]>
Date: Tue, 20 Sep 2011 09:52:57 -0400
Subject: [PATCH 01/63] checkpolicy: the " is not part of the filename for
trans rules

Policy decided that all filenames needed to be wrapped in " in the filename trans rules. But we weren't doing anything with those in the language syntax and instead just passed the " to the kernel as if the filename in question were actually \"file\". Add the " to the policy grammer.

Signed-off-by Eric Paris <[email protected]>

---
 checkpolicy/policy_parse.y |    4 ++--
 checkpolicy/policy_scan.l  |    2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

checkpolicy/policy_parse.y +2 -2

index 49ac15f..1e3ef6f 100644
@@ -353,7 +353,7 @@ cond_rule_def           : cond_transition_def
 			| require_block
 			{ $$ = NULL; }
                         ;
-cond_transition_def	: TYPE_TRANSITION names names ':' names identifier filename ';'
+cond_transition_def	: TYPE_TRANSITION names names ':' names identifier '\"' filename '\"' ';'
                         { $$ = define_cond_filename_trans() ;
                           if ($$ == COND_ERR) return -1;}
 			| TYPE_TRANSITION names names ':' names identifier ';'

@@ -391,7 +391,7 @@ cond_dontaudit_def	: DONTAUDIT names names ':' names names ';'
 			{ $$ = define_cond_te_avtab(AVRULE_DONTAUDIT);
                           if ($$ == COND_ERR) return -1; }
 		        ;
-transition_def		: TYPE_TRANSITION  names names ':' names identifier filename ';'
+transition_def		: TYPE_TRANSITION  names names ':' names identifier '\"' filename '\"' ';'
 			{if (define_filename_trans()) return -1; }
 			| TYPE_TRANSITION names names ':' names identifier ';'
                         {if (define_compute_type(AVRULE_TRANSITION)) return -1;}

checkpolicy/policy_scan.l +0 -1

index a61e0db..2ba5971 100644
@@ -227,7 +227,6 @@ PERMISSIVE			{ return(PERMISSIVE); }
 {digit}{1,3}(\.{digit}{1,3}){3}    { return(IPV4_ADDR); }
 {hexval}{0,4}":"{hexval}{0,4}":"({hexval}|[:.])*  { return(IPV6_ADDR); }
 {digit}+(\.({alnum}|[_.])*)?    { return(VERSION_IDENTIFIER); }
-\"({alnum}|[_\.\-])+\"		{ return(FILENAME); }
 {alnum}*                        { return(FILENAME); }
 \.({alnum}|[_\.\-])*	        { return(FILENAME); }
 {letter}+([-_\.]|{alnum})+      { return(FILENAME); }

checkpolicy/policy_scan.l PERMISSIVE { return(PERMISSIVE); }

Quoted 8 lines

checkpolicy/policy_scan.l +1 -1

@@ -253,6 +252,7 @@ PERMISSIVE			{ return(PERMISSIVE); }
 "-" |
 "." |
 "]" |
+"\"" |
 "~" |
 "*"				{ return(yytext[0]); } 
 .                               { yywarn("unrecognized character");}
-- 1.7.7

I believe this shouldn't be necessary, and it looks like that's because a patch was committed that shouldn't have been.

This was the original filename commit:

commit d4c230386653db49d8e8116b603efcce4423df70
Author: Daniel J Walsh <[email protected]>
Date: Fri Apr 29 15:29:48 2011 -0400

checkpolicy: use a better identifier for filenames

That commit was reverted and changed to require a quote around filenames (which did the quote stripping) in this commit:

commit b42e15ffd5163effe3b2cb910685a5956a00defc
Author: Steve Lawrence <[email protected]>
Date: Mon May 16 08:40:00 2011 -0400

checkpolicy: wrap file names in filename trans with quotes

Then, recnetly, this patch was committed, which looks to be the same as the commit that was reverted:

commit d72a9ec825ef2a8723510f62292cf2adfd4a2a6c
Author: Dan Walsh <[email protected]>
Date: Tue Apr 12 09:54:46 2011 -0400

checkpolicy: Redo filename/filesystem syntax to support filename
trans rules

The comment for that commit said:

In order to support filenames, which might start with "." or filesystems that start with a number we need to rework the matching rules a little bit. Since the new filename rule is so permissive it must be moved to the bottom of the matching list to not cover other definitions.

Both of those cases should have been supported by the "wrap in quotes" commit.

Was this just a mistake of something getting committed that shouldn't have been? Should d72a9ec825ef2a8723510f62292cf2adfd4a2a6c be reverted?