Bug 189845

Summary: FC5 SELinux causes miscompares for restore -C
Product: [Fedora] Fedora Reporter: Tony Nelson <tonynelson>
Component: dumpAssignee: Jindrich Novy <jnovy>
Status: CLOSED RAWHIDE QA Contact: Ben Levenson <benl>
Severity: medium Docs Contact:
Priority: medium    
Version: 5CC: csellers, dwalsh, jmorris, kayvansylvan, mwc, peter.young, pknirsch, sdsmall, stelian
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2006-08-07 11:02:48 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Attachments:
Description Flags
CVS diff to let restore cope with SELinux policy changes
none
Patch (CVS diff -up) to help restore cope with SELinux policy changes
none
Patch (CVS diff -up) to help restore cope with SELinux policy changes
none
Patch (CVS diff -up) to help restore cope with SELinux policy changes
none
Patch (CVS diff -up) to help restore cope with SELinux policy changes none

Description Tony Nelson 2006-04-25 01:40:17 UTC
Description of problem:
I'm not sure whether this is a policy or more a core SELinux issue.  The new
SELinux in Fedora Core 5 enables the 4th (MLS) field of Security Contexts. 
When, e.g., getxattr() is called and the EA doesn't have MLS, SELinux helpfully
invents one.

Dump reads filesystems directly, and dumps EAs as found.  Restore -C compares
data in a dump with the filesystem as seen though system calls.  This can cause
miscompares between the data saved when dumping a filesystem made under earlier
SELinux and that filesystem as seen by restore -C through FC5 SELinux.

I think that programs like dump need to be able to see what is really in the EA,
perhaps through some sort of compatibility mode.  My bugzilla on restore at
sourceforge #1475895:
<http://sourceforge.net/tracker/index.php?func=detail&aid=1475895&group_id=1306&atid=101306>

Version-Release number of selected component (if applicable):
Current FC5 SELinux

How reproducible:
Always, as the behavior is by design

Steps to Reproduce:
1. dump on FC5 (or earlier?) a filesystem made before FC5 (and not relabeled on
FC5, I presume).
2. restore -C on FC5, comparing the filesystem with the dump.
3.
  
Actual results:
Data reported by e.g., getxattr() differs from data in the dump or on disk. 
Restore sensibly considers data changed.

Expected results:
Data reported by getxattr() matches data on disk.  Want more fields?  /Relabel/
the volume first!  (Hmph!)

Additional info:
I observed this on FC5 and see in CVS for dump and restore how it happens.  I
called getxattr() myself (via python) and saw it happen.

Comment 1 Stelian Pop 2006-04-25 08:43:13 UTC
I tend to think this is more a design issue in SELinux itself (like in
'how a SELinux labeled volume must be backuped ?') than a bug in
restore.

As I said in my previous post, the solution might be to never backup the
security related EAs, and just tell the user to relabel the partition
after the restore.

I guess I'll just wait and see what the SELinux guys think about this.


Comment 2 Daniel Walsh 2006-04-25 16:15:21 UTC
Are you saying the getxattr is appending or removing the ":s0"? on the restore/dump?



Comment 3 Tony Nelson 2006-04-26 00:30:52 UTC
Appends the ":s0" when called.  Restore compares the result of getxattr() with
the EA it reads from the dump, and is unhappy.

Note that the dump was made on FC5, of an FC3 / volume.  Dump sees the EAs as on
disk.  Restore sees the EAs through the filesystem calls (e.g., getxattr()) and
finds a difference.

Comment 4 Daniel Walsh 2006-04-26 13:48:44 UTC
If you want to translate the context to the on disk representation you can use
selinux_trans_to_raw_context.

/* Perform context translation between the human-readable format
   ("translated") and the internal system format ("raw"). 
   Caller must free the resulting context via freecon.  
   Returns -1 upon an error or 0 otherwise.
   If passed NULL, sets the returned context to NULL and returns 0. */
extern int selinux_trans_to_raw_context(security_context_t trans, 
					security_context_t *rawp);
extern int selinux_raw_to_trans_context(security_context_t raw, 
					security_context_t *transp);

Comment 5 Stephen Smalley 2006-04-26 16:06:35 UTC
The getxattr canonicalization of the on-disk attribute value was introduced in
Linux 2.6.15, principally to allow transparent migration to the MLS-enabled
SELinux without requiring a full relabel of all filesystems, as in an upgrade
from FC4 to FC5.  It also solved some longstanding issues with enabling SELinux
to return the actual incore inode security label as seen by SELinux for access
control purposes to userspace in a consistent manner for all files, even for
files being labeled by mount option or some other mechanism other than actual
xattrs.  This has been an issue for SELinux ever since we migrated to using
xattrs and using the xattr API.  FreeBSD takes a different approach, with
separate APIs for getting/setting MAC labels than for getting/setting raw
extended attributes, but that wasn't acceptable to Linux.

