matchpathcon regcomp return code

matchpathcon regcomp return code

[Johan Fischer]

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

Hi list,

Well, first message here (yeah).

I have a double problem with restorecon crashing with a segfault.

Some background of the situation:
using centos4 with a cutsomized (not much) targeted selinux policy.
Trying to make the httpd server access /var/lib/svn (svn repositories)
which is a separate ext3 fs.

so basically, I changed the src/policy/file_contexts/types.fc and added
'/var/lib/svn/lost\+found(/.*)?  system_u:object_r:lost_found_t'
to keep my lost+found dir secured.

and changed my apache.fc file to add /var/lib/svn(/.*) to another type
accessible by apache (let's say system_u:object_r:httpd_sys_content_t
but could be anything else...)

Anyway, This configuration will not work as expected since the apache.fc
file is concatened after the types.fc, the lost+found will get the
httpd_sys_content_t type....

So I tried to use a bit of regex and set up a look ahead assertion to
avoid the lost+found and use the following regex (tested with perl happily):

/var/lib/svn(?!(/.*)?/lost\\+found)(/.*)?

Now the problem is restorecon is crashing (segfault in matchpathcon)

About the crashing, it seems that the error code check of regcomp in
matchpathcon.c is wrong (see attached patch from CVS HEAD).

The second question is actually, is lookaround supported at all in posix
regex ?

Cheers.
J.

-- 
Johan Fischer
Capital Markets CRC Limited
Level 2, 9 Castlereagh Street, Sydney NSW 2000
Tel: +61 2 9233 7999   Direct: +61 2 9236 9150
Fax: +61 2 9236 9177   http://www.cmcrc.com

Capital Markets CRC Ltd (CMCRC) - Confidential Communication
The information contained in this e-mail is confidential.  It is intended solely for the addressee. If you receive this e-mail by mistake please promptly inform us by reply e-mail and then delete the e-mail and destroy any printed copy. You must not disclose or use in any way the information in the e-mail. There is no warranty that this e-mail is error or virus free. It may be a private communication, and if so, does not represent the views of the CMCRC and its associates. If it is a private communication, care should be taken in opening it to ensure that undue offence is not given.


