Re: [PATCH v4 3/4] fanotify, audit: Allow audit to use the full permission event response
On Thursday, September 8, 2022 10:41:44 PM EDT Richard Guy Briggs wrote:
> > I'm trying to abide by what was suggested by the fs-devel folks. I can
> > live with it. But if you want to make something non-generic for all
> > users of fanotify, call the new field "trusted". This would decern when
> > a decision was made because the file was untrusted or access denied for
> > another reason.
>
> So, "u32 trusted;" ? How would you like that formatted?
> "fan_trust={0|1}"
So how does this play out if there is another user? Do they want a num= and
trust= if not, then the AUDIT_FANOTIFY record will have multiple formats
which is not good. I'd rather suggest something generic that can be
interpreted based on who's attached to fanotify. IOW we have a fan_type=0 and
then followed by info0= info1= the interpretation of those solely depend on
fan_type. If the fan_type does not need both, then any interpretation skips
what it doesn't need. If fan_type=1, then it follows what arg0= and arg1= is
for that format. But make this pivot on fan_type and not actual names.
> > > You mention that you know what you want to put in the struct, why not
> > > share the details with all of us so we are all on the same page and
> > > can have a proper discussion.
> >
> > Because I want to abide by the original agreement and not impose
> > opinionated requirements that serve no one else. I'd rather have
> > something anyone can use. I want to play nice.
>
> If someone else wants to use something, why not give them a type of
> their own other than FAN_RESPONSE_INFO_AUDIT_RULE that they can shape
> however they like?
Please, let's keep AUDIT_FANOTIFY normalized but pivot on fan_type.
-Steve
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v4 3/4] fanotify,audit: Allow audit to use the full permission event response
On Thu, Sep 8, 2022 at 10:41 PM Richard Guy Briggs wrote:
> On 2022-09-08 22:20, Steve Grubb wrote:
> > On Thursday, September 8, 2022 5:22:15 PM EDT Paul Moore wrote:
> > > On Thu, Sep 8, 2022 at 5:14 PM Steve Grubb wrote:
> > > > On Wednesday, September 7, 2022 4:23:49 PM EDT Paul Moore wrote:
> > > > > On Wed, Sep 7, 2022 at 4:11 PM Steve Grubb wrote:
> > > > > > On Wednesday, September 7, 2022 2:43:54 PM EDT Richard Guy Briggs
> > wrote:
> > > > > > > > > Ultimately I guess I'll leave it upto audit subsystem what it
> > > > > > > > > wants
> > > > > > > > > to
> > > > > > > > > have in its struct fanotify_response_info_audit_rule because
> > > > > > > > > for
> > > > > > > > > fanotify subsystem, it is just an opaque blob it is passing.
> > > > > > > >
> > > > > > > > In that case, let's stick with leveraging the type/len fields in
> > > > > > > > the
> > > > > > > > fanotify_response_info_header struct, that should give us all
> > > > > > > > the
> > > > > > > > flexibility we need.
> > > > > > > >
> > > > > > > > Richard and Steve, it sounds like Steve is already aware of
> > > > > > > > additional
> > > > > > > > information that he wants to send via the
> > > > > > > > fanotify_response_info_audit_rule struct, please include that in
> > > > > > > > the
> > > > > > > > next revision of this patchset. I don't want to get this merged
> > > > > > > > and
> > > > > > > > then soon after have to hack in additional info.
> > > > > > >
> > > > > > > Steve, please define the type and name of this additional field.
> > > > > >
> > > > > > Maybe extra_data, app_data, or extra_info. Something generic that
> > > > > > can
> > > > > > be
> > > > > > reused by any application. Default to 0 if not present.
> > > > >
> > > > > I think the point is being missed ... The idea is to not speculate on
> > > > > additional fields, as discussed we have ways to handle that, the issue
> > > > > was that Steve implied that he already had ideas for "things" he
> > > > > wanted to add. If there are "things" that need to be added, let's do
> > > > > that now, however if there is just speculation that maybe someday we
> > > > > might need to add something else we can leave that until later.
> > > >
> > > > This is not speculation. I know what I want to put there. I know you
> > > > want
> > > > to pin it down to exactly what it is. However, when this started a
> > > > couple years back, one of the concerns was that we're building something
> > > > specific to 1 user of fanotify. And that it would be better for all
> > > > future users to have a generic facility that everyone could use if they
> > > > wanted to. That's why I'm suggesting something generic, its so this is
> > > > not special purpose that doesn't fit any other use case.
> > >
> > > Well, we are talking specifically about fanotify in this thread and
> > > dealing with data structures that are specific to fanotify. I can
> > > understand wanting to future proof things, but based on what we've
> > > seen in this thread I think we are all set in this regard.
> >
> > I'm trying to abide by what was suggested by the fs-devel folks. I can live
> > with it. But if you want to make something non-generic for all users of
> > fanotify, call the new field "trusted". This would decern when a decision
> > was
> > made because the file was untrusted or access denied for another reason.
>
> So, "u32 trusted;" ? How would you like that formatted?
> "fan_trust={0|1}"
>
> > > You mention that you know what you want to put in the struct, why not
> > > share the details with all of us so we are all on the same page and
> > > can have a proper discussion.
> >
> > Because I want to abide by the original agreement and not impose opinionated
> > requirements that serve no one else. I'd rather have something anyone can
> > use. I want to play nice.
>
> If someone else wants to use something, why not give them a type of
> their own other than FAN_RESPONSE_INFO_AUDIT_RULE that they can shape
> however they like?
Yes, exactly. The struct is very clearly specific to both fanotify
and audit, I see no reason why it needs to be made generic for use by
other subsystems when other mechanisms exist to support them.
--
paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v4 3/4] fanotify,audit: Allow audit to use the full permission event response
On 2022-09-08 22:20, Steve Grubb wrote:
> On Thursday, September 8, 2022 5:22:15 PM EDT Paul Moore wrote:
> > On Thu, Sep 8, 2022 at 5:14 PM Steve Grubb wrote:
> > > On Wednesday, September 7, 2022 4:23:49 PM EDT Paul Moore wrote:
> > > > On Wed, Sep 7, 2022 at 4:11 PM Steve Grubb wrote:
> > > > > On Wednesday, September 7, 2022 2:43:54 PM EDT Richard Guy Briggs
> wrote:
> > > > > > > > Ultimately I guess I'll leave it upto audit subsystem what it
> > > > > > > > wants
> > > > > > > > to
> > > > > > > > have in its struct fanotify_response_info_audit_rule because
> > > > > > > > for
> > > > > > > > fanotify subsystem, it is just an opaque blob it is passing.
> > > > > > >
> > > > > > > In that case, let's stick with leveraging the type/len fields in
> > > > > > > the
> > > > > > > fanotify_response_info_header struct, that should give us all the
> > > > > > > flexibility we need.
> > > > > > >
> > > > > > > Richard and Steve, it sounds like Steve is already aware of
> > > > > > > additional
> > > > > > > information that he wants to send via the
> > > > > > > fanotify_response_info_audit_rule struct, please include that in
> > > > > > > the
> > > > > > > next revision of this patchset. I don't want to get this merged
> > > > > > > and
> > > > > > > then soon after have to hack in additional info.
> > > > > >
> > > > > > Steve, please define the type and name of this additional field.
> > > > >
> > > > > Maybe extra_data, app_data, or extra_info. Something generic that can
> > > > > be
> > > > > reused by any application. Default to 0 if not present.
> > > >
> > > > I think the point is being missed ... The idea is to not speculate on
> > > > additional fields, as discussed we have ways to handle that, the issue
> > > > was that Steve implied that he already had ideas for "things" he
> > > > wanted to add. If there are "things" that need to be added, let's do
> > > > that now, however if there is just speculation that maybe someday we
> > > > might need to add something else we can leave that until later.
> > >
> > > This is not speculation. I know what I want to put there. I know you want
> > > to pin it down to exactly what it is. However, when this started a
> > > couple years back, one of the concerns was that we're building something
> > > specific to 1 user of fanotify. And that it would be better for all
> > > future users to have a generic facility that everyone could use if they
> > > wanted to. That's why I'm suggesting something generic, its so this is
> > > not special purpose that doesn't fit any other use case.
> >
> > Well, we are talking specifically about fanotify in this thread and
> > dealing with data structures that are specific to fanotify. I can
> > understand wanting to future proof things, but based on what we've
> > seen in this thread I think we are all set in this regard.
>
> I'm trying to abide by what was suggested by the fs-devel folks. I can live
> with it. But if you want to make something non-generic for all users of
> fanotify, call the new field "trusted". This would decern when a decision was
> made because the file was untrusted or access denied for another reason.
So, "u32 trusted;" ? How would you like that formatted?
"fan_trust={0|1}"
> > You mention that you know what you want to put in the struct, why not
> > share the details with all of us so we are all on the same page and
> > can have a proper discussion.
>
> Because I want to abide by the original agreement and not impose opinionated
> requirements that serve no one else. I'd rather have something anyone can
> use. I want to play nice.
If someone else wants to use something, why not give them a type of
their own other than FAN_RESPONSE_INFO_AUDIT_RULE that they can shape
however they like?
> -Steve
- RGB
--
Richard Guy Briggs
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v4 3/4] fanotify, audit: Allow audit to use the full permission event response
On Thursday, September 8, 2022 5:22:15 PM EDT Paul Moore wrote: > On Thu, Sep 8, 2022 at 5:14 PM Steve Grubb wrote: > > On Wednesday, September 7, 2022 4:23:49 PM EDT Paul Moore wrote: > > > On Wed, Sep 7, 2022 at 4:11 PM Steve Grubb wrote: > > > > On Wednesday, September 7, 2022 2:43:54 PM EDT Richard Guy Briggs wrote: > > > > > > > Ultimately I guess I'll leave it upto audit subsystem what it > > > > > > > wants > > > > > > > to > > > > > > > have in its struct fanotify_response_info_audit_rule because > > > > > > > for > > > > > > > fanotify subsystem, it is just an opaque blob it is passing. > > > > > > > > > > > > In that case, let's stick with leveraging the type/len fields in > > > > > > the > > > > > > fanotify_response_info_header struct, that should give us all the > > > > > > flexibility we need. > > > > > > > > > > > > Richard and Steve, it sounds like Steve is already aware of > > > > > > additional > > > > > > information that he wants to send via the > > > > > > fanotify_response_info_audit_rule struct, please include that in > > > > > > the > > > > > > next revision of this patchset. I don't want to get this merged > > > > > > and > > > > > > then soon after have to hack in additional info. > > > > > > > > > > Steve, please define the type and name of this additional field. > > > > > > > > Maybe extra_data, app_data, or extra_info. Something generic that can > > > > be > > > > reused by any application. Default to 0 if not present. > > > > > > I think the point is being missed ... The idea is to not speculate on > > > additional fields, as discussed we have ways to handle that, the issue > > > was that Steve implied that he already had ideas for "things" he > > > wanted to add. If there are "things" that need to be added, let's do > > > that now, however if there is just speculation that maybe someday we > > > might need to add something else we can leave that until later. > > > > This is not speculation. I know what I want to put there. I know you want > > to pin it down to exactly what it is. However, when this started a > > couple years back, one of the concerns was that we're building something > > specific to 1 user of fanotify. And that it would be better for all > > future users to have a generic facility that everyone could use if they > > wanted to. That's why I'm suggesting something generic, its so this is > > not special purpose that doesn't fit any other use case. > > Well, we are talking specifically about fanotify in this thread and > dealing with data structures that are specific to fanotify. I can > understand wanting to future proof things, but based on what we've > seen in this thread I think we are all set in this regard. I'm trying to abide by what was suggested by the fs-devel folks. I can live with it. But if you want to make something non-generic for all users of fanotify, call the new field "trusted". This would decern when a decision was made because the file was untrusted or access denied for another reason. > You mention that you know what you want to put in the struct, why not > share the details with all of us so we are all on the same page and > can have a proper discussion. Because I want to abide by the original agreement and not impose opinionated requirements that serve no one else. I'd rather have something anyone can use. I want to play nice. -Steve -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: LSM stacking in next for 6.1?
On 9/8/2022 12:32 PM, Paul Moore wrote: > On Thu, Sep 8, 2022 at 2:05 PM Casey Schaufler wrote: >> On 9/7/2022 8:57 PM, Paul Moore wrote: >>> On Wed, Sep 7, 2022 at 7:53 PM Casey Schaufler >>> wrote: On 9/7/2022 4:27 PM, Paul Moore wrote: > .. > > The ease-of-use quality is a bit subjective, but it does need > another interface to use properly and it requires string parsing which > history has shown to be an issue time and time again (although it is > relatively simple here). There was a lot of discussion regarding that. My proposed apparmor="unconfined",smack="User" format was panned for those same reasons. The nil byte format has been used elsewhere and suggested for that reason. >>> Based on what I recall from those discussions, it was my impression >>> the nil byte label delimiter was suggested simply because no one was >>> entertaining the idea of using something other than the existing >>> procfs interface. It is my opinion that we've taken that interface >>> about as far as it can go, and while it needs to stay intact for >>> compatibility reasons, the LSM stacking functionality should move to a >>> different API that is better suited for it. >> It's going to raise its ugly head again with SO_PEERCONTEXT for the >> SELinux+Smack case. But we can cross that bridge when we come to it. > There are also problems with IP_PASSSEC/SCM_SECURITY that we've never > fully resolved (and have gotten a bit lucky over the years); it very > well could be time to add support for IP_SECURITY as the multi-LSM > replacement for SCM_SECURITY. We could leverage the same LSM context > structures as in the other multi-LSM interfaces. Existing > applications could continue to use SCM_SECURITY; in fact I believe we > could have both SCM_SECURITY and IP_SECURITY in the same message for > maximum compatibility. Adding IP_SECURITY seems sensible. Having both at the same time leaves us with the question of which security module's value to put in SCM_SECURITY when there are multiple choices. I assume we'd use the first available value, as determined by the LSM list order, or the interface_lsm if we're supporting that concept. And again, that's a problem for the complete stacking round. > https://github.com/SELinuxProject/selinux-kernel/issues/24 > > For SO_PEERSEC, we should probably just introduce SO_PEERSEC2 or > similar, using the same multi-LSM context structures as the other > interfaces. Also sensible, although I think SO_PEERCONTEXT or SO_PEERCTX is a better name than SO_PEERSEC2. Also a problem for complete stacking and it is way to early for me to get into bikeshedding. >>> In case it helps spur your imagination, here is a revised strawman: >>> >>> /** >>> * struct lsm_ctx - LSM context >>> * @id: the LSM id number, see LSM_ID_XXX >> A LSM ID hard coded in a kernel header makes it harder to develop new >> security modules. > There is so much precedence for defining a token scalar value to > represent a "thing" that I don't know where to begin. Look at IANA, > there are entire organizations that exist to map "things" to numbers. > > If you're objecting to assigning LSMs fixed integer numbers you've got > to give me some very explicit reasons (complete with examples) as to > why that would be a mistake. I talked myself out of the objection below. Thanks for listening. >> The security module can't be self contained. I say >> that, but I acknowledge that I've done the same kind of thing with the >> definition of the struct lsmblob. That isn't part of an external API >> however. > I'm not following you here. See my comment above about better examples. > >> It may also interfere with Tetsuo's long standing request that >> we don't do things that prevent the possibility of loadable security >> modules at some point in the future. > I already replied to Tetsuo's email, and while this particular point > about LSM ID numbers wasn't directly addressed, my response there > seems to apply equally well here: it's not so much about loadable > LSMs, it's about out-of-tree LSMs. These are two very different > things, with different solutions. Agreed. >> On the other hand, there's no great way to include two variable length >> strings in a structure like this. So unless we adopt something as ugly >> as the nil byte scheme this is supposed to replace I expect we're stuck >> with an LSM ID. > I don't like making general comments, but when in doubt, consider me > not-a-fan of string-based identifiers in APIs. Give me token scalar > values instead. Works for me. >>> * @flags: LSM specific flags, zero if unused >> For an API shouldn't this be a specific size? u32? I'm not really >> up to date on the guidance regarding which it should be. > Enh, sure, whatever. You'll remember my initial comment about not > being a syscall stylist; if the discussion has moved to seriously > discussing the syscall prototypes we should likely start a new thread > and bring in the syscall folks ..
Re: [PATCH v4 3/4] fanotify,audit: Allow audit to use the full permission event response
On Thu, Sep 8, 2022 at 5:14 PM Steve Grubb wrote: > On Wednesday, September 7, 2022 4:23:49 PM EDT Paul Moore wrote: > > On Wed, Sep 7, 2022 at 4:11 PM Steve Grubb wrote: > > > On Wednesday, September 7, 2022 2:43:54 PM EDT Richard Guy Briggs wrote: > > > > > > Ultimately I guess I'll leave it upto audit subsystem what it wants > > > > > > to > > > > > > have in its struct fanotify_response_info_audit_rule because for > > > > > > fanotify subsystem, it is just an opaque blob it is passing. > > > > > > > > > > In that case, let's stick with leveraging the type/len fields in the > > > > > fanotify_response_info_header struct, that should give us all the > > > > > flexibility we need. > > > > > > > > > > Richard and Steve, it sounds like Steve is already aware of > > > > > additional > > > > > information that he wants to send via the > > > > > fanotify_response_info_audit_rule struct, please include that in the > > > > > next revision of this patchset. I don't want to get this merged and > > > > > then soon after have to hack in additional info. > > > > > > > > Steve, please define the type and name of this additional field. > > > > > > Maybe extra_data, app_data, or extra_info. Something generic that can be > > > reused by any application. Default to 0 if not present. > > > > I think the point is being missed ... The idea is to not speculate on > > additional fields, as discussed we have ways to handle that, the issue > > was that Steve implied that he already had ideas for "things" he > > wanted to add. If there are "things" that need to be added, let's do > > that now, however if there is just speculation that maybe someday we > > might need to add something else we can leave that until later. > > This is not speculation. I know what I want to put there. I know you want to > pin it down to exactly what it is. However, when this started a couple years > back, one of the concerns was that we're building something specific to 1 user > of fanotify. And that it would be better for all future users to have a > generic facility that everyone could use if they wanted to. That's why I'm > suggesting something generic, its so this is not special purpose that doesn't > fit any other use case. Well, we are talking specifically about fanotify in this thread and dealing with data structures that are specific to fanotify. I can understand wanting to future proof things, but based on what we've seen in this thread I think we are all set in this regard. You mention that you know what you want to put in the struct, why not share the details with all of us so we are all on the same page and can have a proper discussion. -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v4 3/4] fanotify, audit: Allow audit to use the full permission event response
On Wednesday, September 7, 2022 4:23:49 PM EDT Paul Moore wrote: > On Wed, Sep 7, 2022 at 4:11 PM Steve Grubb wrote: > > On Wednesday, September 7, 2022 2:43:54 PM EDT Richard Guy Briggs wrote: > > > > > Ultimately I guess I'll leave it upto audit subsystem what it wants > > > > > to > > > > > have in its struct fanotify_response_info_audit_rule because for > > > > > fanotify subsystem, it is just an opaque blob it is passing. > > > > > > > > In that case, let's stick with leveraging the type/len fields in the > > > > fanotify_response_info_header struct, that should give us all the > > > > flexibility we need. > > > > > > > > Richard and Steve, it sounds like Steve is already aware of > > > > additional > > > > information that he wants to send via the > > > > fanotify_response_info_audit_rule struct, please include that in the > > > > next revision of this patchset. I don't want to get this merged and > > > > then soon after have to hack in additional info. > > > > > > Steve, please define the type and name of this additional field. > > > > Maybe extra_data, app_data, or extra_info. Something generic that can be > > reused by any application. Default to 0 if not present. > > I think the point is being missed ... The idea is to not speculate on > additional fields, as discussed we have ways to handle that, the issue > was that Steve implied that he already had ideas for "things" he > wanted to add. If there are "things" that need to be added, let's do > that now, however if there is just speculation that maybe someday we > might need to add something else we can leave that until later. This is not speculation. I know what I want to put there. I know you want to pin it down to exactly what it is. However, when this started a couple years back, one of the concerns was that we're building something specific to 1 user of fanotify. And that it would be better for all future users to have a generic facility that everyone could use if they wanted to. That's why I'm suggesting something generic, its so this is not special purpose that doesn't fit any other use case. -Steve -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: LSM stacking in next for 6.1?
On Thu, Sep 8, 2022 at 2:05 PM Casey Schaufler wrote:
> On 9/7/2022 8:57 PM, Paul Moore wrote:
> > On Wed, Sep 7, 2022 at 7:53 PM Casey Schaufler
> > wrote:
> >> On 9/7/2022 4:27 PM, Paul Moore wrote:
...
> >>> The ease-of-use quality is a bit subjective, but it does need
> >>> another interface to use properly and it requires string parsing which
> >>> history has shown to be an issue time and time again (although it is
> >>> relatively simple here).
> >> There was a lot of discussion regarding that. My proposed
> >> apparmor="unconfined",smack="User" format was panned for those same
> >> reasons.
> >> The nil byte format has been used elsewhere and suggested for that reason.
> > Based on what I recall from those discussions, it was my impression
> > the nil byte label delimiter was suggested simply because no one was
> > entertaining the idea of using something other than the existing
> > procfs interface. It is my opinion that we've taken that interface
> > about as far as it can go, and while it needs to stay intact for
> > compatibility reasons, the LSM stacking functionality should move to a
> > different API that is better suited for it.
>
> It's going to raise its ugly head again with SO_PEERCONTEXT for the
> SELinux+Smack case. But we can cross that bridge when we come to it.
There are also problems with IP_PASSSEC/SCM_SECURITY that we've never
fully resolved (and have gotten a bit lucky over the years); it very
well could be time to add support for IP_SECURITY as the multi-LSM
replacement for SCM_SECURITY. We could leverage the same LSM context
structures as in the other multi-LSM interfaces. Existing
applications could continue to use SCM_SECURITY; in fact I believe we
could have both SCM_SECURITY and IP_SECURITY in the same message for
maximum compatibility.
https://github.com/SELinuxProject/selinux-kernel/issues/24
For SO_PEERSEC, we should probably just introduce SO_PEERSEC2 or
similar, using the same multi-LSM context structures as the other
interfaces.
> > In case it helps spur your imagination, here is a revised strawman:
> >
> > /**
> > * struct lsm_ctx - LSM context
> > * @id: the LSM id number, see LSM_ID_XXX
>
> A LSM ID hard coded in a kernel header makes it harder to develop new
> security modules.
There is so much precedence for defining a token scalar value to
represent a "thing" that I don't know where to begin. Look at IANA,
there are entire organizations that exist to map "things" to numbers.
If you're objecting to assigning LSMs fixed integer numbers you've got
to give me some very explicit reasons (complete with examples) as to
why that would be a mistake.
> The security module can't be self contained. I say
> that, but I acknowledge that I've done the same kind of thing with the
> definition of the struct lsmblob. That isn't part of an external API
> however.
I'm not following you here. See my comment above about better examples.
> It may also interfere with Tetsuo's long standing request that
> we don't do things that prevent the possibility of loadable security
> modules at some point in the future.
I already replied to Tetsuo's email, and while this particular point
about LSM ID numbers wasn't directly addressed, my response there
seems to apply equally well here: it's not so much about loadable
LSMs, it's about out-of-tree LSMs. These are two very different
things, with different solutions.
> On the other hand, there's no great way to include two variable length
> strings in a structure like this. So unless we adopt something as ugly
> as the nil byte scheme this is supposed to replace I expect we're stuck
> with an LSM ID.
I don't like making general comments, but when in doubt, consider me
not-a-fan of string-based identifiers in APIs. Give me token scalar
values instead.
> > * @flags: LSM specific flags, zero if unused
>
> For an API shouldn't this be a specific size? u32? I'm not really
> up to date on the guidance regarding which it should be.
Enh, sure, whatever. You'll remember my initial comment about not
being a syscall stylist; if the discussion has moved to seriously
discussing the syscall prototypes we should likely start a new thread
and bring in the syscall folks ... I vaguely remember there was a
mailing list for syscalls and API changes ...
> I will head in this direction. A couple questions:
>
> Would we want lsm_prev_ctx() as well as lsm_current_ctx(),
I'm not sure I'm following your thinking, what would lsm_prev_ctx() do?
> or should we use the lsm_ctx->flags to identify the provided
> context? If we did that we should have an lsm_ctx() system call
> that returns the current, prev, and whatever other security
> module specific attributes might be associated with the process,
> each identified in the flags field. While the "current" context
> is usually what we're after, there may be cases where the other
> attributes are desired.
I don't understand what you mean by "prev"{ious} attributes. I'm
thinking of lsm_curren
Re: LSM stacking in next for 6.1?
On Thu, Sep 8, 2022 at 11:19 AM Tetsuo Handa wrote: > On 2022/08/03 9:01, Casey Schaufler wrote: > > I would like very much to get v38 or v39 of the LSM stacking for Apparmor > > patch set in the LSM next branch for 6.1. The audit changes have polished > > up nicely and I believe that all comments on the integrity code have been > > addressed. The interface_lsm mechanism has been beaten to a frothy peak. > > There are serious binder changes, but I think they address issues beyond > > the needs of stacking. Changes outside these areas are pretty well limited > > to LSM interface improvements. > Many modules > > SimpleFlow （ 2016/04/21 https://lwn.net/Articles/684825/ ） > HardChroot （ 2016/07/29 https://lwn.net/Articles/695984/ ） > Checmate （ 2016/08/04 https://lwn.net/Articles/696344/ ） > LandLock （ 2016/08/25 https://lwn.net/Articles/698226/ ） > PTAGS （ 2016/09/29 https://lwn.net/Articles/702639/ ） > CaitSith （ 2016/10/21 https://lwn.net/Articles/704262/ ） > SafeName （ 2016/05/03 https://lwn.net/Articles/686021/ ） > WhiteEgret （ 2017/05/30 https://lwn.net/Articles/724192/ ） > shebang （ 2017/06/09 https://lwn.net/Articles/725285/ ） > S.A.R.A. （ 2017/06/13 https://lwn.net/Articles/725230/ ） > > are proposed 5 or 6 years ago, but mostly became silent... At least one of those, Landlock, has been merged upstream and is now available in modern released Linux Kernels. As far as the other LSMs are concerned, I don't recall there ever being significant interest among other developers or users to warrant their inclusion upstream. If the authors believe that has changed, or is simply not true, they are always welcome to post their patches again for discussion, review, and potential upstreaming. However, I will caution that it is becoming increasingly difficult for people to find time to review potential new LSMs so it may a while to attract sufficient comments and feedback. > I still need byte-code analysis for finding the hook and code for making the > hook > writable in AKARI/CaitSith due to lack of > EXPORT_SYMBOL_GPL(security_add_hooks). > I wonder when I can stop questions like > https://osdn.net/projects/tomoyo/lists/archive/users-en/2022-September/000740.html > caused by > https://patchwork.kernel.org/project/linux-security-module/patch/alpine.lrh.2.20.1702131631490.8...@namei.org/ > . As has been discussed before, this isn't so much an issue with the __ro_after_init change, it's really more of an issue of running out-of-tree kernel code on pre-built distribution kernels, with "pre-built" being the most important part. It is my understanding that if the user/developer built their own patched kernel this would not likely be an issue as the out-of-tree LSM could be patched into the kernel source. The problem comes when the user/developer wants to dynamically load their out-of-tree LSM into a pre-built distribution kernel, presumably to preserve a level of distribution support. Unfortunately, to the best of my knowledge, none of the major enterprise Linux distributions will provide support for arbitrary third-party kernel modules (it may work, but if something fails the user is on their own to triage and resolve). Beyond the support issue, there are likely to be other problems as well since the kernel interfaces, including the LSM hooks themselves, are not guaranteed to be stable across kernel releases. > Last 10 years, my involvement with Linux kernel is "fixing bugs" rather than > "developing security mechanisms". Changes what I found in the past 10 years > are: > > As far as I'm aware, more than 99% of systems still disable SELinux. I would challenge you to support that claim with data. Granted, we are coming from very different LSM backgrounds, but I find that number very suspect. It has been several years since I last looked, but I believe the latest published Android numbers would give some support to the idea that more than 1% of SELinux based systems are running in enforcing (or permissive) mode. Significantly more. > People use RHEL, > but the reason to choose RHEL is not because RHEL supports SELinux. Once again, if you are going to make strong claims such as this, please provide data. I know of several RHEL users that are only able to run SELinux based systems as it is the only LSM which meets their security requirements. > Instead, Ubuntu users are increasing, but the reason people choose Ubuntu > is not because > Ubuntu supports AppArmor. Maybe because easy to use container environment. > Maybe because > available as Windows Subsystem for Linux. I suspect IBM/RH's decision to change CentOS' relationship to RHEL also resulted in a number of users moving to Ubuntu, and that has nothing to do with the LSMs. > However, in many cases, it seems that whether the OS is Windows or Linux no > longer > matters. Programs are written using frameworks/languages which developers > hardly care > about Windows API or Linux syscal
Re: LSM stacking in next for 6.1?
On 9/8/22 11:05, Casey Schaufler wrote: On 9/7/2022 8:57 PM, Paul Moore wrote: On Wed, Sep 7, 2022 at 7:53 PM Casey Schaufler wrote: On 9/7/2022 4:27 PM, Paul Moore wrote: .. I just want an interface that is clearly defined, has reasonable capacity to be extended in the future as needed, and is easy(ish) to use and support over extended periods of time (both from a kernel and userspace perspective). The "smack_label\0apparmor_label\0selinux_label\0future_lsm_label\0" string interface is none of these things. That wasn't the proposal. The proposal was "smack\0smack_label\0apparmor\0apparmor_label\0future_lsm\0future_lsm_label\0" In this we disagree I think we can both agree that there is a subjective aspect to this argument and it may be that we never reach agreement on the "best" approach, if there even is such a thing. What I am trying to do here is describe a path that would allow me to be more comfortable merging the LSM stacking functionality; I don't think you've had a clearly defined path, of any sort, towards getting these patches merged, which is a problem and I suspect the source of some of the frustration. My comments, as objectionable as you may find them to be, are intended to help you move forward with these patches. OK. Let's get'er done. It is not clearly defined as it requires other interfaces to associate the labels with the correct LSMs. The label follows the lsm name directly. What other association is required? You need to know the order of the LSMs in order to interpret/de-serialize the string. That's true for the string you included, but not for what I had actually proposed. The ease-of-use quality is a bit subjective, but it does need another interface to use properly and it requires string parsing which history has shown to be an issue time and time again (although it is relatively simple here). There was a lot of discussion regarding that. My proposed apparmor="unconfined",smack="User" format was panned for those same reasons. The nil byte format has been used elsewhere and suggested for that reason. Based on what I recall from those discussions, it was my impression the nil byte label delimiter was suggested simply because no one was entertaining the idea of using something other than the existing procfs interface. It is my opinion that we've taken that interface about as far as it can go, and while it needs to stay intact for compatibility reasons, the LSM stacking functionality should move to a different API that is better suited for it. It's going to raise its ugly head again with SO_PEERCONTEXT for the SELinux+Smack case. But we can cross that bridge when we come to it. AppArmor too, I am working on revising the out of tree af_unix mediation Once again, the syscall example I tossed out was a quick strawman, but it had clearly separated and defined labels conveyed in data structures that didn't require string parsing to find the label associated with an LSM. True, but it uses pointers internally and you can't do that in memory you're sending up from the system. What comes from the syscall has to look something like the nil byte format. The strawman would have to do the same sort of parsing in userspace that you are objecting to. Then we change the strawman. That's pretty much the definition of a strawman example, it's something you toss out as starting point for discussion and future improvements. If it was something much more fully developed I would have submitted a patch sheesh. Fair enough. In case it helps spur your imagination, here is a revised strawman: /** * struct lsm_ctx - LSM context * @id: the LSM id number, see LSM_ID_XXX A LSM ID hard coded in a kernel header makes it harder to develop new security modules. The security module can't be self contained. I say that, but I acknowledge that I've done the same kind of thing with the definition of the struct lsmblob. That isn't part of an external API however. It may also interfere with Tetsuo's long standing request that we don't do things that prevent the possibility of loadable security modules at some point in the future. I will also mention the out-of-tree security module objection, not because I care, but because someone most likely will bring it up. On the other hand, there's no great way to include two variable length strings in a structure like this. So unless we adopt something as ugly as the nil byte scheme this is supposed to replace I expect we're stuck with an LSM ID. well at a minimum we shouldn't export the kernel internal LSM_ID if its exposed to userspace it needs to be something that can live with for a long time - Fixed length strings, which really are just a long LSM ID, Say 8 bytes. Can still even look human readable. For most* LSMs this could just be their name. * only safesetid and capability don't fit atm - and certainly uglier, for variable length use an index for one of the variable length strings, with an em
Re: LSM stacking in next for 6.1?
On 9/7/2022 8:57 PM, Paul Moore wrote:
> On Wed, Sep 7, 2022 at 7:53 PM Casey Schaufler wrote:
>> On 9/7/2022 4:27 PM, Paul Moore wrote:
> ..
>
>>> I
>>> just want an interface that is clearly defined, has reasonable
>>> capacity to be extended in the future as needed, and is easy(ish) to
>>> use and support over extended periods of time (both from a kernel and
>>> userspace perspective).
>>>
>>> The "smack_label\0apparmor_label\0selinux_label\0future_lsm_label\0"
>>> string interface is none of these things.
That wasn't the proposal. The proposal was
"smack\0smack_label\0apparmor\0apparmor_label\0future_lsm\0future_lsm_label\0"
>> In this we disagree
> I think we can both agree that there is a subjective aspect to this
> argument and it may be that we never reach agreement on the "best"
> approach, if there even is such a thing. What I am trying to do here
> is describe a path that would allow me to be more comfortable merging
> the LSM stacking functionality; I don't think you've had a clearly
> defined path, of any sort, towards getting these patches merged, which
> is a problem and I suspect the source of some of the frustration. My
> comments, as objectionable as you may find them to be, are intended to
> help you move forward with these patches.
OK. Let's get'er done.
>>> It is not clearly defined
>>> as it requires other interfaces to associate the labels with the
>>> correct LSMs.
>> The label follows the lsm name directly. What other association is required?
> You need to know the order of the LSMs in order to
> interpret/de-serialize the string.
That's true for the string you included, but not for what I had
actually proposed.
>>> The ease-of-use quality is a bit subjective, but it does need
>>> another interface to use properly and it requires string parsing which
>>> history has shown to be an issue time and time again (although it is
>>> relatively simple here).
>> There was a lot of discussion regarding that. My proposed
>> apparmor="unconfined",smack="User" format was panned for those same reasons.
>> The nil byte format has been used elsewhere and suggested for that reason.
> Based on what I recall from those discussions, it was my impression
> the nil byte label delimiter was suggested simply because no one was
> entertaining the idea of using something other than the existing
> procfs interface. It is my opinion that we've taken that interface
> about as far as it can go, and while it needs to stay intact for
> compatibility reasons, the LSM stacking functionality should move to a
> different API that is better suited for it.
It's going to raise its ugly head again with SO_PEERCONTEXT for the
SELinux+Smack case. But we can cross that bridge when we come to it.
>>> Once again, the syscall example I tossed out was a quick strawman, but
>>> it had clearly separated and defined labels conveyed in data
>>> structures that didn't require string parsing to find the label
>>> associated with an LSM.
>> True, but it uses pointers internally and you can't do that in memory
>> you're sending up from the system. What comes from the syscall has to
>> look something like the nil byte format. The strawman would have to do
>> the same sort of parsing in userspace that you are objecting to.
> Then we change the strawman. That's pretty much the definition of a
> strawman example, it's something you toss out as starting point for
> discussion and future improvements. If it was something much more
> fully developed I would have submitted a patch sheesh.
Fair enough.
> In case it helps spur your imagination, here is a revised strawman:
>
> /**
> * struct lsm_ctx - LSM context
> * @id: the LSM id number, see LSM_ID_XXX
A LSM ID hard coded in a kernel header makes it harder to develop new
security modules. The security module can't be self contained. I say
that, but I acknowledge that I've done the same kind of thing with the
definition of the struct lsmblob. That isn't part of an external API
however. It may also interfere with Tetsuo's long standing request that
we don't do things that prevent the possibility of loadable security
modules at some point in the future. I will also mention the out-of-tree
security module objection, not because I care, but because someone most
likely will bring it up.
On the other hand, there's no great way to include two variable length
strings in a structure like this. So unless we adopt something as ugly
as the nil byte scheme this is supposed to replace I expect we're stuck
with an LSM ID.
>
> * @flags: LSM specific flags, zero if unused
For an API shouldn't this be a specific size? u32? I'm not really
up to date on the guidance regarding which it should be.
> * @ctx_len: the size of @ctx
> * @ctx: the LSM context
> */
> struct lsm_ctx {
> unsigned in id;
> unsigned int flags;
> size_t ctx_len;
> unsigned char ctx[];
> };
>
> /**
> * lsm_current_ctx - Return current tasks's LSM context
> * @ctx: the LSM contexts
> * @siz
Re: LSM stacking in next for 6.1?
On 2022/08/03 9:01, Casey Schaufler wrote: > I would like very much to get v38 or v39 of the LSM stacking for Apparmor > patch set in the LSM next branch for 6.1. The audit changes have polished > up nicely and I believe that all comments on the integrity code have been > addressed. The interface_lsm mechanism has been beaten to a frothy peak. > There are serious binder changes, but I think they address issues beyond > the needs of stacking. Changes outside these areas are pretty well limited > to LSM interface improvements. > After ((SELinux xor Smack) and AppArmor) is made possible in next for 6.1, what comes next? Are you planning to make (SELinux and Smack and AppArmor) possible? My concern is, when loadable LSM modules becomes legal, for I'm refraining from again proposing CaitSith until LSM stacking completes. Linus Torvalds said You security people are insane. I'm tired of this "only my version is correct" crap. at https://lkml.kernel.org/r/alpine.lfd.0.999.0710010803280.3...@woody.linux-foundation.org . Many modules SimpleFlow （ 2016/04/21 https://lwn.net/Articles/684825/ ） HardChroot （ 2016/07/29 https://lwn.net/Articles/695984/ ） Checmate （ 2016/08/04 https://lwn.net/Articles/696344/ ） LandLock （ 2016/08/25 https://lwn.net/Articles/698226/ ） PTAGS （ 2016/09/29 https://lwn.net/Articles/702639/ ） CaitSith （ 2016/10/21 https://lwn.net/Articles/704262/ ） SafeName （ 2016/05/03 https://lwn.net/Articles/686021/ ） WhiteEgret （ 2017/05/30 https://lwn.net/Articles/724192/ ） shebang （ 2017/06/09 https://lwn.net/Articles/725285/ ） S.A.R.A. （ 2017/06/13 https://lwn.net/Articles/725230/ ） are proposed 5 or 6 years ago, but mostly became silent... I still need byte-code analysis for finding the hook and code for making the hook writable in AKARI/CaitSith due to lack of EXPORT_SYMBOL_GPL(security_add_hooks). I wonder when I can stop questions like https://osdn.net/projects/tomoyo/lists/archive/users-en/2022-September/000740.html caused by https://patchwork.kernel.org/project/linux-security-module/patch/alpine.lrh.2.20.1702131631490.8...@namei.org/ . Last 10 years, my involvement with Linux kernel is "fixing bugs" rather than "developing security mechanisms". Changes what I found in the past 10 years are: As far as I'm aware, more than 99% of systems still disable SELinux. People use RHEL, but the reason to choose RHEL is not because RHEL supports SELinux. The only thing changed is that the way to disable SELinux changed from SELINUX=disabled in /etc/selinux/config to selinux=0 on kernel command line options. Instead, Ubuntu users are increasing, but the reason people choose Ubuntu is not because Ubuntu supports AppArmor. Maybe because easy to use container environment. Maybe because available as Windows Subsystem for Linux. However, in many cases, it seems that whether the OS is Windows or Linux no longer matters. Programs are written using frameworks/languages which developers hardly care about Windows API or Linux syscall. LSM significantly focuses on syscalls, but the trend might no longer be trying to solve in the LSM layer... Also, Linux servers started using AntiVirus software. Enterprise AntiVirus software uses loadable kernel module that rewrites system call table rather than using LSM interface. It seems that people prefer out-of-the-box security over fine grained access control rule based security. In other words, it seems that allowlist based LSM modules are too difficult for normal users. Maybe it is better for normal users to develop and use single-function LSMs than try to utilize ((SELinux xor Smack) and AppArmor)... But still loadable LSM modules are not legally available... -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: LSM stacking in next for 6.1?
On 9/8/2022 8:18 AM, Tetsuo Handa wrote: > On 2022/08/03 9:01, Casey Schaufler wrote: >> I would like very much to get v38 or v39 of the LSM stacking for Apparmor >> patch set in the LSM next branch for 6.1. The audit changes have polished >> up nicely and I believe that all comments on the integrity code have been >> addressed. The interface_lsm mechanism has been beaten to a frothy peak. >> There are serious binder changes, but I think they address issues beyond >> the needs of stacking. Changes outside these areas are pretty well limited >> to LSM interface improvements. >> > After ((SELinux xor Smack) and AppArmor) is made possible in next for 6.1, > what > comes next? Are you planning to make (SELinux and Smack and AppArmor) > possible? The stacking isn't going into 6.1. Paul's decision that LSM system calls are a requirement will require additional work. With LSS-EU next week and a long overdue trip on my part in early October I don't see 6.2 as at all likely, either. The commitment I made to Paul some years ago now was that the stacking would eventually include making all combinations possible. That is still my plan. The issues beyond ((SELinux xor Smack) and AppArmor) are primarily network related. Secmarks, netlabel and SO_PEERSEC in particular. I have posted preliminary patches in the past, but they need to be revisited in light of development that has occurred since then. On the other hand, I'm getting the "when are you going to retire?" question way way way too regularly these days. While I intend to complete the work as promised, I don't expect to be working on much with significant importance come 2030. > My concern is, when loadable LSM modules becomes legal, for I'm refraining > from > again proposing CaitSith until LSM stacking completes. I would not wait. There is no reason the efforts cannot progress in parallel. > Linus Torvalds said > > You security people are insane. I'm tired of this "only my version is > correct" crap. My favorite Linus quote of all time. I try to include it in every presentation I give. > at > https://lkml.kernel.org/r/alpine.lfd.0.999.0710010803280.3...@woody.linux-foundation.org > . > > Many modules > > SimpleFlow （ 2016/04/21 https://lwn.net/Articles/684825/ ） > HardChroot （ 2016/07/29 https://lwn.net/Articles/695984/ ） > Checmate （ 2016/08/04 https://lwn.net/Articles/696344/ ） > LandLock （ 2016/08/25 https://lwn.net/Articles/698226/ ） > PTAGS （ 2016/09/29 https://lwn.net/Articles/702639/ ） > CaitSith （ 2016/10/21 https://lwn.net/Articles/704262/ ） > SafeName （ 2016/05/03 https://lwn.net/Articles/686021/ ） > WhiteEgret （ 2017/05/30 https://lwn.net/Articles/724192/ ） > shebang （ 2017/06/09 https://lwn.net/Articles/725285/ ） > S.A.R.A. （ 2017/06/13 https://lwn.net/Articles/725230/ ） > > are proposed 5 or 6 years ago, but mostly became silent... Yes. Unless a major distributor (Redhat, Canonical, ...) decides to include it the upstream potential of an LSM is very limited. > I still need byte-code analysis for finding the hook and code for making the > hook > writable in AKARI/CaitSith due to lack of > EXPORT_SYMBOL_GPL(security_add_hooks). > I wonder when I can stop questions like > https://osdn.net/projects/tomoyo/lists/archive/users-en/2022-September/000740.html > caused by > https://patchwork.kernel.org/project/linux-security-module/patch/alpine.lrh.2.20.1702131631490.8...@namei.org/ > . The BPF people made noises about revamping the way LSM hooks get called for performance reasons, but I have not seen a proposal from them. I have assumed that they will get around to it eventually. > Last 10 years, my involvement with Linux kernel is "fixing bugs" rather than > "developing security mechanisms". Changes what I found in the past 10 years > are: > > As far as I'm aware, more than 99% of systems still disable SELinux. People > use RHEL, > but the reason to choose RHEL is not because RHEL supports SELinux. The > only thing > changed is that the way to disable SELinux changed from SELINUX=disabled in > /etc/selinux/config to selinux=0 on kernel command line options. -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: LSM stacking in next for 6.1?
On 9/7/22 16:53, Casey Schaufler wrote: On 9/7/2022 4:27 PM, Paul Moore wrote: On Wed, Sep 7, 2022 at 12:42 PM Casey Schaufler wrote: On 9/7/2022 7:41 AM, Paul Moore wrote: On Tue, Sep 6, 2022 at 8:10 PM John Johansen wrote: On 9/6/22 16:24, Paul Moore wrote: On Fri, Sep 2, 2022 at 7:14 PM Casey Schaufler wrote: On 9/2/2022 2:30 PM, Paul Moore wrote: On Tue, Aug 2, 2022 at 8:56 PM Paul Moore wrote: On Tue, Aug 2, 2022 at 8:01 PM Casey Schaufler wrote: .. If you are running AppArmor on the host system and SELinux in a container you are likely going to have some *very* bizarre behavior as the SELinux policy you load in the container will apply to the entire system, including processes which started *before* the SELinux policy was loaded. While I understand the point you are trying to make, I don't believe the example you chose is going to work without a lot of other changes. correct but the reverse does work ... Sure, that doesn't surprise me, but that isn't the example Casey brought up. I said that I'm not sure how they go about doing Android on Ubuntu. I brought it up because I've seen it. Addressed in the other portion of this thread, but the quick response here is: No, you were mistaken, that was not proper Android, SELinux was disabled. I'm sympathetic that this work has been going on for some time, but that is no reason to push through a bad design; look at the ACKs/reviews on the combined-label patches vs the others in the series, that's a pretty good indication that no one is really excited about that design. The kernel developers aren't the consumers of these APIs. There has been considerable feedback from system application developers on the interfaces. This included dbus and systemd. Kernel developers aren't interested in these APIs because they don't care one way or the other. That, and as you are painfully aware, some of them are really busy on their own projects. Yes, everyone is busy yet they found time to ACK/review the other patches in the patchset. I'm not buying the "busy" argument here. Yes, you are also correct in that the kernel devs are not likely to be the main consumers of any kernel/userspace API, but we do have to support these APIs and find ways to handle the inevitable misuse and abuse of these APIs. Further, while I do believe that you've talked to some application developers about the current proposed API, I'm reasonably confident that it isn't the only API they would be happy supporting in their applications. As far as kernel developers not being interested in these APIs, I think the recent conversation in this thread proves the exact opposite ;) Am I really happy with the "hideous" format? Yeah, because it makes the end user happy. Am I happy with interface_lsm? Other than the name, which was the result of feedback, yes, because it in the simplest way to accomplish the goal. I am curious about what you think is "bad" about the current design. The interfaces aren't exciting. They don't need to be. I don't even know what an exciting interface would look like ... ? io_uring? :) I just want an interface that is clearly defined, has reasonable capacity to be extended in the future as needed, and is easy(ish) to use and support over extended periods of time (both from a kernel and userspace perspective). The "smack_label\0apparmor_label\0selinux_label\0future_lsm_label\0" string interface is none of these things. In this we disagree It is not clearly defined as it requires other interfaces to associate the labels with the correct LSMs. The label follows the lsm name directly. What other association is required? its a serialized message format, with all the data in the message instead of pointer to external memory. If you want nicer to handle you wrap a lib around it, this is pretty common. That is why I don't see it as that different from the syscall. It has no provision for extension beyond adding a new LSM. I grant that. But the purpose of the format is to present LSM/context pairs. What other information would make sense? We could add an "aux" field, but that seems somewhat arbitrary. or you know a header that gives potential future, also potentially a count, ... The ease-of-use quality is a bit subjective, but it does need another interface to use properly and it requires string parsing which history has shown to be an issue time and time again (although it is relatively simple here). There was a lot of discussion regarding that. My proposed apparmor="unconfined",smack="User" format was panned for those same reasons. The nil byte format has been used elsewhere and suggested for that reason. At this level the lib provides the ease of use, but I think that is what we are going to need with a syscall too, as marshalling variable number/length data is always somewhat ugly. Once again, the syscall example I tossed out was a quick strawman, but it had clearly separated and defined labels conveyed in data