Note that calling the translation interfaces isn't necessarily sufficient, as
the MLS field data used by the kernel for a file that lacks a MLS attribute is
based on policy, and is not necessarily some fixed value like "s0".  In the real
MLS policy, it would be the highest level of data authorized for the system.

I think restore just has to be adjusted to ignore this particular kind of
difference, possibly by using some new libselinux function to do the context
comparison?



Comment 6 Stelian Pop 2006-04-27 10:08:44 UTC
Dump accesses the disk directly, without passing through getxattr() syscalls. So
the value which is backuped is the _real_ _raw_ value, the one stored inside the
inode.

So what restore needs is to retrive this same raw value. The problem is that
restore doesn't do raw accesses, it only sees the file attributes though the
normal (stat, etc, and getxattr()) syscalls. 

From what I understand from the previous comments, it isn't clear if the kernel
itself allow this...


Comment 7 Stephen Smalley 2006-04-27 16:21:59 UTC
No, the kernel only presents a single view of the SELinux attribute these days,
and that view is always consistent with the SELinux module's internal notion of
the incore inode security label, no matter what is (or isn't) on disk.
So restore likely needs to do the comparison via a new libselinux function so
that we can have that function include logic to ignore this kind of difference
without hardcoding such logic in restore itself.  Unless you have an aversion to
a dependency on libselinux there.

Comment 8 Stephen Smalley 2006-04-27 16:38:10 UTC
Use of lsetxattr could be a problem as well for restore, as that won't go
through the translation library, so if you are saving the raw contexts from a
pre-MLS system, and then call lsetxattr() to restore them, they will be rejected
due to lacking the MLS field.  Which would get added transparently via
lsetfilecon(3).

Comment 9 Stelian Pop 2006-04-28 08:34:21 UTC
First, I don't care about having a dependency on libselinux.

But as long as the kernel doesn't give a way to access and set the *raw* context
I don't see how someone can succesfully restore a backup and keep all the xattrs
unchanged, without being dependent on some kernel version and current policy.

Maybe I'm just too confused by all this SELinux stuff.

If there is a way (with or without libselinux) I'll be happy to implement it
into restore.

If there isn't, as I said before, the solution might be to never restore (or
compare) the security related EAs, and just tell the user to relabel the
partition after the restore. Does this have any side effects I don't see ?




Comment 10 Stephen Smalley 2006-04-28 12:37:16 UTC
I think that (never restoring) the security related EAs would be a real loss in
functionality, because relabeling (based on file_contexts) only resets labels to
their initial defaults.  So you could lose the labels of runtime files or other
customization done after initial labeling of the system.

However, it may make sense to separate out the handling of security EAs in
dump/restore from other EA handling, because of their distinctive nature and
requirements.  For example:
1) It would be better to use lgetfilecon/lsetfilecon to get and set the SELinux
contexts, as libselinux will then handle translation and automatic extension as
needed.
2) Ideally, it would be even better to use setfscreatecon() prior to
open/mkdir/... when restoring files, because that ensures that the file is set
down in the right security context from the beginning rather than mutating the
label after creation.  Think of it as being similar to umask; the fscreate
context is an attribute of the process that is applied to files it creates; if
not set, the default behavior is applied.  If you look at the star selinux
patch, you'll see that is what it does - it skips the SELinux attribute on
normal xattr processing and instead uses setfscreatecon() prior to creation of
each file.
3) You might want to pass the attribute through security_canonicalize_context()
prior to comparison.  This should handle translation/extension and also fold
type aliases. 

Sorry if this is confusing; part of this is just due to mismatch between xattrs
and MAC labels, and part of it is just due to the desire by Red Hat to provide
transparent migration to MLS-enabled systems without relabeling (which is
understandable, but has some surprising side effects).


Comment 11 Tony Nelson 2006-04-29 02:51:01 UTC
I just hacked restore so restore -C could ignore the new MLS default value of
":s0" (EA loose compare, restore -C -E:s0).  This was less useful than I had
hoped, as there were a few (thousand) other changes imposed by the new policy
(specifically, shlib_t -> lib_t, texrel_shlib_t -> textrel_shlib_t, and catman_t
-> man_t; grep -v is my friend).  So there seems to be a difficult to escape
dependency on the current policy during a restore.

I haven't tried security_canonicalize_context().