[-- Attachment #2: matchpathcon.c.diff --]
[-- Type: text/plain, Size: 390 bytes --]

--- matchpathcon.c	2005-10-05 15:33:38.000000000 +1000
+++ matchpathcon.c.new	2005-10-05 15:33:26.000000000 +1000
@@ -501,7 +501,7 @@
 			regcomp(&spec_arr[nspec].regex,
 				anchored_regex,
 				REG_EXTENDED | REG_NOSUB);
-		if (regerr < 0) {
+		if (regerr != 0) {
 			myprintf("%s:  line %d has invalid regex %s\n", path, lineno, anchored_regex); 
 			free(anchored_regex);
 			return 0;

Re: matchpathcon regcomp return code

[Stephen Smalley]

On Wed, 2005-10-05 at 16:14 +1000, Johan Fischer wrote:
> Anyway, This configuration will not work as expected since the apache.fc
> file is concatened after the types.fc, the lost+found will get the
> httpd_sys_content_t type....

Yes, file contexts ordering/precedence is a known problem.  It is
exacerbated by the new policy module support, and Tresys has introduced
a sorting algorithm for their reference policy
(http://serefpolicy.sf.net) based on fixed string vs. regex (already
done by matchpathcon), stem length, string length, and presence of the
file type specifier.  Possibly we should incorporate that sort into
matchpathcon.  Chris, is there a reason you kept it as a separate helper
specific to reference policy versus submitting it as a patch to
matchpathcon itself?  I know that you mentioned that it does alter the
order in some cases from what we have now, but I think we agreed that we
can address those cases as needed.

> So I tried to use a bit of regex and set up a look ahead assertion to
> avoid the lost+found and use the following regex (tested with perl happily):
> 
> /var/lib/svn(?!(/.*)?/lost\\+found)(/.*)?
> 
> Now the problem is restorecon is crashing (segfault in matchpathcon)
> 
> About the crashing, it seems that the error code check of regcomp in
> matchpathcon.c is wrong (see attached patch from CVS HEAD).

Ah, thanks for the bug fix.  Should likely also call regerror there and
report that error string.

> The second question is actually, is lookaround supported at all in posix
> regex ?

I don't know offhand, but if regcomp rejects it, then I suspect not.
Possibly we could convert it over to using pcre.

-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: matchpathcon regcomp return code

[Christopher J. PeBenito]

On Wed, 2005-10-05 at 09:19 -0400, Stephen Smalley wrote:
> On Wed, 2005-10-05 at 16:14 +1000, Johan Fischer wrote:
> > Anyway, This configuration will not work as expected since the apache.fc
> > file is concatened after the types.fc, the lost+found will get the
> > httpd_sys_content_t type....
> 
> Yes, file contexts ordering/precedence is a known problem.  It is
> exacerbated by the new policy module support, and Tresys has introduced
> a sorting algorithm for their reference policy
> (http://serefpolicy.sf.net) based on fixed string vs. regex (already
> done by matchpathcon), stem length, string length, and presence of the
> file type specifier.  Possibly we should incorporate that sort into
> matchpathcon.  Chris, is there a reason you kept it as a separate helper
> specific to reference policy versus submitting it as a patch to
> matchpathcon itself?  I know that you mentioned that it does alter the
> order in some cases from what we have now, but I think we agreed that we
> can address those cases as needed.

We kept it separate because we were still working with the algorithm.
More importantly, we don't want to affect the example policy, since it
altered the order on at least one occasion, as discussed before.  If you
don't have a problem with this, we can certainly work on a patch for
matchpathcon.

-- 
Chris PeBenito
Tresys Technology, LLC
(410) 290-1411 x150


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: matchpathcon regcomp return code

[Stephen Smalley]

On Wed, 2005-10-05 at 10:29 -0400, Christopher J. PeBenito wrote:
> We kept it separate because we were still working with the algorithm.
> More importantly, we don't want to affect the example policy, since it
> altered the order on at least one occasion, as discussed before.  If you
> don't have a problem with this, we can certainly work on a patch for
> matchpathcon.

Yes, I think it would be preferable to have it directly in matchpathcon,
and we can add further pathname regexes as needed to fix any undesired
changes in the actual labeling.  I don't know if you've done anything
further with respect to the idea floated earlier about (optional)
explicit precedence/order specifiers in file contexts, but we might want
to introduce those at the same time.
 
-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: matchpathcon regcomp return code

[Christopher J. PeBenito]

On Wed, 2005-10-05 at 10:34 -0400, Stephen Smalley wrote:
> On Wed, 2005-10-05 at 10:29 -0400, Christopher J. PeBenito wrote:
> > We kept it separate because we were still working with the algorithm.
> > More importantly, we don't want to affect the example policy, since it
> > altered the order on at least one occasion, as discussed before.  If you
> > don't have a problem with this, we can certainly work on a patch for
> > matchpathcon.
> 
> Yes, I think it would be preferable to have it directly in matchpathcon,
> and we can add further pathname regexes as needed to fix any undesired
> changes in the actual labeling.  I don't know if you've done anything
> further with respect to the idea floated earlier about (optional)
> explicit precedence/order specifiers in file contexts, but we might want
> to introduce those at the same time.

We haven't added any precedence/order specifiers in the file contexts,
since, the previous problem I mentioned was fixed by adding another more
specific regex.  If we do want to add the optional specifiers, the
question becomes how we want to use it for comparison?  Should it be
used as the last comparison, like a tie-breaker, or as the first
comparison, which would group regexes of the same priority?  Also, what
happens if one regex has a precedence and the other doesn't?

-- 
Chris PeBenito
Tresys Technology, LLC
(410) 290-1411 x150


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: matchpathcon regcomp return code

[Stephen Smalley]

On Thu, 2005-10-06 at 13:38 -0400, Christopher J. PeBenito wrote:
> We haven't added any precedence/order specifiers in the file contexts,
> since, the previous problem I mentioned was fixed by adding another more
> specific regex.  If we do want to add the optional specifiers, the
> question becomes how we want to use it for comparison?  Should it be
> used as the last comparison, like a tie-breaker, or as the first
> comparison, which would group regexes of the same priority?  Also, what
> happens if one regex has a precedence and the other doesn't?

Ok, let's defer that change until later and just focus on merging your
existing sort algorithm into matchpathcon_init.  BTW, is your sort
stable?  That was a problem we used to have when using qsort there; the
ordering of "equal" members in the array was not stable.
  
-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: matchpathcon regcomp return code

[Christopher J. PeBenito]

On Thu, 2005-10-06 at 14:42 -0400, Stephen Smalley wrote:
> On Thu, 2005-10-06 at 13:38 -0400, Christopher J. PeBenito wrote:
> > We haven't added any precedence/order specifiers in the file contexts,
> > since, the previous problem I mentioned was fixed by adding another more
> > specific regex.  If we do want to add the optional specifiers, the
> > question becomes how we want to use it for comparison?  Should it be
> > used as the last comparison, like a tie-breaker, or as the first
> > comparison, which would group regexes of the same priority?  Also, what
> > happens if one regex has a precedence and the other doesn't?
> 
> Ok, let's defer that change until later and just focus on merging your
> existing sort algorithm into matchpathcon_init.  BTW, is your sort
> stable?  That was a problem we used to have when using qsort there; the
> ordering of "equal" members in the array was not stable.

Ok, we'll start working on a patch for matchpathcon_init.  We do use a
stable sort (mergesort).

-- 
Chris PeBenito
Tresys Technology, LLC
(410) 290-1411 x150


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: matchpathcon regcomp return code

[Johan Fischer]

Just proposing an idea (possibly stupid or out of focus compare to the
new algorithm which I don't know about yet)....

What happens if you have a sort of headers .fc file for suggested
replaceable types (like the / and subtree context) and a trailer .fc to
override important context in any case ( the lost+context and subdir) ?


Christopher J. PeBenito wrote:

>On Wed, 2005-10-05 at 10:34 -0400, Stephen Smalley wrote:
>  
>
>>On Wed, 2005-10-05 at 10:29 -0400, Christopher J. PeBenito wrote:
>>    
>>
>>>We kept it separate because we were still working with the algorithm.
>>>More importantly, we don't want to affect the example policy, since it
>>>altered the order on at least one occasion, as discussed before.  If you
>>>don't have a problem with this, we can certainly work on a patch for
>>>matchpathcon.
>>>      
>>>
>>Yes, I think it would be preferable to have it directly in matchpathcon,
>>and we can add further pathname regexes as needed to fix any undesired
>>changes in the actual labeling.  I don't know if you've done anything
>>further with respect to the idea floated earlier about (optional)
>>explicit precedence/order specifiers in file contexts, but we might want
>>to introduce those at the same time.
>>    
>>
>
>We haven't added any precedence/order specifiers in the file contexts,
>since, the previous problem I mentioned was fixed by adding another more
>specific regex.  If we do want to add the optional specifiers, the
>question becomes how we want to use it for comparison?  Should it be
>used as the last comparison, like a tie-breaker, or as the first
>comparison, which would group regexes of the same priority?  Also, what
>happens if one regex has a precedence and the other doesn't?
>
>  
>

-- 
Johan Fischer
Capital Markets CRC Limited
Level 2, 9 Castlereagh Street, Sydney NSW 2000
Tel: +61 2 9233 7999   Direct: +61 2 9236 9150
Fax: +61 2 9236 9177   http://www.cmcrc.com

Capital Markets CRC Ltd (CMCRC) - Confidential Communication
The information contained in this e-mail is confidential.  It is intended solely for the addressee. If you receive this e-mail by mistake please promptly inform us by reply e-mail and then delete the e-mail and destroy any printed copy. You must not disclose or use in any way the information in the e-mail. There is no warranty that this e-mail is error or virus free. It may be a private communication, and if so, does not represent the views of the CMCRC and its associates. If it is a private communication, care should be taken in opening it to ensure that undue offence is not given.


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: matchpathcon regcomp return code

[Stephen Smalley]

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

On Wed, 2005-10-05 at 09:19 -0400, Stephen Smalley wrote:
> On Wed, 2005-10-05 at 16:14 +1000, Johan Fischer wrote:
> > Now the problem is restorecon is crashing (segfault in matchpathcon)
> > 
> > About the crashing, it seems that the error code check of regcomp in
> > matchpathcon.c is wrong (see attached patch from CVS HEAD).
> 
> Ah, thanks for the bug fix.  Should likely also call regerror there and
> report that error string.

I merged the patch below into libselinux 1.27.4.  Includes your change
plus use of regerror.

With regard to the original problem, I think you just have to put both
pathname regexes (the top-level /var/lib/svn one and the lost+found one)
into apache.fc with the desired ordering explicit for now.  We are
looking into ways to address the general ordering issues in the future,
including the sort algorithm mentioned earlier and an optional explicit
ordering/predecence value field in the .fc entries.

-- 
Stephen Smalley
National Security Agency

[-- Attachment #2: libselinux-1.27.4.patch --]
[-- Type: text/x-patch, Size: 2178 bytes --]

Index: libselinux/ChangeLog
===================================================================
RCS file: /nfshome/pal/CVS/selinux-usr/libselinux/ChangeLog,v
retrieving revision 1.145
retrieving revision 1.146
diff -u -p -r1.145 -r1.146
--- libselinux/ChangeLog	3 Oct 2005 17:06:16 -0000	1.145
+++ libselinux/ChangeLog	5 Oct 2005 15:48:05 -0000	1.146
@@ -1,3 +1,8 @@
+1.27.4 2005-10-05
+	* Merged fix for matchpathcon (regcomp error checking) from Johan
+	Fischer.  Also added use of regerror to obtain the error string
+	for inclusion in the error message.
+
 1.27.3 2005-10-03
 	* Changed getseuserbyname to not require (and ignore if present)
 	the MLS level in seusers.conf if MLS is disabled, setting *level
Index: libselinux/VERSION
===================================================================
RCS file: /nfshome/pal/CVS/selinux-usr/libselinux/VERSION,v
retrieving revision 1.63
retrieving revision 1.64
diff -u -p -r1.63 -r1.64
--- libselinux/VERSION	3 Oct 2005 17:06:16 -0000	1.63
+++ libselinux/VERSION	5 Oct 2005 15:48:05 -0000	1.64
@@ -1 +1 @@
-1.27.3
+1.27.4
Index: libselinux/src/matchpathcon.c
===================================================================
RCS file: /nfshome/pal/CVS/selinux-usr/libselinux/src/matchpathcon.c,v
retrieving revision 1.30
retrieving revision 1.31
diff -u -p -r1.30 -r1.31
--- libselinux/src/matchpathcon.c	30 Sep 2005 18:27:18 -0000	1.30
+++ libselinux/src/matchpathcon.c	5 Oct 2005 15:48:05 -0000	1.31
@@ -501,8 +501,18 @@ static int process_line( const char *pat
 			regcomp(&spec_arr[nspec].regex,
 				anchored_regex,
 				REG_EXTENDED | REG_NOSUB);
-		if (regerr < 0) {
-			myprintf("%s:  line %d has invalid regex %s\n", path, lineno, anchored_regex); 
+		if (regerr != 0) {
+			size_t errsz = 0;
+			char *errbuf = NULL;
+			errsz = regerror(regerr, &spec_arr[nspec].regex, 
+					 errbuf, errsz);
+			if (errsz)
+				errbuf = malloc(errsz);
+			if (errbuf)
+				(void) regerror(regerr, 
+						&spec_arr[nspec].regex, 
+						errbuf, errsz);
+			myprintf("%s:  line %d has invalid regex %s:  %s\n", path, lineno, anchored_regex, (errbuf ? errbuf : "out of memory")); 
 			free(anchored_regex);
 			return 0;
 		}

Re: matchpathcon regcomp return code

[Christopher J. PeBenito]

On Wed, 2005-10-05 at 09:19 -0400, Stephen Smalley wrote:
> Yes, file contexts ordering/precedence is a known problem.  It is
> exacerbated by the new policy module support, and Tresys has introduced
> a sorting algorithm for their reference policy
> (http://serefpolicy.sf.net) based on fixed string vs. regex (already
> done by matchpathcon), stem length, string length, and presence of the
> file type specifier.
[cut]
> Chris, is there a reason you kept it as a separate helper
> specific to reference policy versus submitting it as a patch to
> matchpathcon itself?

We were thinking about this a little more.  Since sorting the file
contexts is a relatively expensive operation, wouldn't it be better to
just sort the file contexts once on install, rather than redoing it on
every matchpathcon/setfiles/restorecon?  Source policies that need more
than the simple sort already done in matchpathcon_init can sort the file
contexts on install, and the loadable modules infrastructure can sort
for loadable modules.  This has the advantage of not disturbing the
example policy.  Then the sorting would be added to libselinux as a
library function, along with a util program for source policies to use
for sorting.

-- 
Chris PeBenito
Tresys Technology, LLC
(410) 290-1411 x150


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: matchpathcon regcomp return code

[Stephen Smalley]

On Mon, 2005-10-10 at 10:03 -0400, Christopher J. PeBenito wrote:
> We were thinking about this a little more.  Since sorting the file
> contexts is a relatively expensive operation, wouldn't it be better to
> just sort the file contexts once on install, rather than redoing it on
> every matchpathcon/setfiles/restorecon?  Source policies that need more
> than the simple sort already done in matchpathcon_init can sort the file
> contexts on install, and the loadable modules infrastructure can sort
> for loadable modules.  This has the advantage of not disturbing the
> example policy.  Then the sorting would be added to libselinux as a
> library function, along with a util program for source policies to use
> for sorting.

I'm uneasy about leaving this to policy (re)generation; I would prefer
that matchpathcon apply a well-defined ordering/precedence to file
contexts (including file_contexts.homedirs and file_contexts.local,
which at least presently can be changed without going through a policy
build).  Note that this only occurs during initialization
(matchpathcon_init), not on every matchpathcon call by an application.
I'm also unclear on whether the overhead from sorting the entries in
memory is actually substantial compared to the rest of what
matchpathcon/matchpathcon_init does already.  The regex compilation and
matching seems more costly.

-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.