Re: iptables audit target causes kernel panic with iptables-persistent (kernel 3.2.78)
On Tue, Apr 26, 2016 at 3:58 PM, Lev Stipakov <lstipa...@gmail.com> wrote: > Yep, it works fine on Debian 8: > > lev@debi:~$ uname -a > Linux debi 3.16.0-4-amd64 #1 SMP Debian 3.16.7-ckt20-1+deb8u3 (2016-01-17) > x86_64 GNU/Linux I would suggest bringing this up with the Debian kernel packagers/maintainers, or doing a git-bisect of the Debian kernel if you are comfortable with that sort of thing. > On 26.04.2016 21:54, Paul Moore wrote: >>> >>> >>> I cannot reproduce it on (one of) previous kernel version: >>> >>>lev@debi7:~$ uname -a >>>Linux debi7 3.2.0-4-amd64 #1 SMP Debian 3.2.73-2+deb7u2 x86_64 >>> GNU/Linux >>> >>>lev@debi7:~$ dpkg -l | grep iptables >>>ii iptables 1.4.14-3.1 >>>ii iptables-persistent0.5.7+deb7u1 >> >> Unfortunately I don't have a Debian system available to test, but have >> you tried reproducing this on a more modern kernel? -- paul moore www.paul-moore.com -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] audit: normalize NETFILTER_PKT
On Tue, Jan 31, 2017 at 2:44 PM, Richard Guy Briggs <r...@redhat.com> wrote: > On 2017-01-31 17:13, Steve Grubb wrote: ... >> I was curious about something. Auparse is trying to interpret the >> icmptype field for every event. This is not good. Which fields are >> valid for each kind of packet? Are there fields valid for all packets? >> Is the icmptype/code the only ones that don't apply in all cases? > > Ok, this is important to know... You sound surprised. So if that field > isn't valid for all cases of that event, then the event should be split > or the "unset" value should be used as a hint to ignore it. > > This was the point of my earlier posting: > https://www.redhat.com/archives/linux-audit/2017-January/msg00074.html > There are still a number of questions from that thread that had no > reply. Answering those questions would help inform this discussion, so > if you could answer some of those questions in that first thread, I'd > have a better chance of understanding what are the limitations of the > parser and design/work around them. > > There is no packet for which all fields are valid. This is why using > "unset" values in those fields was suggested, seemed to be accepted in > discussion, and implemented. ... > Swinging fields in and out makes it very handy to use one message type > for all of them and can save precious disk bandwidth, but the point was > to normalize these messages. Is that still realistic and necessary? If > so, we're trying to find a balance between message type explosion and > disk bandwidth. > > We either need to make this fine-grained, ignore fields that aren't > valid for that type, or swing fields in and out. Or maybe I have missed > something fundamental, such as the presence of subsequent fields depends > on the values of previous fields? I'm still trying to understand what purpose this record actually serves, and what requirements may exist. In an earlier thread somewhere Steve mentioned some broad requirements around data import/export, and I really wonder if the NETFILTER_PKT record provides anything useful here when it really isn't connecting the traffic to the sender/receiver without a lot of additional logging and post-processing smarts. If you were interested in data import/export I think auditing the socket syscalls would provide a much more useful set of records in the audit log. Considering that one of the primary motivations for the audit subsystem is to enable compliance with various security specifications, let's get the ones we know about listed in this thread and then figure out how best to meet those requirements. -- paul moore www.paul-moore.com -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] audit: normalize NETFILTER_PKT
On Wed, Feb 8, 2017 at 7:32 AM, Richard Guy Briggs <r...@redhat.com> wrote: > On 2017-02-07 23:02, Paul Moore wrote: >> On Tue, Feb 7, 2017 at 4:22 PM, Richard Guy Briggs <r...@redhat.com> wrote: >> > On 2017-02-06 14:41, Paul Moore wrote: >> >> On Sat, Feb 4, 2017 at 8:25 AM, Steve Grubb <sgr...@redhat.com> wrote: >> >> > On Friday, February 3, 2017 6:44:16 PM EST Paul Moore wrote: >> >> >> I'm still trying to understand what purpose this record actually >> >> >> serves, and what requirements may exist. In an earlier thread >> >> >> somewhere Steve mentioned some broad requirements around data >> >> >> import/export, and I really wonder if the NETFILTER_PKT record >> >> >> provides anything useful here when it really isn't connecting the >> >> >> traffic to the sender/receiver without a lot of additional logging and >> >> >> post-processing smarts. If you were interested in data import/export >> >> >> I think auditing the socket syscalls would provide a much more useful >> >> >> set of records in the audit log. >> >> > >> >> > The problem here is we cannot be selective enough through the syscall >> >> > interface to get exactly what we want. For example, any auditing of >> >> > connect >> >> > and accept will also get af_unix traffic which is likely to be uid/gid >> >> > lookups >> >> > through sssd or glibc. Typically we want the IPv4/6 traffic. The >> >> > netfilter rules >> >> > are better suited to describing which packets are of interest. >> >> >> >> Okay, but how useful are these NETFILTER_PKT records, really? The >> >> only linkage you have back to the process on the local machine is via >> >> the addr/proto/port tuple and that seems far from ideal. >> > >> > And even that could be spoofed easily and gathering more corroborating >> > information would seem useful. >> > >> > Would the presence of the SOCKADDR record in any SYSCALL record be >> > useful for somehow tagging a class of fd as being of interest? >> >> I don't think we want to create a SOCKADDR record for every syscall, >> but it seems reasonable that we may want to include it for targeted >> syscalls. Right now it looks like we create a SOCKADDR record >> whenever we copy a sockaddr struct across the kernel/userspace >> boundary, that should be sufficient, yes? > > Yes, we certainly don't need it for every syscall. Since the sockaddr > record is only created if it is available we could further flag or check > the protocol to further process only the network-based sockaddrs and > ignore the unix sockaddrs for this purpose. I'm picturing adding a flag > to the fd, but that is making me a bit nervous about overstepping our > usual code area. Let's keep it as-is, I would think there are other cases where having the address info for AF_UNIX (and others) might be helpful. -- paul moore www.paul-moore.com -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: AUDIT_NETFILTER_PKT message format
On Wed, Feb 8, 2017 at 11:30 AM, Steve Grubb <sgr...@redhat.com> wrote: > On Tuesday, February 7, 2017 10:56:39 PM EST Paul Moore wrote: >> On Tue, Feb 7, 2017 at 3:52 PM, Richard Guy Briggs <r...@redhat.com> wrote: >> > So while I'm not advocating this is what should be done and I'm trying >> > to establish bounds to the scope of this feature, but would it be >> > reasonable to simply not log packets that were transiting this machine >> > without a local endpoint? >> >> I'm still waiting on more detailed requirements information from >> Steve, but based on what we've heard so far, it seems that ignoring >> forwarded traffic is a reasonable thing to do. > > OK, I have done teh analysis to see where things stand on this ... ... > At this point, I would say there is no purpose for xt_AUDIT.c based on Common > Criteria. It looks like its built in response to the > CONFIG_NETFILTER_XT_TARGET_AUDIT config option. So, it can be cleanly > deprecated. Based on some off-list discussions with Richard it would appear that there are several users of the NETFILTER_PKT record so I am in no hurry to deprecate it. Considering that there are no CC requirements on the record, I think we can focus on simply providing a basic record that satisfies the whims of the userspace tools without adding any pain to the kernel. I believe Richard is currently working on a proposal to do that, let's discuss it further in that thread. -- paul moore www.paul-moore.com -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] audit: normalize NETFILTER_PKT
On Sat, Feb 4, 2017 at 8:25 AM, Steve Grubb <sgr...@redhat.com> wrote: > On Friday, February 3, 2017 6:44:16 PM EST Paul Moore wrote: >> I'm still trying to understand what purpose this record actually >> serves, and what requirements may exist. In an earlier thread >> somewhere Steve mentioned some broad requirements around data >> import/export, and I really wonder if the NETFILTER_PKT record >> provides anything useful here when it really isn't connecting the >> traffic to the sender/receiver without a lot of additional logging and >> post-processing smarts. If you were interested in data import/export >> I think auditing the socket syscalls would provide a much more useful >> set of records in the audit log. > > The problem here is we cannot be selective enough through the syscall > interface to get exactly what we want. For example, any auditing of connect > and accept will also get af_unix traffic which is likely to be uid/gid lookups > through sssd or glibc. Typically we want the IPv4/6 traffic. The netfilter > rules > are better suited to describing which packets are of interest. Okay, but how useful are these NETFILTER_PKT records, really? The only linkage you have back to the process on the local machine is via the addr/proto/port tuple and that seems far from ideal. >> Considering that one of the primary motivations for the audit >> subsystem is to enable compliance with various security >> specifications, let's get the ones we know about listed in this thread >> and then figure out how best to meet those requirements. > > Common Criteria calls out for the ability to detect any attempt at information > flow. Everything else leverages the CC requirements. Yes, you've mentioned this previously. This is good, but we need to make these requirements a bit more concrete; we need something we can use to arrive at a working implementation that satisfies these requirements. If this is purely about information flowing from A to B, would the source and destination addr/proto/port for TCP and UDP suffice? Do we need anything else? -- paul moore www.paul-moore.com -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: AUDIT_NETFILTER_PKT message format
On Tue, Feb 7, 2017 at 3:52 PM, Richard Guy Briggs <r...@redhat.com> wrote: > So while I'm not advocating this is what should be done and I'm trying > to establish bounds to the scope of this feature, but would it be > reasonable to simply not log packets that were transiting this machine > without a local endpoint? I'm still waiting on more detailed requirements information from Steve, but based on what we've heard so far, it seems that ignoring forwarded traffic is a reasonable thing to do. -- paul moore security @ redhat -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: AUDIT_NETFILTER_PKT message format
On Sat, Jan 21, 2017 at 6:27 AM, Patrick PIGNOL <patrick.pig...@gmail.com> wrote: > Hi all, > > I disagree ! > > Many people in the world would like to allow an software A to go to internet > through OUTPUT TCP port 80 but disallow software B to go to the internet > through this same OUTPUT TCP port 80. Don't you know about viruses on linux > ? Viruses ALWAYS use HTTP/HTTPS ports to get payloads on internet and OUTPUT > TCP port 443 COULD NOT be CLOSED for ALL SOFTWARE if you want to access > internet services (via internet browsers for example). The Linux audit subsystem simply logs system events, it does not enforce security policy. I suggest you investigate the different Linux firewall tools and LSMs, e.g. SELinux, as they should help you accomplish what you describe. -- paul moore www.paul-moore.com -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: AUDIT_NETFILTER_PKT message format
On Fri, Jan 20, 2017 at 9:49 AM, Steve Grubb <sgr...@redhat.com> wrote: > On Wednesday, January 18, 2017 6:35:29 PM EST Paul Moore wrote: >> At this point I think it would be good to hear what requirements exist >> for per-packet auditing. Steve, are there any current Common Criteria >> (or other) requirements that impact per-packet auditing? > > I don't think you want to flood your logs. That is not helpful. It asks for > the > ability to detect information flow. Typically you want to know source and > destination, protocol, where on the system its coming from or going to, pid if > possible and the subject information if available. I know that you can be > acting as a proxy and forwarding outside packets into a network. That is not > the typical case CC is concerned about. Its more about what the user is doing. Determining the pid/subj of a packet is notoriously difficult/impossible in netfilter so let's drop that; with proper policy/rules you should be able to match proto/port with a given process so this shouldn't be that critical. The source/destination addresses and proto/port (assuming IP) should be easy enough. All right, now that we've got the "must" items down, are their any "should" items we want? -- paul moore www.paul-moore.com -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: AUDIT_NETFILTER_PKT message format
[NOTE: I'll respond back to the other part of your email later but I'm running out of time in the day and this was a quick but important response] On Thu, Feb 16, 2017 at 5:36 PM, Richard Guy Briggs <r...@redhat.com> wrote: > Steve has requested the subject attributes which prefixes 7 fields. I already commented on this earlier in this thread - or some other related thread, I've lost track, but both you and Steve were on the To/CC line - last time I checked, you can't reliably link packets to the sender/subject in the netfilter hooks (I'll be shocked if this has changed). The best you can do in some cases is to link the packet to the socket, and that isn't going to help you. -- paul moore www.paul-moore.com -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: AUDIT_NETFILTER_PKT message format
On Thu, Feb 16, 2017 at 5:36 PM, Richard Guy Briggs <r...@redhat.com> wrote: > On 2017-02-15 19:32, Paul Moore wrote: >> On Mon, Feb 13, 2017 at 7:24 PM, Richard Guy Briggs <r...@redhat.com> wrote: >> > On 2017-02-13 18:50, Paul Moore wrote: >> >> On Mon, Feb 13, 2017 at 3:50 PM, Richard Guy Briggs <r...@redhat.com> >> >> wrote: >> >> ... >> >> >> > helpful action, hook >> >> >> >> I haven't checked, but do we allow setting of an audit key in >> >> NETFILTER_PKT records? It seems like that might be a good thing for >> >> the userspace tools and would likely make logging the action/hook >> >> unncessary. >> > >> > Not that I am aware of. That would be way useful if it were possible. >> > "AUDIT" is a netfilter target and you can set the type to "accept", >> > "drop" or "reject". Similarly, having the sub-chain name would be >> > useful but that doesn't appear to be available either. This is why I >> > used a "mark" in the testsuite to track packets. >> >> I've been thinking about this off and on and I think you are on to >> something here ... the netfilter mark is very similar to what we do >> with the audit keys and the audit-folk on this thread already know how >> helpful audit keys can be for associating records with a specific [set >> of] audit rules; I'm thinking we should treat the netfilter mark the >> same way, after all, this is very much in keeping with how >> netfilter/iptables uses the mark data. > > I felt like I was kind of cheating to use it, but no other fine-grained > method was evident to me when I wrote that test script. In a test > script it is a controlled environment with no other conflicting users. > > My thoughts were that use of it as a key for tracking audit events > itself might not be as viable due to other uses of the nfmark. > > What it comes down to is simply spending a bit more careful design > effort to have the uses of nfmark co-exist since I don't see any > inherent conflicts. I think it is safe to say that anyone who is going to the trouble of using NETFILTER_PKT is going to have a well considered security configuration. I don't view using the nfmark as a shorthand for audit to help identify complex netfilter matches to be a major ask in these situations, and it seems in keeping with the intent of the nfmark concept. >> In an effort to simplify >> things greatly for the NETFILTER_PKT record I'm going to offer the >> following suggestion: >> >> * Limit NETFILTER_PKT fields to only those present in the IPv4/IPv6 >> header, e.g. src/dest addresses and next level protocol, and the >> netfilter mark. > > (I'd start with: mark, saddr, daddr, proto) Yeah, that's what I said ;) > That seems a bit oversimplified, requiring a lot more effort and lists > of rules to track down different application-layer protocols (ports). I relies more heavily on the nfmark as discussed above. > This reminds me of Rusty's sig a while back "Premature optmztion is rt > of all evl." ;-) Perhaps this is being nitpicky, but you optimize something for a given set of requirements, something which we are lacking here. This isn't an optimization, but rather a simplified approach brought about by a lack of requirements. Right now the only real requirement we have is that Steve would like something a bit more predictable in the NETFILTER_PKT record . While we have indications that people are using this in the wild, they aren't letting their voices be heard here, or anywhere else that we can see, so I'd like to focus on a solution that satisfies Steve and doesn't burden us in the future in case we have to start adding additional fields/records. This is why I'm thinking of limiting us to just the IP layer information + the nfmark; this should provide a fairly rich capability without saddling us with a lot of baggage to worry about in the future. I do agree that it does add some administrative setup cost, but as I said earlier, those admins who make use of this are likely already used to dealing with a high setup cost. I *really* don't want to add a bunch of new records and fields for a bit of functionality with uncertain usage and requirements; that almost never works out well for anyone, especially those who have to maintain that crap for the long term. > There are a limited number of actions, hooks, interfaces and protocol > families, so this seems plausibly reasonable to ditch in favour of > nfmark, but all of these would just need to be re-coded in the nfmark if > needed, although the typical assumption about number of interfaces may > be naive for those users w
Re: [PATCH V2] audit: normalize NETFILTER_PKT
On Thu, Feb 23, 2017 at 8:59 PM, Florian Westphal <f...@strlen.de> wrote: > Richard Guy Briggs <r...@redhat.com> wrote: >> > Not following, sorry, are you saying users can/should use -j MARK >> > somehow? >> >> Part of the discussed design and rationale for stripping many of the >> vanishing fields is that when setting up netfilter rules to invoke the >> AUDIT target, an accompanying nf mark should be used to indicate which >> rule caught that packet, since the chain name and rule number aren't >> available to the audit target. We would use the nf mark similarly to >> the way we use a rule key in the audit rules (see man auditctl). > > I see. While this works, nfmark might already be used for other > purposes such as policy routing, so you might need an extra cookie > that can be passed to the AUDIT target instead. Yes, we discussed the idea that the nfmark field already serves many purposes, most of which are related to labeling traffic flows. I agree that using the nfmark may complicate some configurations, but using it in this manner seems to be in keeping with the ideas behind nfmark (from what I can tell). As for the configuration complexity, I think it is safe to say that any users of the NETFILTER_PKT record already have a sufficiently complex system configuration and the added complexity here may not be significant; in fact, the existing nfmark configuration may be helpful in identifying traffic categories such that no changes need to be made. -- paul moore www.paul-moore.com -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2] audit: normalize NETFILTER_PKT
On Thu, Feb 23, 2017 at 12:35 PM, Richard Guy Briggs <r...@redhat.com> wrote:
> On 2017-02-23 12:14, Paul Moore wrote:
>> On Thu, Feb 23, 2017 at 12:13 PM, Richard Guy Briggs <r...@redhat.com> wrote:
>> > On 2017-02-23 12:06, Paul Moore wrote:
>> >> On Thu, Feb 23, 2017 at 12:04 PM, Richard Guy Briggs <r...@redhat.com>
>> >> wrote:
>> >> > On 2017-02-23 11:57, Paul Moore wrote:
>> >> >> On Thu, Feb 23, 2017 at 10:51 AM, Richard Guy Briggs <r...@redhat.com>
>> >> >> wrote:
>> >> >> > On 2017-02-23 06:20, Florian Westphal wrote:
>> >> >> >> Richard Guy Briggs <r...@redhat.com> wrote:
>> >> >> >> > Simplify and eliminate flipping in and out of message fields,
>> >> >> >> > relying on nfmark
>> >> >> >> > the way we do for audit_key.
>> >> >> >> >
>> >> >> >> > +struct nfpkt_par {
>> >> >> >> > + int ipv;
>> >> >> >> > + const void *saddr;
>> >> >> >> > + const void *daddr;
>> >> >> >> > + u8 proto;
>> >> >> >> > +};
>> >> >> >>
>> >> >> >> This is problematic, see below for why.
>> >> >> >>
>> >> >> >> > -static void audit_ip4(struct audit_buffer *ab, struct sk_buff
>> >> >> >> > *skb)
>> >> >> >> > +static void audit_ip4(struct audit_buffer *ab, struct sk_buff
>> >> >> >> > *skb, struct nfpkt_par *apar)
>> >> >> >> > {
>> >> >> >> > struct iphdr _iph;
>> >> >> >> > const struct iphdr *ih;
>> >> >> >> >
>> >> >> >> > + apar->ipv = 4;
>> >> >> >> > ih = skb_header_pointer(skb, 0, sizeof(_iph), &_iph);
>> >> >> >> > - if (!ih) {
>> >> >> >> > - audit_log_format(ab, " truncated=1");
>> >> >> >> > + if (!ih)
>> >> >> >> > return;
>> >> >> >>
>> >> >> >> Removing this "truncated" has the consequence that this can later
>> >> >> >> log
>> >> >> >> "saddr=0.0.0.0 daddr=0.0.0.0" if we return here.
>> >> >> >>
>> >> >> >> This cannot happen for ip(6)tables because ip stack discards broken
>> >> >> >> l3 headers
>> >> >> >> before the netfilter hooks get called, but its possible with
>> >> >> >> NFPROTO_BRIDGE.
>> >> >> >>
>> >> >> >> Perhaps you will need to change audit_ip4/6 to return "false" when
>> >> >> >> it can't
>> >> >> >> get the l3 information now so we only log zero addresses when the
>> >> >> >> packet
>> >> >> >> really did contain them.
>> >> >> >
>> >> >> > Ok, to clarify the implications, are you saying that handing a NULL
>> >> >> > pointer to "saddr=%pI4" will print "0.0.0.0" rather than "(none)" or
>> >> >> > "?"
>> >> >>
>> >> >> My initial reaction is that if the packet is so badly
>> >> >> truncated/malformed that we don't have a full IP header than we should
>> >> >> just refrain from logging the packet; it's too malformed/garbage to
>> >> >> offer any useful information and the normal packet processing should
>> >> >> result in the packet being discarded anyway.
>> >> >
>> >> > Which is why I wanted the ethertype, but that can be coded into the
>> >> > nfmark.
>> >>
>> >> If the packet is garbage (garbage without any payload in this case),
>> >> what does it matter? It's noise.
>> >
>> > It could be an indicator that either the logging rules or the filter
>> > rules need honing, or even that there is a bug in the network code.
>>
>> Elaborate on this please, I still don't see how logging the ethertype
>> is helpful for a malformed packet.
>
> Well, since we can encode it in the nfmark, it could be helpful, but not
> necessary.
>
> Each bit of information we can include in the audit log message removes
> something we need to code in the nf mark. That's why things like ifin,
> ifout, action, hook are easy to include and help reduce the amount of
> nf mark coding needed when devising netfilter rules.
... but if the packet is so badly manged it doesn't even have a
complete IP header, what's the point in logging the packet at all?
That's the point I'm trying to make.
> I had another idea on how to include the sport and dport and that was to
> use the same identifier for sport/icmptype and also for dport/icmpcode,
> but you've already said you are not interested.
Not at this point in time since we don't have any good requirements at
the moment. I would like us to keep this small until we have a better
idea of how people want to use this, this way we don't end up stuck
maintaining something that is ill suited for what people actually
want/use.
--
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2] audit: normalize NETFILTER_PKT
On Thu, Feb 23, 2017 at 10:51 AM, Richard Guy Briggs <r...@redhat.com> wrote:
> On 2017-02-23 06:20, Florian Westphal wrote:
>> Richard Guy Briggs <r...@redhat.com> wrote:
>> > Simplify and eliminate flipping in and out of message fields, relying on
>> > nfmark
>> > the way we do for audit_key.
>> >
>> > +struct nfpkt_par {
>> > + int ipv;
>> > + const void *saddr;
>> > + const void *daddr;
>> > + u8 proto;
>> > +};
>>
>> This is problematic, see below for why.
>>
>> > -static void audit_ip4(struct audit_buffer *ab, struct sk_buff *skb)
>> > +static void audit_ip4(struct audit_buffer *ab, struct sk_buff *skb,
>> > struct nfpkt_par *apar)
>> > {
>> > struct iphdr _iph;
>> > const struct iphdr *ih;
>> >
>> > + apar->ipv = 4;
>> > ih = skb_header_pointer(skb, 0, sizeof(_iph), &_iph);
>> > - if (!ih) {
>> > - audit_log_format(ab, " truncated=1");
>> > + if (!ih)
>> > return;
>>
>> Removing this "truncated" has the consequence that this can later log
>> "saddr=0.0.0.0 daddr=0.0.0.0" if we return here.
>>
>> This cannot happen for ip(6)tables because ip stack discards broken l3
>> headers
>> before the netfilter hooks get called, but its possible with NFPROTO_BRIDGE.
>>
>> Perhaps you will need to change audit_ip4/6 to return "false" when it can't
>> get the l3 information now so we only log zero addresses when the packet
>> really did contain them.
>
> Ok, to clarify the implications, are you saying that handing a NULL
> pointer to "saddr=%pI4" will print "0.0.0.0" rather than "(none)" or "?"
My initial reaction is that if the packet is so badly
truncated/malformed that we don't have a full IP header than we should
just refrain from logging the packet; it's too malformed/garbage to
offer any useful information and the normal packet processing should
result in the packet being discarded anyway.
--
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2] audit: normalize NETFILTER_PKT
On Thu, Feb 23, 2017 at 12:13 PM, Richard Guy Briggs <r...@redhat.com> wrote:
> On 2017-02-23 12:06, Paul Moore wrote:
>> On Thu, Feb 23, 2017 at 12:04 PM, Richard Guy Briggs <r...@redhat.com> wrote:
>> > On 2017-02-23 11:57, Paul Moore wrote:
>> >> On Thu, Feb 23, 2017 at 10:51 AM, Richard Guy Briggs <r...@redhat.com>
>> >> wrote:
>> >> > On 2017-02-23 06:20, Florian Westphal wrote:
>> >> >> Richard Guy Briggs <r...@redhat.com> wrote:
>> >> >> > Simplify and eliminate flipping in and out of message fields,
>> >> >> > relying on nfmark
>> >> >> > the way we do for audit_key.
>> >> >> >
>> >> >> > +struct nfpkt_par {
>> >> >> > + int ipv;
>> >> >> > + const void *saddr;
>> >> >> > + const void *daddr;
>> >> >> > + u8 proto;
>> >> >> > +};
>> >> >>
>> >> >> This is problematic, see below for why.
>> >> >>
>> >> >> > -static void audit_ip4(struct audit_buffer *ab, struct sk_buff *skb)
>> >> >> > +static void audit_ip4(struct audit_buffer *ab, struct sk_buff *skb,
>> >> >> > struct nfpkt_par *apar)
>> >> >> > {
>> >> >> > struct iphdr _iph;
>> >> >> > const struct iphdr *ih;
>> >> >> >
>> >> >> > + apar->ipv = 4;
>> >> >> > ih = skb_header_pointer(skb, 0, sizeof(_iph), &_iph);
>> >> >> > - if (!ih) {
>> >> >> > - audit_log_format(ab, " truncated=1");
>> >> >> > + if (!ih)
>> >> >> > return;
>> >> >>
>> >> >> Removing this "truncated" has the consequence that this can later log
>> >> >> "saddr=0.0.0.0 daddr=0.0.0.0" if we return here.
>> >> >>
>> >> >> This cannot happen for ip(6)tables because ip stack discards broken l3
>> >> >> headers
>> >> >> before the netfilter hooks get called, but its possible with
>> >> >> NFPROTO_BRIDGE.
>> >> >>
>> >> >> Perhaps you will need to change audit_ip4/6 to return "false" when it
>> >> >> can't
>> >> >> get the l3 information now so we only log zero addresses when the
>> >> >> packet
>> >> >> really did contain them.
>> >> >
>> >> > Ok, to clarify the implications, are you saying that handing a NULL
>> >> > pointer to "saddr=%pI4" will print "0.0.0.0" rather than "(none)" or "?"
>> >>
>> >> My initial reaction is that if the packet is so badly
>> >> truncated/malformed that we don't have a full IP header than we should
>> >> just refrain from logging the packet; it's too malformed/garbage to
>> >> offer any useful information and the normal packet processing should
>> >> result in the packet being discarded anyway.
>> >
>> > Which is why I wanted the ethertype, but that can be coded into the nfmark.
>>
>> If the packet is garbage (garbage without any payload in this case),
>> what does it matter? It's noise.
>
> It could be an indicator that either the logging rules or the filter
> rules need honing, or even that there is a bug in the network code.
Elaborate on this please, I still don't see how logging the ethertype
is helpful for a malformed packet.
--
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2] audit: normalize NETFILTER_PKT
On Thu, Feb 23, 2017 at 12:04 PM, Richard Guy Briggs <r...@redhat.com> wrote:
> On 2017-02-23 11:57, Paul Moore wrote:
>> On Thu, Feb 23, 2017 at 10:51 AM, Richard Guy Briggs <r...@redhat.com> wrote:
>> > On 2017-02-23 06:20, Florian Westphal wrote:
>> >> Richard Guy Briggs <r...@redhat.com> wrote:
>> >> > Simplify and eliminate flipping in and out of message fields, relying
>> >> > on nfmark
>> >> > the way we do for audit_key.
>> >> >
>> >> > +struct nfpkt_par {
>> >> > + int ipv;
>> >> > + const void *saddr;
>> >> > + const void *daddr;
>> >> > + u8 proto;
>> >> > +};
>> >>
>> >> This is problematic, see below for why.
>> >>
>> >> > -static void audit_ip4(struct audit_buffer *ab, struct sk_buff *skb)
>> >> > +static void audit_ip4(struct audit_buffer *ab, struct sk_buff *skb,
>> >> > struct nfpkt_par *apar)
>> >> > {
>> >> > struct iphdr _iph;
>> >> > const struct iphdr *ih;
>> >> >
>> >> > + apar->ipv = 4;
>> >> > ih = skb_header_pointer(skb, 0, sizeof(_iph), &_iph);
>> >> > - if (!ih) {
>> >> > - audit_log_format(ab, " truncated=1");
>> >> > + if (!ih)
>> >> > return;
>> >>
>> >> Removing this "truncated" has the consequence that this can later log
>> >> "saddr=0.0.0.0 daddr=0.0.0.0" if we return here.
>> >>
>> >> This cannot happen for ip(6)tables because ip stack discards broken l3
>> >> headers
>> >> before the netfilter hooks get called, but its possible with
>> >> NFPROTO_BRIDGE.
>> >>
>> >> Perhaps you will need to change audit_ip4/6 to return "false" when it
>> >> can't
>> >> get the l3 information now so we only log zero addresses when the packet
>> >> really did contain them.
>> >
>> > Ok, to clarify the implications, are you saying that handing a NULL
>> > pointer to "saddr=%pI4" will print "0.0.0.0" rather than "(none)" or "?"
>>
>> My initial reaction is that if the packet is so badly
>> truncated/malformed that we don't have a full IP header than we should
>> just refrain from logging the packet; it's too malformed/garbage to
>> offer any useful information and the normal packet processing should
>> result in the packet being discarded anyway.
>
> Which is why I wanted the ethertype, but that can be coded into the nfmark.
If the packet is garbage (garbage without any payload in this case),
what does it matter? It's noise.
--
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: AUDIT_NETFILTER_PKT message format
On Mon, Feb 13, 2017 at 3:50 PM, Richard Guy Briggs <r...@redhat.com> wrote: > On 2017-02-13 12:57, Steve Grubb wrote: >> On Friday, February 10, 2017 5:54:45 PM EST Richard Guy Briggs wrote: >> > On 2017-02-10 17:39, Steve Grubb wrote: >> > > > The alternatives that I currently see are to drop packets for which >> > > > there is no local process ownership, or to leave the ownership fields >> > > > unset.> >> > >> > > What ownership fields are we talking about? >> > >> > The ones you want, auid, pid, ses. Perhaps I'm using the wrong >> > terminology. What technical term is there for the collection of subject >> > identifiers? >> >> Subject attributes. > > Ah ok, I'll try to remember to use that term... > > Now that you know what I'm talking about, can you go back and answer the > questions I had about packet "ownership" (which is really packet subject > attributes)? If we have that information, how to we include it in the > message format? And if we don't have it, do we ignore the packet, or do > we swing fields out, or do we set those fields to "unset" or do we use > an auxiliary record? Packet "ownership" is likely going to be impossible to determine reliably since in some cases you can't even match a packet to a socket, let alone a process. To back up a few messages in this thread, to Richard's list of things to potentially log: > helpful action, hook I haven't checked, but do we allow setting of an audit key in NETFILTER_PKT records? It seems like that might be a good thing for the userspace tools and would likely make logging the action/hook unncessary. > useless?len I don't see much point in this. > helpful inif, outif, mark Let's split this into two things: the interfaces and the mark. I don't see much value in logging the mark, but I could see some value in logging the interface. > useless?smac, dmac, macproto Probably useless in the majority of use cases. > helpful protocol family I think we need some clarity on protocol logging; we've got "macproto" (I assume this is the ethertype, or similar), "protocol family" (I assume this to be a duplicate of ethertype, e.g. AF_INET), and "proto" (see below, I assume this to be TCP/UDP/etc.). > useless?truncated Definitely useless. Only keep this if we need it for some backwards compatibility. > helpful saddr, daddr Helpful. > useless?ipid Useless. > helpful proto > helpful sport, dport Assuming "proto" means the TCP/UDP/etc. then we should treat the proto/ports as one block; you can't log the ports without logging "proto". > useless?frag > useless?truncated Yes, useless. > helpful icmptype, icmpcode Similar to proto/port above. > helpful secmark (I forgot to change it from "obj" to "secmark" in my > patch). We may also want to log the peer label if we are going to log the secmark. -- paul moore www.paul-moore.com -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: AUDIT_NETFILTER_PKT message format
On Thu, Feb 9, 2017 at 5:56 AM, Pablo Neira Ayuso <pa...@netfilter.org> wrote: > Hi Paul, > > On Wed, Feb 08, 2017 at 06:09:07PM -0500, Paul Moore wrote: >> On Wed, Feb 8, 2017 at 11:30 AM, Steve Grubb <sgr...@redhat.com> wrote: >> > On Tuesday, February 7, 2017 10:56:39 PM EST Paul Moore wrote: >> >> On Tue, Feb 7, 2017 at 3:52 PM, Richard Guy Briggs <r...@redhat.com> >> >> wrote: >> >> > So while I'm not advocating this is what should be done and I'm trying >> >> > to establish bounds to the scope of this feature, but would it be >> >> > reasonable to simply not log packets that were transiting this machine >> >> > without a local endpoint? >> >> >> >> I'm still waiting on more detailed requirements information from >> >> Steve, but based on what we've heard so far, it seems that ignoring >> >> forwarded traffic is a reasonable thing to do. >> > >> > OK, I have done teh analysis to see where things stand on this ... >> >> ... >> >> > At this point, I would say there is no purpose for xt_AUDIT.c based on >> > Common >> > Criteria. It looks like its built in response to the >> > CONFIG_NETFILTER_XT_TARGET_AUDIT config option. So, it can be cleanly >> > deprecated. >> >> Based on some off-list discussions with Richard it would appear that >> there are several users of the NETFILTER_PKT record so I am in no >> hurry to deprecate it. Considering that there are no CC requirements >> on the record, I think we can focus on simply providing a basic record >> that satisfies the whims of the userspace tools without adding any >> pain to the kernel. I believe Richard is currently working on a >> proposal to do that, let's discuss it further in that thread. > > If the concern is to keep the existing output format around, you can > add new functions with the specific new layout at the cost of keeping > more code around. That should be fine since this code is not much > complex IMO. You can probably add a new explicit command line option, > eg. --version, that indicates what audit format version you want to > use, so users don't break. There are several things to consider, and I'm not going to worry too much about it until Richard posts his updated RFC. In quick summary, Steve is worried about record formats which don't meet a specific format specification (e.g. fields that optionally appear depending on the protocol are bad from his perspective) and I'm worried about adding a bunch of additional code to the kernel. We can discuss this further once we see Richard's patches, he is aware of all the concerns and I expect he will have something interesting to use as a starting point for further discussion. > BTW, any plans to add audit support to nf_tables? It would be nice, but it isn't on my TODO list at the moment; if you want to work on it I think that would be great! -- paul moore www.paul-moore.com -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: AUDIT_NETFILTER_PKT message format
On Mon, Feb 13, 2017 at 7:24 PM, Richard Guy Briggs <r...@redhat.com> wrote: > On 2017-02-13 18:50, Paul Moore wrote: >> On Mon, Feb 13, 2017 at 3:50 PM, Richard Guy Briggs <r...@redhat.com> wrote: ... >> > helpful action, hook >> >> I haven't checked, but do we allow setting of an audit key in >> NETFILTER_PKT records? It seems like that might be a good thing for >> the userspace tools and would likely make logging the action/hook >> unncessary. > > Not that I am aware of. That would be way useful if it were possible. > "AUDIT" is a netfilter target and you can set the type to "accept", > "drop" or "reject". Similarly, having the sub-chain name would be > useful but that doesn't appear to be available either. This is why I > used a "mark" in the testsuite to track packets. I've been thinking about this off and on and I think you are on to something here ... the netfilter mark is very similar to what we do with the audit keys and the audit-folk on this thread already know how helpful audit keys can be for associating records with a specific [set of] audit rules; I'm thinking we should treat the netfilter mark the same way, after all, this is very much in keeping with how netfilter/iptables uses the mark data. In an effort to simplify things greatly for the NETFILTER_PKT record I'm going to offer the following suggestion: * Limit NETFILTER_PKT fields to only those present in the IPv4/IPv6 header, e.g. src/dest addresses and next level protocol, and the netfilter mark. * Teach ausearch and the other relevant audit userspace tools to search on the netfilter mark much like they currently search on the audit key. This puts a reasonable bound on the fields in the NETFILTER_PKT record and insulates us from protocol specifics (both very desirable things); I also think we should be able to do this without having to introduce a new record, e.g. NETFILTER_PKT2 (another big win). Any additional packet information can be conveyed by the netfilter mark and careful netfilter rule construction. What do you think Richard? -- paul moore www.paul-moore.com -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: AUDIT_NETFILTER_PKT message format
On Wed, Jan 18, 2017 at 12:39 AM, Richard Guy Briggs <r...@redhat.com> wrote: > On 2017-01-17 21:34, Richard Guy Briggs wrote: >> On 2017-01-17 15:17, Paul Moore wrote: >> > On Tue, Jan 17, 2017 at 11:12 AM, Richard Guy Briggs <r...@redhat.com> >> > wrote: >> > > On 2017-01-17 08:55, Steve Grubb wrote: >> > >> On Tuesday, January 17, 2017 12:25:51 AM EST Richard Guy Briggs wrote: >> > >> > ... >> > >> > >> > Ones that are not so straightforward: >> > >> > - "secmark" depends on a kernel config setting, so should it always be >> > >> > present but "(none)" if that kernel feature is compiled out? >> > >> >> > >> If this is selinux related, I'd treat it the same way that we do subj >> > >> everywhere else. >> > > >> > > Ok. >> > >> > To be clear, a packet's secmark should be recorded via a dedicated >> > field, e.g. "secmark", and not use the "subj" field (it isn't a >> > subject label in the traditional sense). >> >> I think Steve was talking about if, when or where to include that field, >> not what its label is. > > In this case it is an "obj=" field, but since it is part of the LSM, > each one has its own fields. As I said above, use a "secmark" field and not the subject or object fields; packet labeling is rather complex and there is value in differentiating between secmark labels and network peer labels. -- paul moore security @ redhat -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: AUDIT_NETFILTER_PKT message format
On Wed, Jan 18, 2017 at 10:15 AM, Richard Guy Briggs <r...@redhat.com> wrote: > On 2017-01-18 07:32, Paul Moore wrote: >> On Wed, Jan 18, 2017 at 12:39 AM, Richard Guy Briggs <r...@redhat.com> wrote: >> > On 2017-01-17 21:34, Richard Guy Briggs wrote: >> >> On 2017-01-17 15:17, Paul Moore wrote: >> >> > On Tue, Jan 17, 2017 at 11:12 AM, Richard Guy Briggs <r...@redhat.com> >> >> > wrote: >> >> > > On 2017-01-17 08:55, Steve Grubb wrote: >> >> > >> On Tuesday, January 17, 2017 12:25:51 AM EST Richard Guy Briggs >> >> > >> wrote: >> >> > >> >> > ... >> >> > >> >> > >> > Ones that are not so straightforward: >> >> > >> > - "secmark" depends on a kernel config setting, so should it >> >> > >> > always be >> >> > >> > present but "(none)" if that kernel feature is compiled out? >> >> > >> >> >> > >> If this is selinux related, I'd treat it the same way that we do subj >> >> > >> everywhere else. >> >> > > >> >> > > Ok. >> >> > >> >> > To be clear, a packet's secmark should be recorded via a dedicated >> >> > field, e.g. "secmark", and not use the "subj" field (it isn't a >> >> > subject label in the traditional sense). >> >> >> >> I think Steve was talking about if, when or where to include that field, >> >> not what its label is. >> > >> > In this case it is an "obj=" field, but since it is part of the LSM, >> > each one has its own fields. >> >> As I said above, use a "secmark" field and not the subject or object >> fields; packet labeling is rather complex and there is value in >> differentiating between secmark labels and network peer labels. > > Ok, I'll change it from the existing "obj=" to "secmark=". Since it is > an LSM-dependent field, it will go away when that LSM module does. It > is the very last item in the list of fields, so I don't see this as a > problem. > > > I have more questions and observations: > > Do we care if the rest of the record's fields are there if the packet is > truncated? In other words, can I omit all the following fields (that > will end up being set to (none) or -1 since there is no data for them)? > I'd prefer to complete the record, but Steve may not care and might > prefer to save the bandwidth. > > Can I truncate the field name "truncated" to "trunc" (since I don't see > it yet in the audit field dictionary) if we do include all the fields? > > I observe that support for IPPROTO_DCCP and IPPROTO_SCTP can be added > virtually for free since the source and desination ports in their packet > formats is identical to TCP and UDP (and UDPLITE). > > > At this point, it looks like having one record for IP/IPv6 with > TCP/UDP/DCCP/SCTP makes sense. Whether or not to add ethernet bridge > headers and ICMP* is a more difficult question. Ethernet bridge adds 40 chars > if it isn't used, up to 62 if it is. ICMP* adds 26 max. > > It is an independent record, but it would be nice to be able to reuse > the message ID with a new record type to list sub-parts of the packet, > for example, reuse the existing record type (AUDIT_NETFILTER_PKT) for > the first 5 fields, mark and secmark, then another record type > (AUDIT_NETFILTER_PKT_ETH) for ethernet header, a record > (AUDIT_NETFILTER_PKT_IP) for IP/IPv6 header, then another > (AUDIT_NETFILTER_PKT_PROTO) for transport layer protocol. This way, the > absence of an ethernet bridge header won't swing out three fields, or waste 40 > chars. IPv4 adds about 20 chars not used by IPv6. TCP/UDP/DCCP/SCTP vs ICMP* > is about 25 chars each. The max message is 322 chars (eth bridge, IPv6). A > non-ethernet-bridge non-IP* message would be as little as 76 without the extra > fields, but as much as 219 with the extra fields filled with unset values. > > A full message could look like (I've left off secmark, which would go at the > end): > action=9 hook=99 len=99 inif= outif= mark=0x > smac=FF:FF:FF:FF:FF:FF dmac=FF:FF:FF:FF:FF:FF macproto=0x trunc=9 > saddr=::::::: > daddr=::::::: ipid=-1 proto=255 frag=-1 > trunc=9 sport=9 dport=9 icmptype=999 icmpcode=999 > At this point I think it would be good to hear what requirements exist for per-packet auditing. Steve, are there any current Common Criteria (or other) requirements that impact per-packet auditing? -- paul moore www.paul-moore.com -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: AUDIT_NETFILTER_PKT message format
On Tue, Jan 17, 2017 at 11:12 AM, Richard Guy Briggs <r...@redhat.com> wrote: > On 2017-01-17 08:55, Steve Grubb wrote: >> On Tuesday, January 17, 2017 12:25:51 AM EST Richard Guy Briggs wrote: ... >> > Ones that are not so straightforward: >> > - "secmark" depends on a kernel config setting, so should it always be >> > present but "(none)" if that kernel feature is compiled out? >> >> If this is selinux related, I'd treat it the same way that we do subj >> everywhere else. > > Ok. To be clear, a packet's secmark should be recorded via a dedicated field, e.g. "secmark", and not use the "subj" field (it isn't a subject label in the traditional sense). -- paul moore www.paul-moore.com -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V4 1/2] netfilter: xt_AUDIT: use consistent ipv4 network offset
On Wed, Mar 22, 2017 at 7:43 AM, Richard Guy Briggs <r...@redhat.com> wrote: > On 2017-03-22 12:11, Pablo Neira Ayuso wrote: >> On Wed, Mar 22, 2017 at 03:05:36AM -0400, Richard Guy Briggs wrote: >> > Even though the skb->data pointer has been moved from the link layer >> > header to the network layer header, use the same method to calculate the >> > offset in ipv4 and ipv6 routines. >> > >> > Signed-off-by: Richard Guy Briggs <r...@redhat.com> >> > --- >> > net/netfilter/xt_AUDIT.c |2 +- >> > 1 files changed, 1 insertions(+), 1 deletions(-) >> > >> > diff --git a/net/netfilter/xt_AUDIT.c b/net/netfilter/xt_AUDIT.c >> > index 4973cbd..cdb7cee 100644 >> > --- a/net/netfilter/xt_AUDIT.c >> > +++ b/net/netfilter/xt_AUDIT.c >> > @@ -76,7 +76,7 @@ static void audit_ip4(struct audit_buffer *ab, struct >> > sk_buff *skb) >> > struct iphdr _iph; >> > const struct iphdr *ih; >> > >> > - ih = skb_header_pointer(skb, 0, sizeof(_iph), &_iph); >> > + ih = skb_header_pointer(skb, skb_network_offset(skb), sizeof(_iph), >> > &_iph); >> >> This update is completely pointless. > > Its point is to be consistent with audit_ip6() and to prevent further > time consumed by confusion and head-scratching. I know it is slightly > slower with an identical result. Agree with Richard, merged to audit/next. -- paul moore www.paul-moore.com -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V4 2/2] audit: normalize NETFILTER_PKT
set(skb) + sizeof(_ip6h),
> , _off);
>
> audit_log_format(ab, " saddr=%pI6c daddr=%pI6c proto=%hhu",
> >saddr, >daddr, nexthdr);
>
> - if (offset)
> - audit_proto(ab, skb, nexthdr, offset);
> + return true;
> }
>
> static unsigned int
> audit_tg(struct sk_buff *skb, const struct xt_action_param *par)
> {
> - const struct xt_audit_info *info = par->targinfo;
> struct audit_buffer *ab;
> + int fam = -1;
>
> if (audit_enabled == 0)
> goto errout;
> -
> ab = audit_log_start(NULL, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
> if (ab == NULL)
> goto errout;
>
> - audit_log_format(ab, "action=%hhu hook=%u len=%u inif=%s outif=%s",
> -info->type, par->hooknum, skb->len,
> -par->in ? par->in->name : "?",
> -par->out ? par->out->name : "?");
> -
> - if (skb->mark)
> - audit_log_format(ab, " mark=%#x", skb->mark);
> -
> - if (skb->dev && skb->dev->type == ARPHRD_ETHER) {
> - audit_log_format(ab, " smac=%pM dmac=%pM macproto=0x%04x",
> -eth_hdr(skb)->h_source, eth_hdr(skb)->h_dest,
> -ntohs(eth_hdr(skb)->h_proto));
> -
> - if (par->family == NFPROTO_BRIDGE) {
> - switch (eth_hdr(skb)->h_proto) {
> - case htons(ETH_P_IP):
> - audit_ip4(ab, skb);
> - break;
> -
> - case htons(ETH_P_IPV6):
> - audit_ip6(ab, skb);
> - break;
> - }
> - }
> - }
> + audit_log_format(ab, "mark=%#x", skb->mark);
>
> switch (par->family) {
> + case NFPROTO_BRIDGE:
> + switch (eth_hdr(skb)->h_proto) {
> + case htons(ETH_P_IP):
> + fam = audit_ip4(ab, skb) ? NFPROTO_IPV4 : -1;
> + break;
> + case htons(ETH_P_IPV6):
> + fam = audit_ip6(ab, skb) ? NFPROTO_IPV6 : -1;
> + break;
> + }
> + break;
> case NFPROTO_IPV4:
> - audit_ip4(ab, skb);
> + fam = audit_ip4(ab, skb) ? NFPROTO_IPV4 : -1;
> break;
> -
> case NFPROTO_IPV6:
> - audit_ip6(ab, skb);
> + fam = audit_ip6(ab, skb) ? NFPROTO_IPV6 : -1;
> break;
> }
>
> -#ifdef CONFIG_NETWORK_SECMARK
> - if (skb->secmark)
> - audit_log_secctx(ab, skb->secmark);
> -#endif
> + if (fam == -1)
> + audit_log_format(ab, " saddr=? daddr=? proto=-1");
>
> audit_log_end(ab);
>
> --
> 1.7.1
>
> --
> Linux-audit mailing list
> linux-au...@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit
--
paul moore
www.paul-moore.com
On Wed, Mar 22, 2017 at 3:05 AM, Richard Guy Briggs <r...@redhat.com> wrote:
> Eliminate flipping in and out of message fields, dropping fields in the
> process.
>
> Sample raw message format IPv4 UDP:
> type=NETFILTER_PKT msg=audit(1487874761.386:228): mark=0xae8a2732
> saddr=127.0.0.1 daddr=127.0.0.1 proto=17^]
> Sample raw message format IPv6 ICMP6:
> type=NETFILTER_PKT msg=audit(1487874761.381:227): mark=0x223894b7 saddr=::1
> daddr=::1 proto=58^]
>
> Issue: https://github.com/linux-audit/audit-kernel/issues/11
> Test case: https://github.com/linux-audit/audit-testsuite/issues/43
>
> Signed-off-by: Richard Guy Briggs <r...@redhat.com>
> ---
> v4:
> Write out nfmark unmodified rather than trying to indicate "unset".
> Collapse/simplify switch/case statements.
> v3:
> Don't store interim values, but print immediately.
> v2:
> Trim down to 4 fields. Add raw samples.
>
> net/netfilter/xt_AUDIT.c | 124
> ++
> 1 files changed, 27 insertions(+), 97 deletions(-)
>
> diff --git a/net/netfilter/xt_AUDIT.c b/net/netfilter/xt_AUDIT.c
> index cdb7cee..582ee54 100644
> --- a/net/netfilter/xt_AUDIT.c
> +++ b/net/netfilter/xt_AUDIT.c
> @@ -31,146 +31,76 @@ MODULE_ALIAS("ip6t_AUDIT");
> MODULE_ALIAS("ebt_AUDIT");
> MODULE_ALIAS("arpt_AUDIT");
>
>
Re: [PATCH V3] audit: normalize NETFILTER_PKT
On Fri, Mar 3, 2017 at 8:22 AM, Florian Westphal <f...@strlen.de> wrote: > Paul Moore <p...@paul-moore.com> wrote: >> On Fri, Mar 3, 2017 at 7:45 AM, Florian Westphal <f...@strlen.de> wrote: >> > Richard Guy Briggs <r...@redhat.com> wrote: >> >> > Perhaps I'm missing something here, but let me ask again, how does >> >> > userspace distinguish between an unset nfmark and a nfmark of >> >> > 0x? >> >> >> >> It can't. >> > >> > It can if you log it as 0, as I asked in patch 1 review. >> > >> > (You wouldn't log sk uid of 0 as -1 either, would you?) >> >> I want to see the code able to handle the full range of nfmark values >> as well as the unset case; if that means we need to tweak userspace a >> bit, please work with Steve on that. > > There is no 'unset nfmark'. Its just a 32bit integer. Yes, my apologies, this thread has dragged on so long I muddled the details in my mind ... here is what I'm trying to get at, Richard's latest patch (unless I've missed one in my inbox) has the following line: audit_log_format(ab, "mark=%#x", skb->mark ?: -1); ... which I believe to be incorrect. I was trying to lead Richard along to that same realization, but it would appear I'm not having much success, so to put it bluntly, here is what I want that line to look like: audit_log_format(ab, "mark=%#x", skb->mark); -- paul moore www.paul-moore.com -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3] audit: normalize NETFILTER_PKT
On Fri, Mar 3, 2017 at 7:45 AM, Florian Westphal <f...@strlen.de> wrote: > Richard Guy Briggs <r...@redhat.com> wrote: >> > Perhaps I'm missing something here, but let me ask again, how does >> > userspace distinguish between an unset nfmark and a nfmark of >> > 0x? >> >> It can't. > > It can if you log it as 0, as I asked in patch 1 review. > > (You wouldn't log sk uid of 0 as -1 either, would you?) I want to see the code able to handle the full range of nfmark values as well as the unset case; if that means we need to tweak userspace a bit, please work with Steve on that. -- paul moore www.paul-moore.com -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3] audit: normalize NETFILTER_PKT
On Wed, Mar 1, 2017 at 5:34 PM, Richard Guy Briggs <r...@redhat.com> wrote:
> On 2017-03-01 17:19, Paul Moore wrote:
>> On Wed, Mar 1, 2017 at 11:28 AM, Richard Guy Briggs <r...@redhat.com> wrote:
>> > On 2017-02-28 17:22, Paul Moore wrote:
>> >> On Sun, Feb 26, 2017 at 3:49 PM, Richard Guy Briggs <r...@redhat.com>
>> >> wrote:
>> >> > Eliminate flipping in and out of message fields, dropping fields in the
>> >> > process.
>> >> >
>> >> > Sample raw message format IPv4 UDP:
>> >> > type=NETFILTER_PKT msg=audit(1487874761.386:228): mark=0xae8a2732
>> >> > saddr=127.0.0.1 daddr=127.0.0.1 proto=17^]
>> >> > Sample raw message format IPv6 ICMP6:
>> >> > type=NETFILTER_PKT msg=audit(1487874761.381:227): mark=0x223894b7
>> >> > saddr=::1 daddr=::1 proto=58^]
>> >> >
>> >> > Issue: https://github.com/linux-audit/audit-kernel/issues/11
>> >> > Test case: https://github.com/linux-audit/audit-testsuite/issues/43
>> >> >
>> >> > Signed-off-by: Richard Guy Briggs <r...@redhat.com>
>> >> > ---
>> >> > net/netfilter/xt_AUDIT.c | 122
>> >> > ++---
>> >> > 1 files changed, 27 insertions(+), 95 deletions(-)
>> >> >
>> >> > diff --git a/net/netfilter/xt_AUDIT.c b/net/netfilter/xt_AUDIT.c
>> >> > index 4973cbd..945fa29 100644
>> >> > --- a/net/netfilter/xt_AUDIT.c
>> >> > +++ b/net/netfilter/xt_AUDIT.c
>> >> > @@ -31,146 +31,78 @@ MODULE_ALIAS("ip6t_AUDIT");
>> >>
>> >> ...
>> >>
>> >> > -static void audit_ip4(struct audit_buffer *ab, struct sk_buff *skb)
>> >> > +static bool audit_ip4(struct audit_buffer *ab, struct sk_buff *skb)
>> >> > {
>> >> > struct iphdr _iph;
>> >> > const struct iphdr *ih;
>> >> >
>> >> > ih = skb_header_pointer(skb, 0, sizeof(_iph), &_iph);
>> >>
>> >> It seems like we should be using skb_network_offset(skb) instead of 0
>> >> above, yes? Granted, this isn't new, but let's fix it.
>> >
>> > Yes, I agree. How does this even work now? Maybe the MAC header hasn't
>> > been added yet (or has already been processed and stripped off) so that
>> > skb->data is already pointing at the network header and hence has an
>> > offset of 0. Can you be more explicit and elaborate to say if this what
>> > you were thinking?
>>
>> Unfortunately, not really, I haven't thought through of all the
>> situations and it has been a long time since I've had to worry about
>> things like this. I think we are in agreement that it needs to
>> change, so let's just make the change.
>
> Given Pablo's assurances, this could go either way, fix audit_ip4 to use
> skb_network_offset() or fix audit_ip6 to use zero. I don't have a
> strong opinion, but using zero would be more efficient while using
> skb_network_offset() would remove the doubt. Either way, the
> consistency will avoid raising doubt in the future as you have
> (rightfully) done.
Just use skb_network_offset() as it is the safer option and there is
plenty of precedence. Considering that we expect NETFILTER_PKT to see
limited use, I'm more concerned about it not breaking than some small
loss of performance.
>> >> > static unsigned int
>> >> > audit_tg(struct sk_buff *skb, const struct xt_action_param *par)
>> >> > {
>> >> > - const struct xt_audit_info *info = par->targinfo;
>> >> > struct audit_buffer *ab;
>> >> > + int fam = -1;
>> >> >
>> >> > if (audit_enabled == 0)
>> >> > goto errout;
>> >> > -
>> >> > ab = audit_log_start(NULL, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
>> >> > if (ab == NULL)
>> >> > goto errout;
>> >> >
>> >> > - audit_log_format(ab, "action=%hhu hook=%u len=%u inif=%s
>> >> > outif=%s",
>> >> > -info->type, par->hooknum, skb->len,
>> >> > -par->in ? par->in->name : "?",
>> >> > -par->out ? par->out->name : "?");
>> >> > -
>> >> > -
Re: [PATCH V3] audit: normalize NETFILTER_PKT
On Thu, Mar 2, 2017 at 9:00 PM, Richard Guy Briggs <r...@redhat.com> wrote:
> On 2017-03-02 19:16, Paul Moore wrote:
>> On Wed, Mar 1, 2017 at 5:34 PM, Richard Guy Briggs <r...@redhat.com> wrote:
>> > On 2017-03-01 17:19, Paul Moore wrote:
>> >> On Wed, Mar 1, 2017 at 11:28 AM, Richard Guy Briggs <r...@redhat.com>
>> >> wrote:
>> >> > On 2017-02-28 17:22, Paul Moore wrote:
>> >> >> On Sun, Feb 26, 2017 at 3:49 PM, Richard Guy Briggs <r...@redhat.com>
>> >> >> wrote:
>> >> >> > Eliminate flipping in and out of message fields, dropping fields in
>> >> >> > the process.
>> >> >> >
>> >> >> > Sample raw message format IPv4 UDP:
>> >> >> > type=NETFILTER_PKT msg=audit(1487874761.386:228): mark=0xae8a2732
>> >> >> > saddr=127.0.0.1 daddr=127.0.0.1 proto=17^]
>> >> >> > Sample raw message format IPv6 ICMP6:
>> >> >> > type=NETFILTER_PKT msg=audit(1487874761.381:227): mark=0x223894b7
>> >> >> > saddr=::1 daddr=::1 proto=58^]
>> >> >> >
>> >> >> > Issue: https://github.com/linux-audit/audit-kernel/issues/11
>> >> >> > Test case: https://github.com/linux-audit/audit-testsuite/issues/43
>> >> >> >
>> >> >> > Signed-off-by: Richard Guy Briggs <r...@redhat.com>
>> >> >> > ---
>> >> >> > net/netfilter/xt_AUDIT.c | 122
>> >> >> > ++---
>> >> >> > 1 files changed, 27 insertions(+), 95 deletions(-)
>> >> >> >
>> >> >> > diff --git a/net/netfilter/xt_AUDIT.c b/net/netfilter/xt_AUDIT.c
>> >> >> > index 4973cbd..945fa29 100644
>> >> >> > --- a/net/netfilter/xt_AUDIT.c
>> >> >> > +++ b/net/netfilter/xt_AUDIT.c
>> >> >> > @@ -31,146 +31,78 @@ MODULE_ALIAS("ip6t_AUDIT");
...
>> >> >> > static unsigned int
>> >> >> > audit_tg(struct sk_buff *skb, const struct xt_action_param *par)
>> >> >> > {
>> >> >> > - const struct xt_audit_info *info = par->targinfo;
>> >> >> > struct audit_buffer *ab;
>> >> >> > + int fam = -1;
>> >> >> >
>> >> >> > if (audit_enabled == 0)
>> >> >> > goto errout;
>> >> >> > -
>> >> >> > ab = audit_log_start(NULL, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
>> >> >> > if (ab == NULL)
>> >> >> > goto errout;
>> >> >> >
>> >> >> > - audit_log_format(ab, "action=%hhu hook=%u len=%u inif=%s
>> >> >> > outif=%s",
>> >> >> > -info->type, par->hooknum, skb->len,
>> >> >> > -par->in ? par->in->name : "?",
>> >> >> > -par->out ? par->out->name : "?");
>> >> >> > -
>> >> >> > - if (skb->mark)
>> >> >> > - audit_log_format(ab, " mark=%#x", skb->mark);
>> >> >> > -
>> >> >> > - if (skb->dev && skb->dev->type == ARPHRD_ETHER) {
>> >> >> > - audit_log_format(ab, " smac=%pM dmac=%pM
>> >> >> > macproto=0x%04x",
>> >> >> > -eth_hdr(skb)->h_source,
>> >> >> > eth_hdr(skb)->h_dest,
>> >> >> > -ntohs(eth_hdr(skb)->h_proto));
>> >> >> > + audit_log_format(ab, "mark=%#x", skb->mark ?: -1);
>> >> >>
>> >> >> How do Steve's userspace tools like the unset/-1 value represented
>> >> >> when it is a hex value: -1 or 0x?
>> >> >
>> >> > My understanding is they are set up to cope with this.
>> >>
>> >> How does userspace distinguish between an unset nfmark and a nfmark of
>> >> 0x?
>> >
>> > It never had to deal specifically with nfmark previously because it
>> > wasn't included if it was blank. Generally other values that are -1 are
>> > interpreted by the audit userspace tools as unset (session id, auid,
>> > etc...)
>>
>> Yes, I know, let me get straight to the point: should we use "mark=-1"
>> when the nfmark is unset instead of "mark=0x"?
>
> I'd prefer to keep the format as it was, explicitly labelled hex. The
> other fields that are printed as unset, -1, come out in the logs as
> MAX_UINT: "auid=4294967295 ses=4294967295", so I don't see any reason to
> change that convention. Once that field is known by userspace tools,
> they can interpret (-i) that as -1.
Perhaps I'm missing something here, but let me ask again, how does
userspace distinguish between an unset nfmark and a nfmark of
0x?
--
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/6 RFC] netfilter: add audit operation field
gt;af, table->name,
> private->number);
> audit_log_end(ab);
> @@ -1209,7 +1210,7 @@ struct xt_table_info *xt_replace_table(struct xt_table
> *table,
> AUDIT_NETFILTER_CFGSOLO);
> if (ab) {
> audit_log_task(ab);
> - audit_log_format(ab, " family=%u table=%s
> entries=%u",
> + audit_log_format(ab, " op=replace family=%u
> table=%s entries=%u",
> table->af, table->name,
> private->number);
> audit_log_end(ab);
> --
> 1.7.1
--
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/6 RFC] netfilter: ebtables: audit table registration
On Thu, May 18, 2017 at 1:21 PM, Richard Guy Briggs <r...@redhat.com> wrote:
> Generate audit NETFILTER_CFG records on ebtables table registration.
>
> Previously this was only being done for all x_tables operations and ebtables
> table replacement.
>
> Audit only when there is an existing syscall audit rule, otherwise issue a
> standalone record only on table modification rather than empty table creation.
> Include subject attributes to the new standalone NETFILTER_CFGSOLO record
> using
> audit_log_task().
>
> Here is a sample accompanied record:
> type=NETFILTER_CFG msg=audit(1494907217.558:5403): family=7 table=filter
> entries=0
>
> and unaccompanied case:
> type=UNKNOWN[1331] msg=audit(1494723394.832:111): auid=4294967295 uid=0
> gid=0 ses=4294967295 subj=system_u:system_r:iptables_t:s0 pid=556
> comm="ebtables-restor" exe="/usr/sbin/ebtables-restore" family=7 table=broute
> entries=1
>
> See: https://github.com/linux-audit/audit-kernel/issues/43
>
> Signed-off-by: Richard Guy Briggs <r...@redhat.com>
> ---
> net/bridge/netfilter/ebtables.c | 26 ++
> 1 files changed, 26 insertions(+), 0 deletions(-)
>
> diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
> index 743f9e6..7499232 100644
> --- a/net/bridge/netfilter/ebtables.c
> +++ b/net/bridge/netfilter/ebtables.c
> @@ -1251,6 +1251,32 @@ struct ebt_table * ebt_register_table(struct net *net,
> }
> list_add(>list, >xt.tables[NFPROTO_BRIDGE]);
> mutex_unlock(_mutex);
> +#ifdef CONFIG_AUDIT
> + if (audit_enabled) {
> + struct audit_buffer *ab;
> +
> + if(!audit_dummy_context()) {
> + ab = audit_log_start(current->audit_context,
> GFP_KERNEL,
> +AUDIT_NETFILTER_CFG);
> + if (ab) {
> + audit_log_format(ab, "family=%u table=%s
> entries=%u",
> +AF_BRIDGE, repl->name,
> +repl->nentries);
> + audit_log_end(ab);
> + }
> + } else if(repl->nentries) {
> + ab = audit_log_start(NULL, GFP_KERNEL,
> +AUDIT_NETFILTER_CFGSOLO);
> + if (ab) {
> + audit_log_task(ab);
> + audit_log_format(ab, " family=%u table=%s
> entries=%u",
> +AF_BRIDGE, repl->name,
> + repl->nentries);
> + audit_log_end(ab);
> + }
> + }
> + }
> +#endif
Similar comments from patch 3/6 apply here, let's stick with a single
audit record type.
--
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/6 RFC] netfilter: audit only on xtables and ebtables syscall rule or standalone
On Wed, May 24, 2017 at 2:09 PM, Richard Guy Briggs <r...@redhat.com> wrote: > On 2017-05-24 19:36, Pablo Neira Ayuso wrote: >> On Thu, May 18, 2017 at 01:21:49PM -0400, Richard Guy Briggs wrote: >> > There were syscall events unsolicited by any audit rule caused by a missing >> > !audit_dummy_context() check before creating an >> > iptables/ip6tables/arptables/ebtables NETFILTER_CFG record. Check >> > !audit_dummy_context() before creating the NETFILTER_CFG record. >> > >> > The vast majority of observed unaccompanied records are caused by the >> > fedora >> > default rule: "-a never,task" and the occasional early startup one is I >> > believe >> > caused by the iptables filter table module hard linked into the kernel >> > rather >> > than a loadable module. The !audit_dummy_context() check above should avoid >> > them. Audit only when there is an existing syscall audit rule, otherwise >> > issue >> > a standalone record only on table modification rather than empty table >> > creation. >> > >> > Add subject attributes to the new standalone NETFILTER_CFGSOLO record using >> > a newly exported audit_log_task(). >> >> This new NETFILTER_CFGSOLO looks like audit infra is missing some way >> to export a revision / context to userspace? It's duplicating quite a >> bit of the code from what I can see in this patch. > > Interesting you brought that up. I did another revision that stores > this information in a struct audit_context and greatly simplifies the > code in netfilter and re-uses code in audit itself, which may be a > better way to go, but that idea needed to settle a bit more before > seeing peer review. > > I'm also having doubts about two record types. Richard and I had a discussion about this a week (or two?) ago and I'm currently of the opinion that two record types are a mistake. I agree that we need to add the audit_dummy_context() check but the other changes in this patch I'm less excited about. -- paul moore www.paul-moore.com -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/6 RFC] netfilter: add audit netns ID
On Wed, May 24, 2017 at 3:44 PM, Eric W. Biederman
<ebied...@xmission.com> wrote:
> Richard Guy Briggs <r...@redhat.com> writes:
>
>> On 2017-05-24 19:31, Pablo Neira Ayuso wrote:
>>> Cc'ing Eric Biederman.
>>>
>>> On Thu, May 18, 2017 at 01:21:52PM -0400, Richard Guy Briggs wrote:
>>> > diff --git a/net/bridge/netfilter/ebtables.c
>>> > b/net/bridge/netfilter/ebtables.c
>>> > index 59b63a8..0f77b2a 100644
>>> > --- a/net/bridge/netfilter/ebtables.c
>>> > +++ b/net/bridge/netfilter/ebtables.c
>>> > @@ -27,6 +27,7 @@
>>> > #include
>>> > #include
>>> > #include
>>> > +#define PROC_DYNAMIC_FIRST 0xF000U
>>> > #include
>>> > /* needed for logical [in,out]-dev filtering */
>>> > #include "../br_private.h"
>>> > @@ -1075,7 +1076,8 @@ static int do_replace_finish(struct net *net,
>>> > struct ebt_replace *repl,
>>> >ab = audit_log_start(current->audit_context,
>>> > GFP_KERNEL,
>>> > AUDIT_NETFILTER_CFG);
>>> >if (ab) {
>>> > - audit_log_format(ab, "op=replace family=%u
>>> > table=%s entries=%u",
>>> > + audit_log_format(ab, "op=replace net=%u
>>> > family=%u table=%s entries=%u",
>>> > + net->ns.inum -
>>> > PROC_DYNAMIC_FIRST,
>>>
>>> IIRC, there was a discussion on exposing netns i-node number to
>>> userspace time ago on netdev and Eric Biederman was not happy about
>>> this?
>>
>> He was not happy about it being exposed in the /proc filesystem. We've
>> been talking since then and while we've not come to a definitive
>> conclusion there is a communication channel open.
>>
>> This is more of an RFC patch than the rest of this set and I didn't
>> seriously expect this one to be accepted, I did want to present the idea
>> to see if there were concerns or better ideas generated how to
>> differentiate this record from a seemingly identical one. The only
>> other ID would be the network namespace' struct pointer.
>>
>> At this stage, one thing that is missing is a device number to qualify
>> this namespace ID.
>>
>> Once I started printing the namespace proc inode number (minus the
>> starting offset) in decimal, it was very clear what was happenning and
>> seemed worth sharing that debugging tool patch.
>
> If the appropriate device number and full inode number is included I
> don't have any deep problems with the idea. I don't like the bare inode
> number as we have had in the past and may in the future have these inode
> numbers in multiple filesystems so the inode number by itself is not
> unique.
Let's punt on this netfilter record specific patch until we sort out
the general problem of recording namespace/container information in
the audit records.
--
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH ghak81 RFC V2 1/5] audit: normalize loginuid read access
On Sat, May 12, 2018 at 9:58 PM, Richard Guy Briggs <r...@redhat.com> wrote:
> Recognizing that the loginuid is an internal audit value, use an access
> function to retrieve the audit loginuid value for the task rather than
> reaching directly into the task struct to get it.
>
> Signed-off-by: Richard Guy Briggs <r...@redhat.com>
> ---
> kernel/auditsc.c | 18 +-
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 479c031..0d4e269 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -374,7 +374,7 @@ static int audit_field_compare(struct task_struct *tsk,
> case AUDIT_COMPARE_EGID_TO_OBJ_GID:
> return audit_compare_gid(cred->egid, name, f, ctx);
> case AUDIT_COMPARE_AUID_TO_OBJ_UID:
> - return audit_compare_uid(tsk->loginuid, name, f, ctx);
> + return audit_compare_uid(audit_get_loginuid(tsk), name, f,
> ctx);
> case AUDIT_COMPARE_SUID_TO_OBJ_UID:
> return audit_compare_uid(cred->suid, name, f, ctx);
> case AUDIT_COMPARE_SGID_TO_OBJ_GID:
> @@ -385,7 +385,7 @@ static int audit_field_compare(struct task_struct *tsk,
> return audit_compare_gid(cred->fsgid, name, f, ctx);
> /* uid comparisons */
> case AUDIT_COMPARE_UID_TO_AUID:
> - return audit_uid_comparator(cred->uid, f->op, tsk->loginuid);
> + return audit_uid_comparator(cred->uid, f->op,
> audit_get_loginuid(tsk));
> case AUDIT_COMPARE_UID_TO_EUID:
> return audit_uid_comparator(cred->uid, f->op, cred->euid);
> case AUDIT_COMPARE_UID_TO_SUID:
> @@ -394,11 +394,11 @@ static int audit_field_compare(struct task_struct *tsk,
> return audit_uid_comparator(cred->uid, f->op, cred->fsuid);
> /* auid comparisons */
> case AUDIT_COMPARE_AUID_TO_EUID:
> - return audit_uid_comparator(tsk->loginuid, f->op, cred->euid);
> + return audit_uid_comparator(audit_get_loginuid(tsk), f->op,
> cred->euid);
> case AUDIT_COMPARE_AUID_TO_SUID:
> - return audit_uid_comparator(tsk->loginuid, f->op, cred->suid);
> + return audit_uid_comparator(audit_get_loginuid(tsk), f->op,
> cred->suid);
> case AUDIT_COMPARE_AUID_TO_FSUID:
> - return audit_uid_comparator(tsk->loginuid, f->op,
> cred->fsuid);
> + return audit_uid_comparator(audit_get_loginuid(tsk), f->op,
> cred->fsuid);
> /* euid comparisons */
> case AUDIT_COMPARE_EUID_TO_SUID:
> return audit_uid_comparator(cred->euid, f->op, cred->suid);
> @@ -611,7 +611,7 @@ static int audit_filter_rules(struct task_struct *tsk,
> result = match_tree_refs(ctx, rule->tree);
> break;
> case AUDIT_LOGINUID:
> - result = audit_uid_comparator(tsk->loginuid, f->op,
> f->uid);
> + result =
> audit_uid_comparator(audit_get_loginuid(tsk), f->op, f->uid);
> break;
> case AUDIT_LOGINUID_SET:
> result = audit_comparator(audit_loginuid_set(tsk),
> f->op, f->val);
> @@ -2281,14 +2281,14 @@ int audit_signal_info(int sig, struct task_struct *t)
> struct audit_aux_data_pids *axp;
> struct task_struct *tsk = current;
> struct audit_context *ctx = tsk->audit_context;
> - kuid_t uid = current_uid(), t_uid = task_uid(t);
> + kuid_t uid = current_uid(), auid, t_uid = task_uid(t);
>
> if (auditd_test_task(t) &&
> (sig == SIGTERM || sig == SIGHUP ||
> sig == SIGUSR1 || sig == SIGUSR2)) {
> audit_sig_pid = task_tgid_nr(tsk);
> - if (uid_valid(tsk->loginuid))
> - audit_sig_uid = tsk->loginuid;
> + if (uid_valid(auid = audit_get_loginuid(tsk)))
> + audit_sig_uid = auid;
> else
> audit_sig_uid = uid;
> security_task_getsecid(tsk, _sig_sid);
A gentle reminder that you should try to make you patches as
"checkpatch clean" as possible (see scripts/checkpatch.pl). There are
several 80-char warnings, which aren't fatal, but the big no-no is
below:
ERROR: do not use assignment in if condition
#72: FILE: kernel/auditsc.c:2290:
+ if (uid_valid(auid = audit_get_loginuid(tsk)))
... while I don't completely agree with everything checkpatch has to
say, I definitely agree with checkpatch when it comes to assignments
in if conditions.
--
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH ghak81 RFC V2 2/5] audit: convert sessionid unset to a macro
On Sat, May 12, 2018 at 9:58 PM, Richard Guy Briggs <r...@redhat.com> wrote:
> Use a macro, "AUDIT_SID_UNSET", to replace each instance of
> initialization and comparison to an audit session ID.
>
> Signed-off-by: Richard Guy Briggs <r...@redhat.com>
> ---
> include/linux/audit.h | 2 +-
> include/net/xfrm.h | 2 +-
> include/uapi/linux/audit.h | 1 +
> init/init_task.c | 3 ++-
> kernel/auditsc.c | 4 ++--
> 5 files changed, 7 insertions(+), 5 deletions(-)
Merged, thanks.
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 75d5b03..5f86f7c 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -513,7 +513,7 @@ static inline kuid_t audit_get_loginuid(struct
> task_struct *tsk)
> }
> static inline unsigned int audit_get_sessionid(struct task_struct *tsk)
> {
> - return -1;
> + return AUDIT_SID_UNSET;
> }
> static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
> { }
> diff --git a/include/net/xfrm.h b/include/net/xfrm.h
> index a872379..fcce8ee 100644
> --- a/include/net/xfrm.h
> +++ b/include/net/xfrm.h
> @@ -751,7 +751,7 @@ static inline void xfrm_audit_helper_usrinfo(bool
> task_valid,
> audit_get_loginuid(current) :
> INVALID_UID);
> const unsigned int ses = task_valid ? audit_get_sessionid(current) :
> - (unsigned int) -1;
> + AUDIT_SID_UNSET;
>
> audit_log_format(audit_buf, " auid=%u ses=%u", auid, ses);
> audit_log_task_context(audit_buf);
> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index 4e61a9e..04f9bd2 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -465,6 +465,7 @@ struct audit_tty_status {
> };
>
> #define AUDIT_UID_UNSET (unsigned int)-1
> +#define AUDIT_SID_UNSET ((unsigned int)-1)
>
> /* audit_rule_data supports filter rules with both integer and string
> * fields. It corresponds with AUDIT_ADD_RULE, AUDIT_DEL_RULE and
> diff --git a/init/init_task.c b/init/init_task.c
> index 3ac6e75..74f60ba 100644
> --- a/init/init_task.c
> +++ b/init/init_task.c
> @@ -9,6 +9,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -119,7 +120,7 @@ struct task_struct init_task
> .thread_node= LIST_HEAD_INIT(init_signals.thread_head),
> #ifdef CONFIG_AUDITSYSCALL
> .loginuid = INVALID_UID,
> - .sessionid = (unsigned int)-1,
> + .sessionid = AUDIT_SID_UNSET,
> #endif
> #ifdef CONFIG_PERF_EVENTS
> .perf_event_mutex = __MUTEX_INITIALIZER(init_task.perf_event_mutex),
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 0d4e269..e157595 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -2050,7 +2050,7 @@ static void audit_log_set_loginuid(kuid_t koldloginuid,
> kuid_t kloginuid,
> int audit_set_loginuid(kuid_t loginuid)
> {
> struct task_struct *task = current;
> - unsigned int oldsessionid, sessionid = (unsigned int)-1;
> + unsigned int oldsessionid, sessionid = AUDIT_SID_UNSET;
> kuid_t oldloginuid;
> int rc;
>
> @@ -2064,7 +2064,7 @@ int audit_set_loginuid(kuid_t loginuid)
> /* are we setting or clearing? */
> if (uid_valid(loginuid)) {
> sessionid = (unsigned int)atomic_inc_return(_id);
> - if (unlikely(sessionid == (unsigned int)-1))
> + if (unlikely(sessionid == AUDIT_SID_UNSET))
> sessionid = (unsigned
> int)atomic_inc_return(_id);
> }
>
> --
> 1.8.3.1
>
--
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH ghak81 RFC V2 3/5] audit: use inline function to get audit context
On Sat, May 12, 2018 at 9:58 PM, Richard Guy Briggs <r...@redhat.com> wrote: > Recognizing that the audit context is an internal audit value, use an > access function to retrieve the audit context pointer for the task > rather than reaching directly into the task struct to get it. > > Signed-off-by: Richard Guy Briggs <r...@redhat.com> > --- > include/linux/audit.h| 14 ++-- > include/net/xfrm.h | 2 +- > kernel/audit.c | 6 ++-- > kernel/audit_watch.c | 2 +- > kernel/auditsc.c | 64 > +--- > net/bridge/netfilter/ebtables.c | 2 +- > net/core/dev.c | 2 +- > net/netfilter/x_tables.c | 2 +- > net/netlabel/netlabel_user.c | 2 +- > security/integrity/ima/ima_api.c | 2 +- > security/integrity/integrity_audit.c | 2 +- > security/lsm_audit.c | 2 +- > security/selinux/hooks.c | 4 +-- > security/selinux/selinuxfs.c | 6 ++-- > security/selinux/ss/services.c | 12 +++ > 15 files changed, 64 insertions(+), 60 deletions(-) Merged, but there was some fuzz due to the missing 1/5 patch and a handfull of checkpatch.pl fixes. Please take a look at the commit in the audit/next branch and if anything looks awry please send a patch to fix it. -- paul moore www.paul-moore.com -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH ghak81 RFC V2 5/5] audit: collect audit task parameters
): */
> -struct audit_context;
> struct backing_dev_info;
> struct bio_list;
> struct blk_plug;
> @@ -832,10 +832,8 @@ struct task_struct {
>
> struct callback_head*task_works;
>
> - struct audit_context*audit_context;
> #ifdef CONFIG_AUDITSYSCALL
> - kuid_t loginuid;
> - unsigned intsessionid;
> + struct audit_task_info audit;
> #endif
> struct seccomp seccomp;
>
> diff --git a/init/init_task.c b/init/init_task.c
> index 74f60ba..d33260d 100644
> --- a/init/init_task.c
> +++ b/init/init_task.c
> @@ -119,8 +119,11 @@ struct task_struct init_task
> .thread_group = LIST_HEAD_INIT(init_task.thread_group),
> .thread_node= LIST_HEAD_INIT(init_signals.thread_head),
> #ifdef CONFIG_AUDITSYSCALL
> - .loginuid = INVALID_UID,
> - .sessionid = AUDIT_SID_UNSET,
> + .audit = {
> + .loginuid = INVALID_UID,
> + .sessionid = AUDIT_SID_UNSET,
> + .ctx= NULL,
> + },
> #endif
> #ifdef CONFIG_PERF_EVENTS
> .perf_event_mutex = __MUTEX_INITIALIZER(init_task.perf_event_mutex),
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index d441d68..4c1fd18 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -836,7 +836,7 @@ static inline struct audit_context
> *audit_take_context(struct task_struct *tsk,
> int return_valid,
> long return_code)
> {
> - struct audit_context *context = tsk->audit_context;
> + struct audit_context *context = tsk->audit.ctx;
>
> if (!context)
> return NULL;
> @@ -2066,8 +2066,8 @@ int audit_set_loginuid(kuid_t loginuid)
> sessionid = (unsigned
> int)atomic_inc_return(_id);
> }
>
> - task->sessionid = sessionid;
> - task->loginuid = loginuid;
> + task->audit.sessionid = sessionid;
> + task->audit.loginuid = loginuid;
> out:
> audit_log_set_loginuid(oldloginuid, loginuid, oldsessionid,
> sessionid, rc);
> return rc;
> --
> 1.8.3.1
>
--
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH ghak81 RFC V2 4/5] audit: use inline function to set audit context
On Sat, May 12, 2018 at 9:58 PM, Richard Guy Briggs <r...@redhat.com> wrote: > Recognizing that the audit context is an internal audit value, use an > access function to set the audit context pointer for the task > rather than reaching directly into the task struct to set it. > > Signed-off-by: Richard Guy Briggs <r...@redhat.com> > --- > include/linux/audit.h | 6 ++ > kernel/auditsc.c | 7 +++ > kernel/fork.c | 2 +- > 3 files changed, 10 insertions(+), 5 deletions(-) Merged with some minor fuzz. -- paul moore www.paul-moore.com -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH ghak81 RFC V1 0/5] audit: group task params
On Fri, May 4, 2018 at 4:54 PM, Richard Guy Briggs <r...@redhat.com> wrote: > Group the audit parameters for each task into one structure. > In particular, remove the loginuid and sessionid values and the audit > context pointer from the task structure, replacing them with an audit > task information structure to contain them. Use access functions to > access audit values. > > Note: Use static allocation of the audit task information structure > initially. Dynamic allocation was considered and attempted, but isn't > ready yet. Static allocation has the limitation that future audit task > information structure changes would cause a visible change to the rest > of the kernel, whereas dynamic allocation would mostly hide any future > changes. > > The first four access normalization patches could stand alone. I agree that the first four patches have some standalone value, and since we are currently at -rc4, did you want to post another patchset of just those four patches with feedback incorporated? I imagine that should be quick work, and that way they aren't help up with any problems/discussion regarding the take_struct changes. > Passes audit-testsuite. > > Richard Guy Briggs (5): > audit: normalize loginuid read access > audit: convert sessionid unset to a macro > audit: use inline function to get audit context > audit: use inline function to set audit context > audit: collect audit task parameters > > MAINTAINERS | 2 +- > include/linux/audit.h| 30 ++--- > include/linux/audit_task.h | 31 ++ > include/linux/sched.h| 6 +-- > include/net/xfrm.h | 4 +- > include/uapi/linux/audit.h | 1 + > init/init_task.c | 8 +++- > kernel/audit.c | 4 +- > kernel/audit_watch.c | 2 +- > kernel/auditsc.c | 82 > ++-- > kernel/fork.c| 2 +- > net/bridge/netfilter/ebtables.c | 2 +- > net/core/dev.c | 2 +- > net/netfilter/x_tables.c | 2 +- > net/netlabel/netlabel_user.c | 2 +- > security/integrity/ima/ima_api.c | 2 +- > security/integrity/integrity_audit.c | 2 +- > security/lsm_audit.c | 2 +- > security/selinux/hooks.c | 4 +- > security/selinux/selinuxfs.c | 6 +-- > security/selinux/ss/services.c | 12 +++--- > 21 files changed, 129 insertions(+), 79 deletions(-) > create mode 100644 include/linux/audit_task.h > > -- > 1.8.3.1 > -- paul moore www.paul-moore.com -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH ghak81 RFC V1 1/5] audit: normalize loginuid read access
On Fri, May 4, 2018 at 4:54 PM, Richard Guy Briggs <r...@redhat.com> wrote:
> Recognizing that the loginuid is an internal audit value, use an access
> function to retrieve the audit loginuid value for the task rather than
> reaching directly into the task struct to get it.
>
> Signed-off-by: Richard Guy Briggs <r...@redhat.com>
> ---
> kernel/auditsc.c | 16
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 479c031..f3817d0 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -374,7 +374,7 @@ static int audit_field_compare(struct task_struct *tsk,
> case AUDIT_COMPARE_EGID_TO_OBJ_GID:
> return audit_compare_gid(cred->egid, name, f, ctx);
> case AUDIT_COMPARE_AUID_TO_OBJ_UID:
> - return audit_compare_uid(tsk->loginuid, name, f, ctx);
> + return audit_compare_uid(audit_get_loginuid(tsk), name, f,
> ctx);
> case AUDIT_COMPARE_SUID_TO_OBJ_UID:
> return audit_compare_uid(cred->suid, name, f, ctx);
> case AUDIT_COMPARE_SGID_TO_OBJ_GID:
> @@ -385,7 +385,7 @@ static int audit_field_compare(struct task_struct *tsk,
> return audit_compare_gid(cred->fsgid, name, f, ctx);
> /* uid comparisons */
> case AUDIT_COMPARE_UID_TO_AUID:
> - return audit_uid_comparator(cred->uid, f->op, tsk->loginuid);
> + return audit_uid_comparator(cred->uid, f->op,
> audit_get_loginuid(tsk));
> case AUDIT_COMPARE_UID_TO_EUID:
> return audit_uid_comparator(cred->uid, f->op, cred->euid);
> case AUDIT_COMPARE_UID_TO_SUID:
> @@ -394,11 +394,11 @@ static int audit_field_compare(struct task_struct *tsk,
> return audit_uid_comparator(cred->uid, f->op, cred->fsuid);
> /* auid comparisons */
> case AUDIT_COMPARE_AUID_TO_EUID:
> - return audit_uid_comparator(tsk->loginuid, f->op, cred->euid);
> + return audit_uid_comparator(audit_get_loginuid(tsk), f->op,
> cred->euid);
> case AUDIT_COMPARE_AUID_TO_SUID:
> - return audit_uid_comparator(tsk->loginuid, f->op, cred->suid);
> + return audit_uid_comparator(audit_get_loginuid(tsk), f->op,
> cred->suid);
> case AUDIT_COMPARE_AUID_TO_FSUID:
> - return audit_uid_comparator(tsk->loginuid, f->op,
> cred->fsuid);
> + return audit_uid_comparator(audit_get_loginuid(tsk), f->op,
> cred->fsuid);
> /* euid comparisons */
> case AUDIT_COMPARE_EUID_TO_SUID:
> return audit_uid_comparator(cred->euid, f->op, cred->suid);
> @@ -611,7 +611,7 @@ static int audit_filter_rules(struct task_struct *tsk,
> result = match_tree_refs(ctx, rule->tree);
> break;
> case AUDIT_LOGINUID:
> - result = audit_uid_comparator(tsk->loginuid, f->op,
> f->uid);
> + result =
> audit_uid_comparator(audit_get_loginuid(tsk), f->op, f->uid);
> break;
> case AUDIT_LOGINUID_SET:
> result = audit_comparator(audit_loginuid_set(tsk),
> f->op, f->val);
> @@ -2287,8 +2287,8 @@ int audit_signal_info(int sig, struct task_struct *t)
> (sig == SIGTERM || sig == SIGHUP ||
> sig == SIGUSR1 || sig == SIGUSR2)) {
> audit_sig_pid = task_tgid_nr(tsk);
> - if (uid_valid(tsk->loginuid))
> - audit_sig_uid = tsk->loginuid;
> + if (uid_valid(audit_get_loginuid(tsk)))
> + audit_sig_uid = audit_get_loginuid(tsk);
I realize this comment is a little silly given the nature of loginuid,
but if we are going to abstract away loginuid accesses (which I think
is good), we should probably access it once, store it in a local
variable, perform the validity check on the local variable, then
commit the local variable to audit_sig_uid. I realize a TOCTOU
problem is unlikely here, but with this new layer of abstraction it
seems that some additional safety might be a good thing.
> else
> audit_sig_uid = uid;
> security_task_getsecid(tsk, _sig_sid);
> --
> 1.8.3.1
--
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH ghak81 RFC V1 2/5] audit: convert sessionid unset to a macro
On Tue, May 8, 2018 at 9:34 PM, Richard Guy Briggs <r...@redhat.com> wrote:
> On 2018-05-04 16:54, Richard Guy Briggs wrote:
>> Use a macro, "AUDIT_SID_UNSET", to replace each instance of
>> initialization and comparison to an audit session ID.
>>
>> Signed-off-by: Richard Guy Briggs <r...@redhat.com>
>
> There's a minor issue with this patch, adding a header include to
> init/init_task.c in this patch and removing it from patch 5. That'll be
> in the next revision.
Okay, thanks for the heads-up. FWIW, this patch looks reasonable in
principle; changing magic numbers to macros/constants is almost always
a step in the right direction.
> I have dynamic allocation working, so that has a good chance of
> appearing too.
I'll comment on that in your patch 0, I just want to get through the
rest of the patches first.
>> ---
>> include/linux/audit.h | 2 +-
>> include/net/xfrm.h | 2 +-
>> include/uapi/linux/audit.h | 1 +
>> init/init_task.c | 2 +-
>> kernel/auditsc.c | 4 ++--
>> 5 files changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/linux/audit.h b/include/linux/audit.h
>> index 75d5b03..5f86f7c 100644
>> --- a/include/linux/audit.h
>> +++ b/include/linux/audit.h
>> @@ -513,7 +513,7 @@ static inline kuid_t audit_get_loginuid(struct
>> task_struct *tsk)
>> }
>> static inline unsigned int audit_get_sessionid(struct task_struct *tsk)
>> {
>> - return -1;
>> + return AUDIT_SID_UNSET;
>> }
>> static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
>> { }
>> diff --git a/include/net/xfrm.h b/include/net/xfrm.h
>> index a872379..fcce8ee 100644
>> --- a/include/net/xfrm.h
>> +++ b/include/net/xfrm.h
>> @@ -751,7 +751,7 @@ static inline void xfrm_audit_helper_usrinfo(bool
>> task_valid,
>> audit_get_loginuid(current) :
>> INVALID_UID);
>> const unsigned int ses = task_valid ? audit_get_sessionid(current) :
>> - (unsigned int) -1;
>> + AUDIT_SID_UNSET;
>>
>> audit_log_format(audit_buf, " auid=%u ses=%u", auid, ses);
>> audit_log_task_context(audit_buf);
>> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
>> index 4e61a9e..04f9bd2 100644
>> --- a/include/uapi/linux/audit.h
>> +++ b/include/uapi/linux/audit.h
>> @@ -465,6 +465,7 @@ struct audit_tty_status {
>> };
>>
>> #define AUDIT_UID_UNSET (unsigned int)-1
>> +#define AUDIT_SID_UNSET ((unsigned int)-1)
>>
>> /* audit_rule_data supports filter rules with both integer and string
>> * fields. It corresponds with AUDIT_ADD_RULE, AUDIT_DEL_RULE and
>> diff --git a/init/init_task.c b/init/init_task.c
>> index 3ac6e75..c788f91 100644
>> --- a/init/init_task.c
>> +++ b/init/init_task.c
>> @@ -119,7 +119,7 @@ struct task_struct init_task
>> .thread_node= LIST_HEAD_INIT(init_signals.thread_head),
>> #ifdef CONFIG_AUDITSYSCALL
>> .loginuid = INVALID_UID,
>> - .sessionid = (unsigned int)-1,
>> + .sessionid = AUDIT_SID_UNSET,
>> #endif
>> #ifdef CONFIG_PERF_EVENTS
>> .perf_event_mutex = __MUTEX_INITIALIZER(init_task.perf_event_mutex),
>> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>> index f3817d0..6e3ceb9 100644
>> --- a/kernel/auditsc.c
>> +++ b/kernel/auditsc.c
>> @@ -2050,7 +2050,7 @@ static void audit_log_set_loginuid(kuid_t
>> koldloginuid, kuid_t kloginuid,
>> int audit_set_loginuid(kuid_t loginuid)
>> {
>> struct task_struct *task = current;
>> - unsigned int oldsessionid, sessionid = (unsigned int)-1;
>> + unsigned int oldsessionid, sessionid = AUDIT_SID_UNSET;
>> kuid_t oldloginuid;
>> int rc;
>>
>> @@ -2064,7 +2064,7 @@ int audit_set_loginuid(kuid_t loginuid)
>> /* are we setting or clearing? */
>> if (uid_valid(loginuid)) {
>> sessionid = (unsigned int)atomic_inc_return(_id);
>> - if (unlikely(sessionid == (unsigned int)-1))
>> + if (unlikely(sessionid == AUDIT_SID_UNSET))
>> sessionid = (unsigned
>> int)atomic_inc_return(_id);
>> }
>>
>> --
>> 1.8.3.1
>>
>> --
>> Linux-audit mailing list
>> linux-au...@redhat.com
>> https://www.redhat.com/mailman/listinfo/linux-audit
>
> - RGB
>
> --
> Richard Guy Briggs <r...@redhat.com>
> 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-au...@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit
--
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH ghak81 RFC V1 5/5] audit: collect audit task parameters
if
Considering that the audit_context pointer is now in the
audit_task_info struct, should the audit_task_info struct be placed
outside the CONFIG_AUDITSYSCALL protections? Or rather, shouldn't the
CONFIG_AUDITSYSCALL protections be moved inside audit_task_info or
removed entirely?
> diff --git a/init/init_task.c b/init/init_task.c
> index c788f91..d33260d 100644
> --- a/init/init_task.c
> +++ b/init/init_task.c
> @@ -9,6 +9,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -118,8 +119,11 @@ struct task_struct init_task
> .thread_group = LIST_HEAD_INIT(init_task.thread_group),
> .thread_node= LIST_HEAD_INIT(init_signals.thread_head),
> #ifdef CONFIG_AUDITSYSCALL
> - .loginuid = INVALID_UID,
> - .sessionid = AUDIT_SID_UNSET,
> + .audit = {
> + .loginuid = INVALID_UID,
> + .sessionid = AUDIT_SID_UNSET,
> + .ctx= NULL,
> + },
> #endif
> #ifdef CONFIG_PERF_EVENTS
> .perf_event_mutex = __MUTEX_INITIALIZER(init_task.perf_event_mutex),
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index f294e4a..b5d8bff 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -2068,8 +2068,8 @@ int audit_set_loginuid(kuid_t loginuid)
> sessionid = (unsigned
> int)atomic_inc_return(_id);
> }
>
> - task->sessionid = sessionid;
> - task->loginuid = loginuid;
> + task->audit.sessionid = sessionid;
> + task->audit.loginuid = loginuid;
> out:
> audit_log_set_loginuid(oldloginuid, loginuid, oldsessionid,
> sessionid, rc);
> return rc;
> --
> 1.8.3.1
--
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH ghak81 V3 2/3] audit: normalize loginuid read access
On Wed, May 16, 2018 at 7:55 AM, Richard Guy Briggs <r...@redhat.com> wrote:
> Recognizing that the loginuid is an internal audit value, use an access
> function to retrieve the audit loginuid value for the task rather than
> reaching directly into the task struct to get it.
>
> Signed-off-by: Richard Guy Briggs <r...@redhat.com>
> ---
> kernel/auditsc.c | 24 +++-
> 1 file changed, 15 insertions(+), 9 deletions(-)
Also merged into audit/next.
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index f3d3dc6..ef3e189 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -374,7 +374,7 @@ static int audit_field_compare(struct task_struct *tsk,
> case AUDIT_COMPARE_EGID_TO_OBJ_GID:
> return audit_compare_gid(cred->egid, name, f, ctx);
> case AUDIT_COMPARE_AUID_TO_OBJ_UID:
> - return audit_compare_uid(tsk->loginuid, name, f, ctx);
> + return audit_compare_uid(audit_get_loginuid(tsk), name, f,
> ctx);
> case AUDIT_COMPARE_SUID_TO_OBJ_UID:
> return audit_compare_uid(cred->suid, name, f, ctx);
> case AUDIT_COMPARE_SGID_TO_OBJ_GID:
> @@ -385,7 +385,8 @@ static int audit_field_compare(struct task_struct *tsk,
> return audit_compare_gid(cred->fsgid, name, f, ctx);
> /* uid comparisons */
> case AUDIT_COMPARE_UID_TO_AUID:
> - return audit_uid_comparator(cred->uid, f->op, tsk->loginuid);
> + return audit_uid_comparator(cred->uid, f->op,
> + audit_get_loginuid(tsk));
> case AUDIT_COMPARE_UID_TO_EUID:
> return audit_uid_comparator(cred->uid, f->op, cred->euid);
> case AUDIT_COMPARE_UID_TO_SUID:
> @@ -394,11 +395,14 @@ static int audit_field_compare(struct task_struct *tsk,
> return audit_uid_comparator(cred->uid, f->op, cred->fsuid);
> /* auid comparisons */
> case AUDIT_COMPARE_AUID_TO_EUID:
> - return audit_uid_comparator(tsk->loginuid, f->op, cred->euid);
> + return audit_uid_comparator(audit_get_loginuid(tsk), f->op,
> + cred->euid);
> case AUDIT_COMPARE_AUID_TO_SUID:
> - return audit_uid_comparator(tsk->loginuid, f->op, cred->suid);
> + return audit_uid_comparator(audit_get_loginuid(tsk), f->op,
> + cred->suid);
> case AUDIT_COMPARE_AUID_TO_FSUID:
> - return audit_uid_comparator(tsk->loginuid, f->op,
> cred->fsuid);
> + return audit_uid_comparator(audit_get_loginuid(tsk), f->op,
> + cred->fsuid);
> /* euid comparisons */
> case AUDIT_COMPARE_EUID_TO_SUID:
> return audit_uid_comparator(cred->euid, f->op, cred->suid);
> @@ -611,7 +615,8 @@ static int audit_filter_rules(struct task_struct *tsk,
> result = match_tree_refs(ctx, rule->tree);
> break;
> case AUDIT_LOGINUID:
> - result = audit_uid_comparator(tsk->loginuid, f->op,
> f->uid);
> + result = audit_uid_comparator(audit_get_loginuid(tsk),
> + f->op, f->uid);
> break;
> case AUDIT_LOGINUID_SET:
> result = audit_comparator(audit_loginuid_set(tsk),
> f->op, f->val);
> @@ -2278,14 +2283,15 @@ int audit_signal_info(int sig, struct task_struct *t)
> {
> struct audit_aux_data_pids *axp;
> struct audit_context *ctx = audit_context();
> - kuid_t uid = current_uid(), t_uid = task_uid(t);
> + kuid_t uid = current_uid(), auid, t_uid = task_uid(t);
>
> if (auditd_test_task(t) &&
> (sig == SIGTERM || sig == SIGHUP ||
> sig == SIGUSR1 || sig == SIGUSR2)) {
> audit_sig_pid = task_tgid_nr(current);
> - if (uid_valid(current->loginuid))
> - audit_sig_uid = current->loginuid;
> + auid = audit_get_loginuid(current);
> + if (uid_valid(auid))
> + audit_sig_uid = auid;
> else
> audit_sig_uid = uid;
> security_task_getsecid(current, _sig_sid);
> --
> 1.8.3.1
>
--
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH ghak81 V3 1/3] audit: use new audit_context access funciton for seccomp_actions_logged
On Wed, May 16, 2018 at 7:55 AM, Richard Guy Briggs <r...@redhat.com> wrote:
> On the rebase of the following commit on the new seccomp actions_logged
> function, one audit_context access was missed.
>
> commit cdfb6b341f0f2409aba24b84f3b4b2bba50be5c5
> ("audit: use inline function to get audit context")
>
> Signed-off-by: Richard Guy Briggs <r...@redhat.com>
> ---
> kernel/auditsc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Merged into audit/next, thanks for the follow-up.
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index cbab0da..f3d3dc6 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -2497,7 +2497,7 @@ void audit_seccomp_actions_logged(const char *names,
> const char *old_names,
> if (!audit_enabled)
> return;
>
> - ab = audit_log_start(current->audit_context, GFP_KERNEL,
> + ab = audit_log_start(audit_context(), GFP_KERNEL,
> AUDIT_CONFIG_CHANGE);
> if (unlikely(!ab))
> return;
> --
> 1.8.3.1
--
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH ghak81 V3 3/3] audit: collect audit task parameters
- if (!context) > - return; > - > /* Check for system calls that do not go through the exit > * function (e.g., exit_group), then free context block. > * We use GFP_ATOMIC here because we might be doing this > * in the context of the idle thread */ > /* that can happen only if we are called from do_exit() */ > - if (context->in_syscall && context->current_state == > AUDIT_RECORD_CONTEXT) > + if (context && context->in_syscall && > + context->current_state == AUDIT_RECORD_CONTEXT) > audit_log_exit(context, tsk); > + /* Freeing the audit_task_info struct must be performed after > +* audit_log_exit() due to need for loginuid and sessionid. > +*/ > + info = tsk->audit; > + tsk->audit = NULL; > + kmem_cache_free(audit_task_cache, info); > + if (!context) > + return; > if (!list_empty(>killed_trees)) > audit_kill_trees(>killed_trees); > > @@ -2071,8 +2104,8 @@ int audit_set_loginuid(kuid_t loginuid) > sessionid = (unsigned > int)atomic_inc_return(_id); > } > > - task->sessionid = sessionid; > - task->loginuid = loginuid; > + task->audit->sessionid = sessionid; > + task->audit->loginuid = loginuid; > out: > audit_log_set_loginuid(oldloginuid, loginuid, oldsessionid, > sessionid, rc); > return rc; > diff --git a/kernel/fork.c b/kernel/fork.c > index cd18448..92ab849 100644 > --- a/kernel/fork.c > +++ b/kernel/fork.c > @@ -1713,7 +1713,7 @@ static __latent_entropy struct task_struct > *copy_process( > p->start_time = ktime_get_ns(); > p->real_start_time = ktime_get_boot_ns(); > p->io_context = NULL; > - audit_set_context(p, NULL); > + p->audit = NULL; > cgroup_fork(p); > #ifdef CONFIG_NUMA > p->mempolicy = mpol_dup(p->mempolicy); > -- > 1.8.3.1 -- paul moore www.paul-moore.com -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