If anyone still wants my changes I can put up a patch. >8v(

Comment 12 Kayvan Sylvan 2006-04-30 17:11:45 UTC
I used dump to create a filesystem backup, then used the FC5 Install DVD
to go into rescue mode and restore the dump. That seem to work okay for
the file data. However, for each and every file, I get the message:

    restore: lsetxattr ./filename_being_restored failed: Invalid argument

Using "ls -Z", I see that all the files end up being unlabeled (or they
are in the unlabeled_t context).

These files were all set up in Fedora FC4, using the targeted policy.

When I am booting up using the FC5 Install DVD ("linux rescue"), the SELinux
startup shows:

  security:  3 users, 6 roles, 1161 types, 135 bools, 1 sens, 256 cats
  security:  55 classes, 38679 rules
  SELinux:  Completing initialization.

I am wondering if I have to have the same SELinux policy loaded while
in the rescue mode in order to avoid the "lsetxattr: invalid argument"
error? How would I go about doing that?

Comment 13 Tony Nelson 2006-05-01 02:47:43 UTC
Created attachment 128432 [details]
CVS diff to let restore cope with SELinux policy changes

I've made a patch that seems to fix my problem on restore -C.  It canonicalizes
security EAs from the dump before using them, so it might fix kayvan's problem
as well.  Functions from libselinux are dynamically loaded, so it is not an
error if they aren't present, when they won't be needed anyway.  It also prints
the new EA value on miscompares, to make the process less mysterious.

This is my first patch and I haven't used CVS much.  Someone with actual
SELinux knowledge should also look at it, so I've put it here rather than with
Restore's bug.	All the activity is here anyway.

Comment 14 Stephen Smalley 2006-05-01 12:43:48 UTC
diff -up is nicer for reading the patches, or at least diff -u.

I'm a little unclear on your use of the _raw function rather than the normal
function for canonicalization, as I would have thought you would have wanted the
normal function in order to go through the context translation library and get
automatic MLS extension for contexts that lack them.  The _raw function won't do
that.

Given that you already computed the length, you could just memcpy to the value
rather than using strcpy at that point for efficiency.  You likely want to
bounds check strlen(newvalue) before adding to it, to avoid integer overflow
case, although that should never happen in practice.


Comment 15 Tony Nelson 2006-05-01 14:14:29 UTC
Mr. Smalley -- Thank you for looking at the patch.

> diff -up is nicer for reading the patches, or at least diff -u.

I'll keep that in mind.  I don't know how to do that yet from CVS.


> I'm a little unclear on your use of the _raw function rather than the normal
> function for canonicalization, as I would have thought you would have wanted
> the normal function in order to go through the context translation library
> and get automatic MLS extension for contexts that lack them.  The _raw
> function won't do that.

I'm unclear about it as well.  I couldn't find any documentation on writing
SELinux code.  I did see some discussions on the NSA site's SELinux mailing list
relating to "raw".  It seemed to me that EAs from a dump were in "raw" form, but
that was just a guess from the discussion, which seemed to say that non-raw was
for display to the user.  In any case, the function I called transforms the EA
so that it matches the value returned by getxattr(), and all my miscompares go away.

Given that it did add the MLS field, is security_canonicalize_context_raw() the
wrong thing to call, then?


> Given that you already computed the length, you could just memcpy to the
> value rather than using strcpy at that point for efficiency.

I've just called into the kernel, so there isn't much efficiency left to be had.

> You likely want to bounds check strlen(newvalue) before adding to it, to
> avoid integer overflow case, although that should never happen in practice.

In case security_canonicalize_context_raw() returns a value exactly 2 GB in
size?  Well, OK, but in that case, things would already be pretty bad, and if
the size is greater than 2 GB, the code will always fail.  As presently coded,
security_canonicalize_context_raw() cannot return a value longer than PAGE_SIZE.
 I'd like to check that against it's specification, though.  Could you tell me
where that is?  (Probably with the coding docs I couldn't find.)

BTW, in security_canonicalize_context_raw():

	strncpy(buf, con, size);

	ret = write(fd, buf, strlen(buf)+1);

Strncpy() won't append a trailing NUL if strlen(con) == size.  Calling strlen()
on the result may write() a larger amount of data than PAGE_SIZE.  I prefer to use:

	buf[0] = '\0';
	strncat(buf, con, size);

which will always write the terminating NUL, and generates shorter machine code
than:

	strncpy(buf, con, size);
	buf[size-1] = '\0';

Here "efficiency" is code size.

Comment 16 Stephen Smalley 2006-05-01 14:40:23 UTC
1) cvs diff -up generates the desired output.
2) security_canonicalize_context_raw("system_u:object_r:file_t", &x) returns -1
with errno EINVAL on FC5; I just confirmed that here; it rejects the context
because it lacks the MLS field.  So if the EA you are passing in lacks the :s0
suffix, it should fail.  So I think you want to use
security_canonicalize_context().  However, you also need to either canonicalize
the context obtained by lgetxattr or (better) use lgetfilecon instead when
obtaining the actual context for comparison purposes; otherwise, they may still
differ due to libsetrans interference.
3) Current kernel interface limits us to page size, but we don't want to assume
that will always be the case.  Fair point about the code.


Comment 17 Tony Nelson 2006-05-01 15:17:38 UTC
0) My patch cannot work and I withdraw it.  Sorry for the inconvenience.

1) OK, thanks.

2) You're certainly right about my code failing.  I'm unfamiliar with restore
(dump), and I assumed that returning FAIL would be considered some sort of error
on the compare.  That may be a bug in restore, but I should have verified my
patches operation.

I see now that the unreported errors caused the comparisons to be skipped. 
Security_canonicalize_context() does not return an error, but actually does
nothing.  Whatever "canonicalize" means, it doesn't mean what I need.

Since only xattr_walk() knows that it is looking at a security EA, more
restructuring of the code would be needed for the comparison to be made as you
suggest.  Unless there is a SELinux call that will transform an old context into
a new one, my approach cannot work.

3) OK.

Comment 18 Kayvan Sylvan 2006-05-01 15:21:18 UTC
When you come up with a better patch, please email me so I can see if it fixes
my problem with dump/restore and xattr security.* attributes.

Thanks!

Comment 19 Tony Nelson 2006-05-02 02:24:06 UTC
Let me see if I understand.

There is no documentation for security_canonicalize_context().

There is no documented or available routine to do what happens magically inside
getxattr(): translate an on-disk context to a "raw" context and then
"cannonicalize" it by substituting aliases.

By accident, trans_to_raw_context() does the first part, at least on my system.
 Security_canonicalize_context_raw() appears to do the second part.

Comment 20 Stephen Smalley 2006-05-02 12:42:54 UTC
1) Correct, sorry.  It converts the context to the canonical form as defined in
the kernel policy, which means it folds aliases to primary names, orders
categories in a well-defined way, etc.  It was introduced to avoid miscompares
by rpm, setfiles, restorecon, etc. when dealing with file labels, since targeted
policy often folds types together via aliases that are separated by the strict
policy to maximize their on-disk compatibility and sharing of the same policy
source base.   
2) Correct, and further, the actual handling in the kernel is specific to the
filesystem.  When an inode is brought incore that lacks a MLS field in its
attribute, the kernel determines a default based on the default file label
associated with the superblock/filesystem, and uses that value.  getxattr(2)
then just returns the full context that was already determined by the kernel
earlier.  This was introduced to provide filesystem compatibility between
non-MLS and MLS, particularly for upgrades.  It only allowed such "partial"
contexts for existing inodes to support such legacy filesystems; any context
specified to the kernel by userspace still needs to be fully qualified.
3) trans_to_raw_context() does automatically extend the context with a dummy MLS
value if it lacks one, and raw_to_trans_context() strips it.  This was also a
compatibility measure, particularly for the MCS policy where they are only using
the categories and not the sensitivity.  The non-raw libselinux functions apply
trans_to_raw_context() to all context arguments before passing them on to the
kernel, and apply raw_to_trans_context() to all context values before passing
them back to the application.  The raw libselinux functions skip that
processing, assuming that the caller is providing and/or wants a raw context value.



Comment 21 Stephen Smalley 2006-05-02 14:19:38 UTC
Let me suggest a different approach for dump/restore and similar kinds of backup
functionality:
- skip SELinux attributes when performing normal EA handling, as they don't fit
well with such handling,
- use lgetfilecon and either setfscreatecon() or lsetfilecon() to get and set
the SELinux labels for dump and restore separate from the normal EA handling.

This has several benefits:
- You get a consistent view of the context of a file, solving your comparison
problem.
- You get translated contexts for storage in the dump, which makes the dump more
 meaningful if you want to transfer it to another system or later revive it
after policy changes.
- You get automatic translation of contexts when setting them, which solves the
non-MLS->MLS compatibility headache.

This should also be done in star, IMHO; it already does skip the SELinux
attribute for setxattr and uses setfscreatecon(), but doesn't presently use
lgetfilecon to get the attribute.


Comment 22 Stelian Pop 2006-05-02 14:39:02 UTC
Sure, but how do I cope with the fact that dump doesn't access the inode data
through the standard syscalls ? (the filesystem being dumped doesn't even have
to be mounted...)

Comment 23 Tony Nelson 2006-05-03 21:55:00 UTC
(In reply to comment #20)

> 2) Correct, and further, the actual handling in the kernel is specific
> to the filesystem.  When an inode is brought incore that lacks a MLS
> field in its attribute, the kernel determines a default based on the
> default file label associated with the superblock/filesystem, and uses
> that value.  getxattr(2) then just returns the full context that was
> already determined by the kernel earlier. ...

If there were a routine in selinux.h that did what restore and any
other program that looks at the raw disk needs, would it be called
"selinux_trans_to_raw_context()"?

> 3) trans_to_raw_context() does automatically extend the context with a
> dummy MLS value if it lacks one, and raw_to_trans_context() strips it. 
> This was also a compatibility measure, particularly for the MCS policy
> where they are only using the categories and not the sensitivity.

Until there is a routine in selinux.h that translates an on-disk
context to a "raw" context, will selinux_trans_to_raw_context()
continue to work to translate a disk context to a "raw" context?

"Raw" is now a rather misleading name for the in-kernel version of
a security context.

(In reply to comment #21)

It appears to me that there will be problems for any backup program
that saves security contexts made with one version of SELinux and
then restores them with another (later) version.  Dump and restore
are only the first example of this.  I don't see that handling
security contexts separately fixes the problem.  When I call
security_canonicalize_context(), the context has the MLS added,
then aliasing is done, and then the MLS is stripped, leaving a
security context that when backed up and later restored would
still not fit the new SELinux or policy.  Does lgetfilecon() (on a
earlier system, making a dump that will be used (years) later on a
new system) do a translation that will make the security context
acceptable to a new SELinux or policy?

Comment 24 Tony Nelson 2006-05-03 21:58:58 UTC
(In reply to comment #23)

> ...would it be called "selinux_trans_to_raw_context()"?

Doh!  "...'selinux_disk_to_raw_context()'?"

Comment 26 Tony Nelson 2006-05-07 01:35:11 UTC
Created attachment 128699 [details]
Patch (CVS diff -up) to help restore cope with SELinux policy changes

Here is another attempt at a patch to let restore handle restoring onto a
different SELinux policy from the dump.  My changes were tested with -C and -i.
 It /might/ link staticly -- I clearly am not doing static builds properly, so
I'm not certain.

Adds -e option to translate SELinux security context EAs from the dump into the
current format, using the coincedental selinux_trans_to_raw_context() and then
security_canonicalize_context_raw().  If selinux_disk_to_raw_context() exists,
it will be used instead of selinux_trans_to_raw_context() (except on static
builds).  Prints a message on failure to translate.  Weak links to
selinux_disk_to_raw_context().	Weak linking doesn't seem to work with static
linking, though it does work with the usual dynamic linking.

Adds -E "mls" option to append a literal suffix to each SELinux security
context EA from the dump, and then call security_canonicalize_context_raw(). 
Prints a message on failure to translate.

No security context translation is done without one of -e or -E.

Prints both EAs on compare failure, and which of the tests failed.

Prints the error count (if non-zero) at each tape change and at the end, just
to be less mysterious.	Note that returning FAIL usually gets counted as an
error, even if no message is printed.

Adds --enable-transselinux to configure.in, default yes, so that restore can be
build without SELinux support.	Associated changes are made to
restore/Makefile.in.  I'm still a novice with the autotools, so watch out.

usage() has been updated, but the restore man page and the changelog have not.

I've tried this patch with -C -e, -i -e, -i -E ":s0" (works), -i -E ":s1"
(fails properly), and -P -e (rejects combination).  I haven't tried -r, -R, -t,
or -x.

Old dumps will exist into the distant future, and SELinux will keep evolving. 
I don't know whether selinux_trans_to_raw_context() will continue to do what
this patch needs, or if any API will be added to SELinux to do it instead, or
if that API will be selinux_disk_to_raw_context(), or what else will be
required for future changes to SELinux and its policy.	There is a chance that
-E "mls" will keep this patch useful if -e falls apart.  In the future, a more
sophisticated -E might be needed.  Perhaps there should also be an option to
skip all security EAs.

Comment 27 Stephen Smalley 2006-05-08 12:57:07 UTC
Possible alternative approach, suggested by Chad Sellers of Tresys:
restore passes the raw EA saved by prior dump through
security_canonicalize_context(), uses that canonical result for setting the
context of the restored file via lsetfilecon(), and uses lgetfilecon() to get
the context, and then compares that result against the canonical context
obtained earlier via security_canonicalize_context().  So we end up using the
canonical context on the restoring system for all setting and comparison.

Another possible alternative being contemplated for rpm is to allow certain
programs (like restore and rpm) to actually set the raw label value even if the
policy doesn't understand it, treating it as the unlabeled context incore in
that case until such a time as either the policy is changed to make it valid or
the file is relabeled to a known label.

Comment 28 Tony Nelson 2006-05-08 19:02:16 UTC
(In reply to comment #27)

1) But security_canonicalize_context() is usually a no-op.  Internally it calls
selinux_trans_to_raw_context() which happens to add a default MLS, then it calls
security_canonicalize_context_raw() to translate aliases, if any, and then it
calls selinux_raw_to_trans_context() which happens to remove the default MLS. 
The security context is usually unaltered and does not work with lsetxattr(), as
reported by kayvan in #12; would an unaltered security context missing the new
MLS field work with lsetfilecon()?  Calling security_canonicalize_context() is
the first thing I tried.  Would security_canonicalize_context() have to be
changed to do something, and would it be re-spec'ed that it does what is needed
to convert disk to trans?  Really, all my patch is doing is skipping the "remove
the MLS" part, and the objection to my patch's approach is that it depends on
the accident that selinux_trans_to_raw_context() is doing what is needed here.

In the case of restore -C comparing a dump, what file should have lsetfilecon()
called on it?  Note that the filesystem being compared should not be written to,
and may well be mounted read-only.

2) Umm, setting unusable security contexts would help, how?  Restore would be
able to put lots of bogus security contexts on a restored volume?  This seems
like a bad idea.  The restore would seem to have worked, and the volume would
seem to be labeled when someone checks, but SELinux wouldn't work, as everything
would seem to be unlabeled.


Comment 29 Stephen Smalley 2006-05-08 19:19:29 UTC
1) All of the libselinux functions internally call the translation functions on
input and output context arguments to convert back and forth between the
translated representation and the kernel representation.  In the case where the
string is already in the right representation, the translation function just
becomes a no-op.  So lsetfilecon() will accept the context with or without a MLS
suffix and adjust it as needed before passing it along to lsetxattr().  And
lgetfilecon() will take the kernel-provided value and massage it before giving
it (strip the :s0 suffix) back to restore.  So if you call
security_canonicalize_context() first and use that as your subsequent baseline
and if you use lsetfilecon() and lgetfilecon() afterwards, then I think it will
just work (tm).   Again, suggestion was made by Chad Sellers of Tresys.  In the
case of restore -C comparing a dump, you would just skip the lsetfilecon() step
of course, and merely call security_canonicalize_context() first to get a
baseline, and then call lgetfilecon() on the file and compare the result with
that baseline.  Both lgetfilecon and security_canonicalize_context take the
kernel-provided canonical context back through a raw-to-translated conversion,
so they end up with the same final result.  And security_canonicalize_context
takes the input argument from the application through the translated-to-raw
conversion before feeding it to the kernel, so that gets you a valid context
even if the original lacked a MLS field.

2) If you consider the parallel with uids, restore will happily restore uids (or
gids) that have no local meaning, right?  And it is then up to the administrator
to reset them if he wants the files to be accessible (in the SELinux case,
relabeling them via chcon or restorecon), or to add accounts with those
uids/gids (in the SELinux case, adding policy that defines them).  In the
situation where they lack the MLS field, the kernel already knows how to handle
that, and restoring them without the MLS field preserves the precise same
behavior that existed on the original filesystem.  But this does require a
kernel change, and it isn't clear yet exactly how it will fall out.  So (1) is
likely worth pursuing.
    

Comment 30 Tony Nelson 2006-05-11 01:21:58 UTC
Created attachment 128868 [details]
Patch (CVS diff -up) to help restore cope with SELinux policy changes

This attempt at a patch to let restore handle restoring onto a
different SELinux policy from the dump follows the approach in #27, but is
otherwise similar to #26.  The various xattr_cb_*() routines have a new
paramter that tells them to handle security contexts specially.

Comment 31 Tony Nelson 2006-05-23 01:59:20 UTC
"SE-Linux guys" -- does my patch in #30 look good enough for stelian to use? 
Modulo whatever he thinks of it, of course.

(having trouble submitting this -- hope you don't see it twice)

Comment 32 Stelian Pop 2006-05-24 08:19:54 UTC
As far as I am concerned, the patch looks fine to me. If the SELinux guys
validate the logic I'll apply it upstream.

Thanks for the efforts Tony !


Comment 33 Stephen Smalley 2006-05-24 12:33:52 UTC
Question re the patch:
1) It looks like you switch on the conversion support for any attribute in the
security namespace, which could be a problem later if anyone else starts to use
that namespace (e.g. if someone implements filesystem support for capabilities,
they might use security.cap or similar, or if someone implements another LSM,
they might use security.foo for their foo LSM).  So it should only happen for
attributes specifically named "security.selinux".
2) It looks like part of the patch is ext2-specific?  So what about ext3
(possibly a non-issue, if the same code path and indices apply), and what about
xfs?  In all cases, the attribute name would be "security.selinux" if the
attribute is the SELinux attribute, regardless of index or other specifics.


Comment 34 Tony Nelson 2006-05-24 16:26:13 UTC
(In reply to comment #33)

1) OK.  Do you mean "exactly named" or "starting with" "security.selinux"?

2) I don't see anything EXT2 specific in my patch.  What part of the patch seems
EXT2 specific -- give file and line number?

Comment 35 Stephen Smalley 2006-05-24 16:40:40 UTC
1) Exactly named.  There is a single SELinux attribute per file named
"security.selinux".
2) I was looking at the diffs to restore/xattr.c, and saw that your diffs
touched code paths referring to things like EXT2_XATTR_INDEX_SECURITY, which is
an ext2-specific definition.  Looking at the base code, I see that it is already
inherently ext2/ext3-specific (which makes sense, since dump/restore is
necessarily filesystem-specific), so I guess my comment is irrelevant.


Comment 36 Tony Nelson 2006-05-24 16:58:34 UTC
(In reply to comment #35)

1) OK.

2) OK.  (At least dump is fs-specific.  For all I know, restore would work on
another fs.)

Comment 37 Tony Nelson 2006-05-24 18:02:21 UTC
Created attachment 129955 [details]
Patch (CVS diff -up) to help restore cope with SELinux policy changes

This attempt at a patch to let restore handle restoring onto a
different SELinux policy from the dump is nearly the same as #30, but only
affects security.selinux EAs.

Comment 38 Jindrich Novy 2006-06-16 13:30:37 UTC
The patch is not cleanly applicable. Could you please port the patch to the FC-5
dump-0.4b41 and attach it here again?

Patch #1 (dump-selinux.patch):
+ patch -p0 -b --suffix .selinux -s
1 out of 1 hunk FAILED -- saving rejects to file restore/Makefile.in.rej
5 out of 7 hunks FAILED -- saving rejects to file restore/main.c.rej
1 out of 1 hunk FAILED -- saving rejects to file restore/tape.c.rej
5 out of 8 hunks FAILED -- saving rejects to file restore/xattr.c.rej


Comment 39 Tony Nelson 2006-06-16 16:18:33 UTC
(In reply to comment #38)
> The patch is not cleanly applicable. Could you please port the patch to the
> FC-5 dump-0.4b41 and attach it here again?
 ...

I don't know how to make patches.  The patch was made with:

cvs -z3 -d:pserver:anonymous.sourceforge.net:/cvsroot/dump diff -up

as requested by Stephen Smalley.  He was satisfied with it done that way, so I
assume that he knows how to apply such patches, but I don't.  AFAICT, the patch
should match Makefile.in, but it doesn't work for me either.

Comment 40 Jindrich Novy 2006-06-16 16:29:03 UTC
Could you attach here the packed modified sources then? I'll generate patches
myself.

Comment 41 Stephen Smalley 2006-06-16 16:39:20 UTC
Looks like there were overlapping commits to the dump cvs to fix problems with
ACLs.  cvs log restore/xattr.c shows:
revision 1.3
date: 2005/06/08 13:24:12;  author: stelian;  state: Exp;  lines: +25 -15
Fix big endian issues with EA/ACL
----------------------------
revision 1.2
date: 2005/06/08 09:34:40;  author: stelian;  state: Exp;  lines: +195 -3
Fix restore of ACLs.
----------------------------

So your patch likely needs to be re-based.  Any problem with getting this
accepted upstream?


Comment 42 Stephen Smalley 2006-06-16 16:42:43 UTC
Oh, sorry - misread that as 2006.  So I don't understand why a diff you
generated earlier doesn't apply.

Comment 43 Stephen Smalley 2006-06-16 16:47:40 UTC
Ok, your patch is whitespace damaged.  Did you cut-and-paste it into something?
patch -p0 -l < mls_patch4.txt
works here on current dump cvs.

Naturally, you really want your patches to be -p1 appliable, so better to
generate one level up.

Comment 44 Tony Nelson 2006-06-17 01:37:08 UTC
Created attachment 131091 [details]
Patch (CVS diff -up) to help restore cope with SELinux policy changes

(In reply to comment #43)
> Ok, your patch is whitespace damaged.  Did you cut-and-paste it into
> something?

Yes.  Don't do that?

> patch -p0 -l < mls_patch4.txt

I tried -l, but not -p0.

> works here on current dump cvs.

Yes, that WFM too.


> Naturally, you really want your patches to be -p1 appliable, so better to
> generate one level up.

OK.  I see that the command I used before works one level up as well.

Comment 45 Jindrich Novy 2006-06-22 10:04:23 UTC
Tony, should be the ability to translate SELinux EAs be an option? Is there any
reason why to make it configurable? I'm asking because I need to run autoconf to
regenerate configure script what would add next BuildRequires. Even after that
the dump won't build because of the missing device-mapper symbols.

Stelian, is there a chance to see it applied upstream soon?

Comment 46 Tony Nelson 2006-06-22 15:44:39 UTC
(In reply to comment #45)
> Tony, should be the ability to translate SELinux EAs be an option?

Yes.  It is an option.  Use "--enable-transselinux"

> Is there any reason why to make it configurable?

Yes.  Some dump users don't have selinux at all.  If it breaks something for
them, it is important that they be able to disable it.

> I'm asking because I need to run autoconf to
> regenerate configure script what would add next BuildRequires. Even after that
> the dump won't build because of the missing device-mapper symbols.

I don't understand.  This patch doesn't use device-mapper.

Comment 47 Stelian Pop 2006-08-01 10:25:45 UTC
(In reply to comment #45)

> Stelian, is there a chance to see it applied upstream soon?

Since I don't use SELinux myself, I hoped to see some testing of this patch in
Fedora's dump before applying it upstream.





Comment 49 Jindrich Novy 2006-08-07 11:02:48 UTC
Ok, thanks for reply, it's now applied in rawhide (since 0.4b41-2.fc6).

Comment 50 peter young 2006-09-12 16:28:23 UTC
is there an rpm with the patch for FC5 x86_64 I can use ?

If I dump from FC5 and then restore, I get this problem and it adds hours onto 
the restore time.
So using dump and restore to build laptops is nolonger practical, but I need 
to roll out FC5...

Comment 51 Tony Nelson 2006-09-12 21:24:27 UTC
I haven't made one.  The patch hasn't been adopted by Fedora yet, because it
hasn't been adopted by the dump maintainer yet because of insufficient testing.
 If you build a patched restore and use it to do a restore, plase add a comment
to this bug on how it worked.  If no one does this, then it is presumed that the
patch is not needed, so restore will not be updated.

Comment 52 Jindrich Novy 2006-09-13 05:12:11 UTC
Peter, I'm going to release FC5 update so that you can use the fixed dump.

Comment 53 peter young 2006-09-13 09:13:51 UTC
in fact what I need is a rescue CD so that I can dump and restore images.

when I tried to use the rescue CD from rawhide
it detected Broadcom BCM5788
but then hung when 
Starting Interface
Attempting to start eth0

Is it possible to get the revised rescue CD for FC5 ?
If not could I just run the dump and restore files when booted in rescue mode 
or are there other dependencies ?

You assistance is much appreciated


Comment 54 peter young 2006-09-13 09:27:52 UTC
in fact what I need is a rescue CD so that I can dump and restore images.

when I tried to use the rescue CD from rawhide
it detected Broadcom BCM5788
but then hung when 
Starting Interface
Attempting to start eth0

Is it possible to get the revised rescue CD for FC5 ?
If not could I just run the dump and restore files when booted in rescue mode 
or are there other dependencies ?

You assistance is much appreciated


Comment 55 peter young 2006-09-13 13:02:45 UTC
Can you let me know when the FC5 patch is done ?

I managed to get the rawhide rescueCD to work if I used a fixed IP address 
instead of DHCP
On the whole the restore was successful

I did get this message

restore: file_location: EA set security.selinux:system_u:object_r:file_t 
failed: Invalid argument

but only for a few temporary and dev files, i.e. file_location of

./var/spool/at/.SEQ
./var/tmp/kdecache-root/ksycoca
./var/tmp/kdecache-root/ksycocastamp
./tmp/.vas_nss.vdb

/dev/null
/dev/mapper/control
/dev/mapper/VolGroup00-LogVol00
/dev/mapper/LogVol00


so this is a vast improvement

Is there a way I can get a rawhide recuse/install CD that contains working 
versions of sfdisk and ed ?


In my tests all files were written on FC5 - no upgrade was involved.

I get the warnings whether selinux is enabled, permissive or disabled. Once 
the extended attrubutes have been set, I can't find a way of removing them - 
is this correct ?






Comment 56 Stephen Smalley 2006-09-13 13:14:14 UTC
(In reply to comment #55)
> I get the warnings whether selinux is enabled, permissive or disabled. Once 
> the extended attrubutes have been set, I can't find a way of removing them - 
> is this correct ?

As long as SELinux is enabled, the kernel won't let you remove the SELinux
attributes (removexattr of security.selinux is prohibited - all data must be
labeled).  If you disable SELinux in the kernel (i.e. boot with selinux=0 or set
SELINUX=disabled in your /etc/selinux/config and reboot), then you can remove
the SELinux attributes (e.g. find / -exec setfattr -x security.selinux {} \;),
although that isn't a good idea unless you are sure you don't ever plan to
re-enable SELinux.



Comment 57 Fedora Update System 2006-09-15 01:45:08 UTC
dump-0.4b41-2.fc5 has been pushed for fc5, which should resolve this issue.  If these problems are still present in this version, then please make note of it in this bug report.