Kernel audit output is inconsistent, hard to parse
The format of audit messages from the kernel is a mess. The bottom line is one cannot parse the audit messages without special case knowledge of each audit message because the data formatting does not follow any regular rules.
I don't know how it got this way, but it really needs to be fixed.
The primary offense is string formatting, specifically the use or non-use of the functions audit_log_hex() audit_log_untrustedstring(), and audit_log_n_untrustedstring(); depending on circumstances.
The net result is a field value might be one of the following cases:
1) a string without quotes (maybe a string, maybe an int, etc.)
2) a string enclosed in quotes (implies a string with no escaped chars)
3) a string which is represented as a sequence of hex values (not enclosed in quotes, but how do you distinguish this from case 1?)
Given the name=value formatting it is absolutely impossible to correctly interpret the value component unless you know how audit_log_format was invoked to generate the name=value pair. This will be dependent on the kernel version, the field name, and the audit record type. To be specific, during parsing only case 2 is unambiguous. You cannot determine between case 1 and case
3. Heuristics based on each character being in the hexadecimal character set fail for a significant subset of data, thus you don't know if the value is a string encoded in hexadecimal which needs to be decoded or a string which happens to be composed of hexadecimal characters but is not encoded.
Thus we have the situation where to correctly parse the name=value pair one must know the audit record type, the field name, and the kernel version. That is just plain CRAZY and UNNECESSARY. Trying hide this logic in auparse is just a band-aid over the problem compounded by the fact auparse does not always get it right either. This is in conjunction with the fact auparse has no way to know the kernel version of the audit data it is attempting to parse (nor should it even have tables based on kernel version).
The answer is to make the output parsable without special case knowledge. It would appear many of these problems were introduced with the functions audit_log_hex() audit_log_untrustedstring(), and audit_log_n_untrustedstring() which attempt to correct for a double quote, white space, or non-printable character in the output string. However these are not used uniformly nor do they follow any common approach for string representations in user land (why not?).
All field values without exception need to be enclosed in quotes to delimit the value. Special characters inside the quotes need to be escaped, following some standard convention. Please, lets not invent a new encoding, this problem has already been solved elsewhere many times before!
Also note the function audit_log_n_untrustedstring() in audit.c has a bug and ignores the len parameter (it iterates till it finds a NULL terminator even though it's supposed to stop after n chars).
Suggested Fix:
--------------
Most of these problems can easily be fixed if there is exactly one central place to format an audit field value. The function audit_log_vformat() could very easily ensure consistent formatting via % format specifiers in the format string, e.g.:
audit_log_format("n=%d path=%s", n, path);Building audit output piecewise would be deprecated, e.g. these types of sequences would be eliminated.
audit_log_format(ab, " n=%d", n);
audit_log_format(ab, " name=");
audit_log_foo();and replaced with:
audit_log_format(ab, " n=%d name=%s", foo_to_string(foo));
Whenever audit_log_vformat() encounters a % format specifier it formats to a string, then it converts the string to an escaped quoted string, and then inserts the escaped quoted string into the buffer (e.g. n="123" name="foo bar\n" )
This way the formatting is consistent, easy to apply, and is never special cased by the caller.
There are no performance penalties of any note, calling a routine to escape only needs to be done when the format specifier is %s. Currently this is already done for a subset of output strings, so all we're doing is removing the responsibility for escaping from the caller and doing it consistently instead of in a subset of cases.
I don't really care what the encoding is. I only care that it is an encoding with wide support. Backslash quoting is very popular, familiar and has many implementations. The MIME quoted-printable transfer encoding would be another option but might pose some problems with line endings. I think backslash quoting would be a good choice. I suspect everyone reading this message already knows exactly how to interpret a string with backslash escapes.
Auparse is not the answer:
--------------------------
Auparse is not the answer to irregular kernel audit message formatting. First of all it forces auparse to have special case logic which is not 100% robust and is tied to the kernel source code version.
Second, in it's current implementation auparse confuses transfer decoding and substitution, two entirely different concepts needing to be applied in entirely different circumstances, but which have been conflated.
auparse_get_field_str() returns the field value in it's encoded form, this is almost never of value to the caller. The caller wants the field value to be unencoded so it can operate on it. If you want the field value to be unencoded you have to call auparse_interpret_field(). But auparse_interpret_field() performs two distinctly different operations, it both decodes AND performs contextual substitution. Contextual substitution only has meaning when applied on the same host and at approximately the same time as when the audit record was generated. Contextual substitution is mainly of value for human readable output, it is difficult to utilize with automated machine processing. At the moment it is not possible to get a decoded value from auparse without it also performing undesired substitution.
While we're at it:
------------------
If we do fix the format of audit messages we might as well fix some other inconsistencies at the same time.
1) The initial part of AVC messages do not follow the standard name=value formatting used everywhere else in audit.
a) It includes the string "avc:" which is redundant with the audit record type (e.g. type=AVC), the string "avc:" should be removed, it serves no purpose and only makes parsing much harder because of the inconsistency.
b) denied|granted are bare words without a field name, it should be seresult="denied", once again to avoid special case parsing.
c) The list of operations are enclosed in curly braces {} without a field name, this should be seperms=xxx, where xxx is a list. The use of curly braces to encode a list in audit data is unique. We should define how any audit message should encode a list of values and use that consistently for all audit data. While one could define a syntax such as "[value1, value2]" or some such, it might be informative to look at how other transfer mechanisms such as structured markup and ldap handle this case. They both utilize the concept of multi-valued attributes. Thus there is no list structure, but an attribute is allowed to repeat itself and in the process implicitly creates a list of values for the attribute. Thus {read write} might be represented as seperms="read" seperms="write". This regularity makes parsing much easier, it avoids special case syntax.
2) (Note, this is not a kernel issue) The host data is currently prepended to the audit record with the format host=xxx. Is this an encoded string or not? It should be encoded and it should be encoded in exactly the same format as the name/value pairs in the audit records. The same holds true for the record type, it should follow the same syntax as every other name/value pair.
3) The string "audit(ssssss.mmmm:iiii):" is a critical delimiter, it separates record properties (e.g. host, type, timestamp) from record data, which must be a sequence of name="value" pairs. But the time stamp should really follow the name/value pair encoding used elsewhere.
Desired syntax:
---------------
Records consist of a sequence of name="value" pairs.
Ordering of name/value pairs is significant for multi-valued attributes (i.e. where name appears more than once), insignificant otherwise.
The value MUST be enclosed in double quotes with interior characters properly escaped.
White space between name, '=', and "value" is insignificant and ignored.
The audit record is partitioned into two parts
a) record properties (i.e. host, record type, timestamp)
b) record data
The partition of properties and data occurs at a colon delimiter, i.e. properties : data
The current formatting of the record timestamp
(e.g. audit(ssss.mmm:iii) is inconsistent with
all other name/value pairs. It should be "seconds="sss" milliseconds="mmm" serial="iii", this allows parsing to be regular and consistent.
Thus an audit record with consistent syntax would look like this, where brackets [] indicate optional components:
[host=""] type="" seconds="" milliseconds="" serial="" : name="" [name=""]
What has to change and what's optional:
---------------------------------------
The formatting of name/value pairs in the kernel must be fixed, it is simply impossible to correctly parse in it's current state.
The rest of the suggested changes are syntactic sugar which would make parsing easier because of regular syntax, but they are not critical. We could retain the existing formats if backwards compatibility is felt to trump syntactic cleanliness and ease in parsing. It's a judgment call over when and how to introduce change and the anticipated impact.
Signature · 1 line
John Dennis <[email protected]>
Hello,
John Dennis napsal(a):
Quoted 5 lines
The current formatting of the record timestamp
(e.g. audit(ssss.mmm:iii) is inconsistent with
all other name/value pairs. It should be "seconds="sss" milliseconds="mmm" serial="iii", this allows parsing to be regular and consistent.
Isn't this unnecessarily verbose? Just
time="sss.mmm" serial="iii"
would be smaller, easier to read - and it would allow using better time precision in the future.
Quoted 2 lines
It's a judgment call over when and how to introduce change and the anticipated impact.
If this change is implemented, we should use the opportunity to clean up other inconsistencies in audit messages - e.g. different messages use "success", "res" and "result" fields to record whether the audited operation was successful.
Also note that similar changes are necessary in user-space, e.g.
type=USER_ERR ...: ... msg='PAM: bad_ident acct=? :
exe="/usr/sbin/gdm-binary" (hostname=?, addr=?, terminal=? res=failed)' contains name-value pairs within a value, using both pairs of quotes.
Mirek
Quoted 10 lines · Miloslav Trmac
Hello,
John Dennis napsal(a):Quoted 5 lines
The current formatting of the record timestamp
(e.g. audit(ssss.mmm:iii) is inconsistent with
all other name/value pairs. It should be "seconds="sss" milliseconds="mmm" serial="iii", this allows parsing to be regular and consistent.Isn't this unnecessarily verbose? Just
time="sss.mmm" serial="iii"
would be smaller, easier to read - and it would allow using better time precision in the future.
It's a reasonable argument. I can see value in either approach.
Quoted 8 lines
Quoted 2 lines
It's a judgment call over when and how to introduce change and the anticipated impact.
If this change is implemented, we should use the opportunity to clean up other inconsistencies in audit messages - e.g. different messages use "success", "res" and "result" fields to record whether the audited operation was successful.
Also note that similar changes are necessary in user-space, e.g.
type=USER_ERR ...: ... msg='PAM:bad_identacct=? :
exe="/usr/sbin/gdm-binary" (hostname=?, addr=?, terminal=? res=failed)' contains name-value pairs within a value, using both pairs of quotes.
Excellent points. Thank you for drawing attention to these.
Signature · 1 line
John Dennis <[email protected]>
Quoted 1 line · On Tuesday 29 January 2008 17:56:36 John Dennis wrote:
The format of audit messages from the kernel is a mess.
<humor> John, the code was hard to write, so it should be hard to parse!
Quoted 3 lines
The bottom line is one cannot parse the audit messages without special case knowledge of each audit message because the data formatting does not follow any regular rules.
Hence the audit parsing library. The idea is to abstract this away so that anyone wanting to write a tool does not need to study all the messages and figure out the parsing rules.
The way forward has to be the audit parsing library. At this point, there are tools developed around these messages and making wholesale changes will break them. This is the reason that the SE Linux messages are such a mess. I've raised my concern with the developers on the selinux mail list and they essentially told me they are not willing to make any changes. But they did agree to some keywords for the fields you mention so that we could go ahead and code tools.
Quoted 1 line
I don't know how it got this way, but it really needs to be fixed.
Any fix will break someone's tool somewhere unless they are coded to the audit parsing library.
Quoted 2 lines
Most of these problems can easily be fixed if there is exactly one central place to format an audit field value.
This is what we've done with user space. As for the kernel, essentially there is no maintainer or anyone interested in doing audit work. I pretty much have to force people to touch it. So, good luck getting kernel work done.
At this point, I don't think we want to do too much to the kernel. If you know of any cases in the kernel that has result= instead of res=, send a patch. We should probably fix those.
Quoted 7 lines
Auparse is not the answer:
--------------------------Auparse is not the answer to irregular kernel audit message formatting. First of all it forces auparse to have special case logic which is not 100% robust and is tied to the kernel source code version.
This is the answer in so many ways. In order to make any change, you have to decouple applications from the actual data structure. You cannot normalize the data without breaking somebody somewhere.
For example, suppose we all agreed the data structure is an abomination and had to be fixed. We get all the code into 2.6.26 kernel. meanwhile Fedora 9 is released on the 2.6.24 kernel. We get the user space pieces fixed up to be released at the same time as 2.6.26. Then Fedora steps up to 2.6.25 kernel and then ultimately 2.6.26. The userspace in Fedora 9 was never intended to work with the new format. We can't keep the kernel team from doing what's right for everyone that wants new device drivers. We're stuck.
The only answer that is sane is to convert tools to auparse. When that is done, make some small changes over time and evolve the data slowly so any new quirks can be adjusted over time. We can introduce the notion of aliases to keep old tools working. A big change will be a big disruption.
Quoted 1 line
auparse_get_field_str()returns the field value in it's encoded form,
I would chose the words, raw form.
Quoted 2 lines
this is almost never of value to the caller. The caller wants the field value to be unencoded so it can operate on it.
Sometimes. It depends on the situation.
Quoted 2 lines
If you want the field value to be unencoded you have to call
auparse_interpret_field().
Correct.
Quoted 1 line
But
auparse_interpret_field()performs two distinctly different operations,
It does only one thing, that is translate the data from raw to interpreted form.
Quoted 3 lines
it both decodes AND performs contextual substitution. Contextual substitution only has meaning when applied on the same host and at approximately the same time as when the audit record was generated.
Correct. You are talking about something the library does not handle today. The reason is because there is no designed method to aggregate logs. So, when that work is done, auparse will be fixed up to handle the situation.
This rant kinda sounds like you are volunteering to help when we get there.
Quoted 5 lines
If we do fix the format of audit messages we might as well fix some other inconsistencies at the same time.
1) The initial part of AVC messages do not follow the standard name=value formatting used everywhere else in audit.
Right, they said bugger off, we like syslog better anyways.
Quoted 7 lines
a) It includes the string "avc:" which is redundant with the audit record type (e.g. type=AVC), the string "avc:" should be removed, it serves no purpose and only makes parsing much harder because of the inconsistency.
b) denied|granted are bare words without a field name, it should be seresult="denied", once again to avoid special case parsing.
What's worse...I'm writing IDS software. Sometimes all you have is an AVC record and no syscall record. You don't know if the system was in permissive or enforcing mode at the time of the syscall. (Sure you can test when you see the record, but that's at a different time than the event.) So, you have no way of assessing the impact of the AVC. Was the action really denied or not? You know what it wants to do, but not if it did it. There is no success or result field. Anyways, instead of whine about it here, I will eventually write to the selinux mail list where that kind of discussion belongs.
So, John, if you want selinux format changes, complain on their mail list. I've already done that and lost. :)
-Steve
Quoted 5 lines · Steve Grubb
Quoted 3 lines · On Tuesday 29 January 2008 17:56:36 John Dennis wrote:
The bottom line is one cannot parse the audit messages without special case knowledge of each audit message because the data formatting does not follow any regular rules.
Quoted 7 lines
Hence the audit parsing library. The idea is to abstract this away so that anyone wanting to write a tool does not need to study all the messages and figure out the parsing rules.
The way forward has to be the audit parsing library. At this point, there are tools developed around these messages and making wholesale changes will break them.
Okay, while userspace backwards compatibility is a valid and important concern, using it as reason to prevent fixing problems such as this is bad. We need to remember that backwards compatibility is a requirement for improvement, not a reason to block improvement. I'm not as much of an audit expert as some of the people on this list, but I'm certain there is a workable solution here, we just need to think harder and more creatively.
That said, don't take my comments to mean that I disagree with a userspace library to parse audit events, I think that is a good idea. What I think is a bad idea is using it as an excuse to fix the kernel.
Quoted 5 lines
This is the reason that the SE Linux messages are such a mess. I've raised my concern with the developers on the selinux mail list and they essentially told me they are not willing to make any changes. But they did agree to some keywords for the fields you mention so that we could go ahead and code tools.
I have a sneaking suspicion that if we made audit work better we could also make it work better for SELinux.
Quoted 7 lines
Quoted 2 lines
Most of these problems can easily be fixed if there is exactly one central place to format an audit field value.
This is what we've done with user space. As for the kernel, essentially there is no maintainer or anyone interested in doing audit work. I pretty much have to force people to touch it. So, good luck getting kernel work done.
I thought you/Eric/Al were the maintainer(s)? No? Okay, fine.
If that really is the problem I'll throw my hat into the ring and volunteer to maintain the kernel code. I'm not an audit expert but I learn quickly and I'm very allergic to people saying "we can't do/fix that". John brought up some really good points and I think dismissing them so quickly is doing a disservice to audit and the community.
Quoted 11 lines
Quoted 7 lines
Auparse is not the answer:
--------------------------Auparse is not the answer to irregular kernel audit message formatting. First of all it forces auparse to have special case logic which is not 100% robust and is tied to the kernel source code version.
This is the answer in so many ways. In order to make any change, you have to decouple applications from the actual data structure. You cannot normalize the data without breaking somebody somewhere.
That is why we have compatibility "modes", second versions that run in parallel, etc. While this problem may be new to audit, it is not new to the kernel or other software projects; it _is_ a solvable problem, it just requires some of that "hard work".
Signature · 2 lines
paul moore linux security @ hp
Quoted 10 lines · On Wednesday 30 January 2008 10:34:00 Paul Moore wrote:
Quoted 9 lines · Steve Grubb
Quoted 3 lines · On Tuesday 29 January 2008 17:56:36 John Dennis wrote:
The bottom line is one cannot parse the audit messages without special case knowledge of each audit message because the data formatting does not follow any regular rules.
Hence the audit parsing library. The idea is to abstract this away so that anyone wanting to write a tool does not need to study all the messages and figure out the parsing rules.
Quoted 6 lines
Quoted 3 lines
The way forward has to be the audit parsing library. At this point, there are tools developed around these messages and making wholesale changes will break them.
Okay, while userspace backwards compatibility is a valid and important concern,
That is what blocks any change. As long as we have apps that do their own parsing, we will break them all.
Quoted 3 lines
That said, don't take my comments to mean that I disagree with a userspace library to parse audit events, I think that is a good idea. What I think is a bad idea is using it as an excuse to fix the kernel.
That's not what i was saying. I said that using it is the way to allow changes to be made. We need to decouple the data representation from the apps, then we can tinker under the hood and make changes to just 1 library so that everything still works. If we don't, you will have to fixup all apps that parse audit data directly. (Which will be broken soon anyways.)
Quoted 8 lines
Quoted 5 lines
This is the reason that the SE Linux messages are such a mess. I've raised my concern with the developers on the selinux mail list and they essentially told me they are not willing to make any changes. But they did agree to some keywords for the fields you mention so that we could go ahead and code tools.
I have a sneaking suspicion that if we made audit work better we could also make it work better for SELinux.
Only if you had a willingness for them to change their tools. Last time I checked, they weren't interested as it would introduce a lot a rework for a problem they don't care about. Feel free to point out their format doesn't follow our convention. If you can convince them to change, super. I couldn't. I agree that it would be nice if they could make some changes. But they are a separate community from us.
Quoted 9 lines
Quoted 7 lines
Quoted 2 lines
Most of these problems can easily be fixed if there is exactly one central place to format an audit field value.
This is what we've done with user space. As for the kernel, essentially there is no maintainer or anyone interested in doing audit work. I pretty much have to force people to touch it. So, good luck getting kernel work done.
I thought you/Eric/Al were the maintainer(s)? No? Okay, fine.
I'll let them speak for themselves. I review the code and have a wishlist that I can sometimes get people to work on. Sometimes I do the work when no one else will.
Quoted 5 lines
Quoted 3 lines
This is the answer in so many ways. In order to make any change, you have to decouple applications from the actual data structure. You cannot normalize the data without breaking somebody somewhere.
That is why we have compatibility "modes",
So are you suggesting that a new auditctl command be introduced to tell the kernel to leave AVC's the way they are?
-Steve
Quoted 15 lines · Steve Grubb
Quoted 14 lines · On Wednesday 30 January 2008 10:34:00 Paul Moore wrote:
Quoted 12 lines · Steve Grubb
Quoted 3 lines · On Tuesday 29 January 2008 17:56:36 John Dennis wrote:
The bottom line is one cannot parse the audit messages without special case knowledge of each audit message because the data formatting does not follow any regular rules.
Hence the audit parsing library. The idea is to abstract this away so that anyone wanting to write a tool does not need to study all the messages and figure out the parsing rules.
The way forward has to be the audit parsing library. At this point, there are tools developed around these messages and making wholesale changes will break them.
Quoted 5 lines
Quoted 2 lines
Okay, while userspace backwards compatibility is a valid and important concern,
That is what blocks any change. As long as we have apps that do their own parsing, we will break them all.
You cut out my comment that said, "We need to remember that backwards compatibility is a requirement for improvement, not a reason to block improvement.". There are ways to introduce new/improved behavior while preserving old behavior during a transition period. Throwing your arms up and saying we can't make any changes is ... well, lazy to be honest.
Quoted 5 lines
Quoted 3 lines
That said, don't take my comments to mean that I disagree with a userspace library to parse audit events, I think that is a good idea. What I think is a bad idea is using it as an excuse to fix the kernel.
That's not what i was saying ...
Okay, I'm going to drop this point then.
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
This is the reason that the SE Linux messages are such a mess. I've raised my concern with the developers on the selinux mail list and they essentially told me they are not willing to make any changes. But they did agree to some keywords for the fields you mention so that we could go ahead and code tools.
Quoted 8 lines
Quoted 2 lines
I have a sneaking suspicion that if we made audit work better we could also make it work better for SELinux.
Only if you had a willingness for them to change their tools. Last time I checked, they weren't interested as it would introduce a lot a rework for a problem they don't care about. Feel free to point out their format doesn't follow our convention. If you can convince them to change, super. I couldn't. I agree that it would be nice if they could make some changes.
I understand this is not a new issue and you have tried many a time in the past only to run into a brick wall. I'm sensitive to that, I really am, but I think there is still an opportunity to find a better solution.
Quoted 1 line
But they are a separate community from us.
... that plays in the same sandbox. It's not really an "us versus them" issue, it's more of a "plays well with others" thing. We need to find out, in detail, what they object too and then work around it in a systematic and persistent manner. Regardless, let's drop this point for the time being as it is a distraction to the greater issue being discussed.
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
This is the answer in so many ways. In order to make any change, you have to decouple applications from the actual data structure. You cannot normalize the data without breaking somebody somewhere.
That is why we have compatibility "modes",
So are you suggesting that a new auditctl command be introduced to tell the kernel to leave AVC's the way they are?
Think in more general terms right now ... we need to figure out what we want the new solution to be before we figure out how to make it compatible.
Signature · 2 lines
paul moore linux security @ hp
Quoted 8 lines · Paul Moore
Quoted 4 lines · Steve Grubb
This is what we've done with user space. As for the kernel, essentially there is no maintainer or anyone interested in doing audit work. I pretty much have to force people to touch it. So, good luck getting kernel work done.
I thought you/Eric/Al were the maintainer(s)? No? Okay, fine.
Depends what the work is. I've been paying close attention and fixing problems that creep up around audit even if a bit slow getting those pushed to linus sometimes. We've got a lot of changes/fixes going into the new tree. Do I regularly just rewrite stuff to make it pretty/better? No. Lately its been all bug fix, no general 'make me better' work.
And although interesting, this isn't 'broken', so it is the number one thing on my infinitely long list of stuff to look at...
-Eric
Quoted 1 line · Eric Paris
And although interesting, this isn't 'broken', so it is
NOT
Quoted 9 lines
the number one
thing on my infinitely long list of stuff to look at...-Eric
--
Linux-audit mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/linux-audit
Quoted 5 lines · Steve Grubb
On Tuesday 29 January 2008 17:56:36 John Dennis wrote:
Hence the audit parsing library. The idea is to abstract this away so that anyone wanting to write a tool does not need to study all the messages and figure out the parsing rules.
The way forward has to be the audit parsing library.
The problem is auparse is just as screwed as anybody else. Unparseable output is is just plain wrong and inexcusable. You're suggesting auparse embed all sorts of hacks and heuristics to unravel a problem which should never exist in the first place. It's a house of cards which in time will collapse. You also haven't explained how auparse is going to deal with log data generated by different kernel versions, especially when logs are aggregated.
Quoted 1 line
tools developed around these messages and making wholesale changes will break them.
Break what is already fundamentally broken? That's not an answer ;-)
Quoted 1 line
Any fix will break someone's tool somewhere unless they are coded to the audit parsing library.
auparse is going to break too. The current situation is you can't determine if a field is encoded or not by reading the output, you also have to know the kernel source code, that's wrong.
Quoted 3 lines
Quoted 1 line
Auparse is not the answer to irregular kernel audit message
This is the answer in so many ways. In order to make any change, you have to decouple applications from the actual data structure. You cannot normalize the data without breaking somebody somewhere.
Which is why making the output so it can be parsed independent of the kernel version an essential requirement.
Quoted 1 line
For example, suppose we all agreed the data structure is an abomination and had to be fixed. We get all the code into 2.6.26 kernel. meanwhile Fedora 9 is released on the 2.6.24 kernel. We get the user space pieces fixed up to be released at the same time as 2.6.26. Then Fedora steps up to 2.6.25 kernel and then ultimately 2.6.26. The userspace in Fedora 9 was never intended to work with the new format. We can't keep the kernel team from doing what's right for everyone that wants new device drivers. We're stuck.
You're only stuck if the output can only be parsed by one version, if the output were regular the problem goes away. Isn't that the desired result?
Quoted 3 lines
Quoted 1 line
auparse_get_field_str()returns the field value in it's encoded form,I would chose the words, raw form.
Yes, raw is a better term. Some raw values are encoded, some aren't, that's the problem.
Quoted 4 lines
Quoted 2 lines
this is almost never of value to the caller. The caller wants the field value to be unencoded so it can operate on it.
Sometimes. It depends on the situation.
Very rarely. As an analogy 99.99% of the time you want your email client to decode the contents from the transfer encoding it arrived in, otherwise it's just gibberish. Raw form is really only useful when debugging the encoding/decoding.
Quoted 8 lines
Quoted 2 lines
If you want the field value to be unencoded you have to call
auparse_interpret_field().Correct.
Quoted 1 line
But
auparse_interpret_field()performs two distinctly different operations,It does only one thing, that is translate the data from raw to interpreted form.
Wrong :-) It does two entirely different things and those operations cannot be separated. The two operations are:
1) decoding (e.g. decoding a field value encoded in hexadecimal form back into it's original string)
2) interpretation (e.g. translating a uid field into a username). I call this interpretation "contextual substitution" because it's taking a field value and substituting in another value, often in a different format. You cannot interpret a field value until it has been decoded.
What if I don't want auparse to change the field value and instead simply return the field value? Currently you can't simply get the field value! Why? Because some fields are encoded, so you either get the raw encoded value (which is meaningless 99.99% of the time, if it had been encoded) or you get something which is completely munged.
Quoted 1 line
So, John, if you want selinux format changes, complain on their mail list. I've already done that and lost. :)
FWIW, I can live with not changing the message contents. But no one can live with a situation where the data can't be parsed, it is simply wrong. Just to be clear the problem is you can't determine as one parses if a field value is encoded or not which means you can't decide if it has to be decoded or not.
Here is an example from the real world, an audit message has this field
comm=df
So is the value the string "df" (e.g. disk free) or is this the hexadecimal encoded byte value 223? The only way to know is by looking at the kernel source code and knowing that the "comm" field in a specific audit record is generated by calling audit_log_untrustedstring(). What if it doesn't call that in an different kernel version? What if a new field is added in a new kernel version, how will the parser know what which function kernel used to generate the string? What if in one kernel version the string was output with audit_log_untrustedstring() but in another kernel version it wasn't?
Signature · 1 line
John Dennis <[email protected]>
Quoted 6 lines · John Dennis
The format of audit messages from the kernel is a mess. The bottom line is one cannot parse the audit messages without special case knowledge of each audit message because the data formatting does not follow any regular rules.
I don't know how it got this way, but it really needs to be fixed.
I agree.
Quoted 21 lines
Suggested Fix:
--------------Most of these problems can easily be fixed if there is exactly one central place to format an audit field value. The function
audit_log_vformat()could very easily ensure consistent formatting via % format specifiers in the format string, e.g.:audit_log_format("n=%d path=%s", n, path);Building audit output piecewise would be deprecated, e.g. these types of sequences would be eliminated.
audit_log_format(ab, " n=%d", n); audit_log_format(ab, " name="); audit_log_foo();and replaced with:
audit_log_format(ab, " n=%d name=%s", foo_to_string(foo));
Quoted 7 lines
Whenever
audit_log_vformat()encounters a % format specifier it formats to a string, then it converts the string to an escaped quoted string, and then inserts the escaped quoted string into the buffer (e.g. n="123" name="foo bar\n" )This way the formatting is consistent, easy to apply, and is never special cased by the caller.
I'm afraid that your proposed solution is not enough as it still relies on the caller for the overall message format. I'm a firm believer that if you have strict requirements that need to be met (I think we do, your concerns are not new) you need to enforce those requirements through the API, relying on others to adhere to a "gentlemen's agreement" regarding the careful use of a flexible API is always going to be problematic.
What I have in mind is an API similar to the following:
/* Audit field names */
enum {
AUD_NAME_UID,
AUD_NAME_GUID,
AUD_NAME_PATH,
... etc ...
}
/* Audit field value types */
enum {
AUD_TYPE_BOOL,
AUD_TYPE_U32,
AUD_TYPE_S32,
AUD_TYPE_U64,
AUD_TYPE_S64,
AUD_TYPE_STRING,
AUD_TYPE_STRING_RAW,
... etc ...
} audit_value_type;
/**
* audit_log_field - Record a name=value field in the audit message
* @buffer: Audit buffer/message
* @name: Audit message field name
* @type: Audit message field value type
* @length: Audit message field value length
* @value: Audit message field value
*
* Description:
* Record a name=value pair in the audit message specified by @buffer,
* the value in @value is automatically formatted according to @type. */
void audit_log_field(buffer, name, type, length, value);
So the following code snippet would record a UID and PATH fields:
audit_log_field(buffer, AUD_NAME_UID, sizeof(my_uid), my_uid); audit_log_field(buffer, AUD_NAME_PATH, my_path_len, my_path);
The way I see it, there are three main advantages to this approach: first, the caller must use a well defined field name, second, the caller is not responsible for any of the message formatting, third, we free the API from dealing with audit messages in a particular format (i.e. the message no longer needs to be in a string representation, it could be in a binary format ... or both). Done right, an API similar to the one above should solve a good deal of the variability in the audit messages while at the same time opening the door for future optimizations.
Quoted 7 lines
Auparse is not the answer:
--------------------------Auparse is not the answer to irregular kernel audit message formatting. First of all it forces auparse to have special case logic which is not 100% robust and is tied to the kernel source code version.
I also agree.
Quoted 8 lines
While we're at it:
------------------If we do fix the format of audit messages we might as well fix some other inconsistencies at the same time.
1) The initial part of AVC messages do not follow the standard name=value formatting used everywhere else in audit.
I believe there are long standing issues here, revolving around SELinux's desire to emit AVC messages regardless of the state of audit. I can't say I've payed too much attention to this in the past but there should be plenty of info in the archives (check the SELinux list too).
You might be better off dealing with the above API changes first then dealing with this.
Quoted 6 lines
What has to change and what's optional:
---------------------------------------The formatting of name/value pairs in the kernel must be fixed, it is simply impossible to correctly parse in it's current state.
Quoted 6 lines
The rest of the suggested changes are syntactic sugar which would make parsing easier because of regular syntax, but they are not critical. We could retain the existing formats if backwards compatibility is felt to trump syntactic cleanliness and ease in parsing. It's a judgment call over when and how to introduce change and the anticipated impact.
All reasons for why I think we need to remove as much of the formatting decisions from the caller.
Signature · 2 lines
paul moore linux security @ hp
Quoted 11 lines · John Dennis
The format of audit messages from the kernel is a mess. The bottom line is one cannot parse the audit messages without special case knowledge of each audit message because the data formatting does not follow any regular rules.
I don't know how it got this way, but it really needs to be fixed.
The primary offense is string formatting, specifically the use or non-use of the functions
audit_log_hex()audit_log_untrustedstring(), andaudit_log_n_untrustedstring(); depending on circumstances.
Quoted 8 lines
The net result is a field value might be one of the following cases:
1) a string without quotes (maybe a string, maybe an int, etc.)
2) a string enclosed in quotes (implies a string with no escaped chars)
3) a string which is represented as a sequence of hex values (not enclosed in quotes, but how do you distinguish this from case 1?)
I'm certainly not going to argue this isn't problomatic. I believe that case #1 should go away. Maybe when I get back from Chile I'll get a chance to look at that. Some fields just aren't ever strings, so I don't see a need to waste time/space writing " around something I know is always an int. But if the field can be a string it should ALWAYS be either case 2 or 3.
Quoted 12 lines
Given the name=value formatting it is absolutely impossible to correctly interpret the value component unless you know how
audit_log_formatwas invoked to generate the name=value pair. This will be dependent on the kernel version, the field name, and the audit record type. To be specific, during parsing only case 2 is unambiguous. You cannot determine between case 1 and case
3. Heuristics based on each character being in the hexadecimal character set fail for a significant subset of data, thus you don't know if the value is a string encoded in hexadecimal which needs to be decoded or a string which happens to be composed of hexadecimal characters but is not encoded.
It "shouldn't" depend on the kernel version, but I'm sure it does. I'm going to work on eliminating case 1, which makes a lot of this mute.
Quoted 7 lines
The answer is to make the output parsable without special case knowledge. It would appear many of these problems were introduced with the functions
audit_log_hex()audit_log_untrustedstring(), andaudit_log_n_untrustedstring()which attempt to correct for a double quote, white space, or non-printable character in the output string. However these are not used uniformly nor do they follow any common approach for string representations in user land (why not?).
What approach would you like when I try to audit binary data? You want me to start adding \x before every byte? Forget it, waste of space.
Quoted 5 lines
All field values without exception need to be enclosed in quotes to delimit the value. Special characters inside the quotes need to be escaped, following some standard convention. Please, lets not invent a new encoding, this problem has already been solved elsewhere many times before!
I don't want to waste time/effort having to write res="1" when I can just say res=1. I know you are complaining that you have to look at message type, field name, and kernel version. My opinion, you should just have to look at field name. We know res!="happy go lucky" so lets just print that int and move along. I certainly agree though that res needs to be consistant across all message types.
Quoted 4 lines
Also note the function
audit_log_n_untrustedstring()inaudit.chas a bug and ignores the len parameter (it iterates till it finds a NULL terminator even though it's supposed to stop after n chars).
Check the audit tree, I pretty much rewrote all of these and will be in linus's tree as soon as al pushes them...
Quoted 26 lines
Suggested Fix:
--------------Most of these problems can easily be fixed if there is exactly one central place to format an audit field value. The function
audit_log_vformat()could very easily ensure consistent formatting via % format specifiers in the format string, e.g.:audit_log_format("n=%d path=%s", n, path);Building audit output piecewise would be deprecated, e.g. these types of sequences would be eliminated.
audit_log_format(ab, " n=%d", n); audit_log_format(ab, " name="); audit_log_foo();and replaced with:
audit_log_format(ab, " n=%d name=%s", foo_to_string(foo));Whenever
audit_log_vformat()encounters a % format specifier it formats to a string, then it converts the string to an escaped quoted string, and then inserts the escaped quoted string into the buffer (e.g. n="123" name="foo bar\n" )
Quoted 8 lines
This way the formatting is consistent, easy to apply, and is never special cased by the caller.
There are no performance penalties of any note, calling a routine to escape only needs to be done when the format specifier is %s. Currently this is already done for a subset of output strings, so all we're doing is removing the responsibility for escaping from the caller and doing it consistently instead of in a subset of cases.
I've got no problems eliminating all of the %s in the code and making everything go through audit_log_string()
Quoted 8 lines
I don't really care what the encoding is. I only care that it is an encoding with wide support. Backslash quoting is very popular, familiar and has many implementations. The MIME quoted-printable transfer encoding would be another option but might pose some problems with line endings. I think backslash quoting would be a good choice. I suspect everyone reading this message already knows exactly how to interpret a string with backslash escapes.
My take, if the field name is something that can be a string you either get field="string" or you get field=737472696e67. Either way its easy to recognize. Quotes means string, no quotes means hex.
Quoted 7 lines
Auparse is not the answer:
--------------------------Auparse is not the answer to irregular kernel audit message formatting. First of all it forces auparse to have special case logic which is not 100% robust and is tied to the kernel source code version.
Like I said, I wish this wasn't true. Do you have an example so I can learn from past mistakes?
Quoted 16 lines
--------------
If we do fix the format of audit messages we might as well fix some other inconsistencies at the same time.
1) The initial part of AVC messages do not follow the standard name=value formatting used everywhere else in audit.
a) It includes the string "avc:" which is redundant with the audit record type (e.g. type=AVC), the string "avc:" should be removed, it serves no purpose and only makes parsing much harder because of the inconsistency.
b) denied|granted are bare words without a field name, it should be seresult="denied", once again to avoid special case parsing.
Quoted 14 lines
c) The list of operations are enclosed in curly braces {} without a field name, this should be seperms=xxx, where xxx is a list. The use of curly braces to encode a list in audit data is unique. We should define how any audit message should encode a list of values and use that consistently for all audit data. While one could define a syntax such as "[value1, value2]" or some such, it might be informative to look at how other transfer mechanisms such as structured markup and ldap handle this case. They both utilize the concept of multi-valued attributes. Thus there is no list structure, but an attribute is allowed to repeat itself and in the process implicitly creates a list of values for the attribute. Thus {read write} might be represented as seperms="read" seperms="write". This regularity makes parsing much easier, it avoids special case syntax.
While I have no problem with changing this, I'm not going to rewrite all of the documentation on how to read selinux messages nor am I going to rewrite all of the tools that parse/use selinux messages. SELinux is a user of the audit system and in my opinion should get to use it wants to use it. SELinux has had this syntax long before audit was a twinkle in Rik Faith's eye. What are you going to do when 'unknown kernel module X' decides to use the audit subsystem and they don't follow your name/value pair rules?
Audit tools needs to learn to deal with messages that are not from the audit system (external audit users like selinux and unknown kernel modules). Most likely by seeing the type=field and realizing this isn't from audit so I may not understand. Tools that parse selinux messages need to fully parse and understand selinux messages. Tools that parse audit messages need to fully parse and understand audit messages. I think this is 2 different operations.
Quoted 2 lines
The formatting of name/value pairs in the kernel must be fixed, it is simply impossible to correctly parse in it's current state.
name/value pairs from the audit system itself I completely agree with. Trying to force an external user of the audit system into the 'audit way' doesn't make sense. Why is the audit system trying to parse those message anyway? Its not actually a message from the audit system, its a message from selinux.
Quoted 8 lines · Eric Paris
On Tue, 2008-01-29 at 17:56 -0500, John Dennis wrote:
I'm certainly not going to argue this isn't problomatic. I believe that case #1 should go away. Maybe when I get back from Chile I'll get a chance to look at that. Some fields just aren't ever strings, so I don't see a need to waste time/space writing " around something I know is always an int. But if the field can be a string it should ALWAYS be either case 2 or 3.
I'm cool with not using quotes around everything, I very much see the appeal in that. But if you do that then *all* strings have to be quoted *and* you can't continue to use the current unquoted hexadecimal string encoding (case 3), it's ambiguous, you can't determine if its a string because the quotes would be missing. That means case 3 also must be eliminated. All strings have to be enclosed in quotes and what appears inside the quotes has to support encoding arbitrary bytes. FWIW backslash quoting meets this requirement, as does many other encodings in popular use.
Quoted 2 lines
It "shouldn't" depend on the kernel version, but I'm sure it does. I'm going to work on eliminating case 1, which makes a lot of this mute.
Eliminating both case 1 and case 3 would be necessary to make it moot. It would be a superb step forward and greatly appreciated.
Quoted 2 lines
What approach would you like when I try to audit binary data? You want me to start adding \x before every byte? Forget it, waste of space.
Encoding binary data is solved problem. Some encodings are more efficient than others, pick a standard encoding which satisfies your requirements. The only key is that when the input stream shifts into and out of encoded format it must be determinable only from the input stream. It cannot rely on private version specific knowledge that certain fields have this encoding.
Quoted 4 lines
I don't want to waste time/effort having to write res="1" when I can just say res=1. I know you are complaining that you have to look at message type, field name, and kernel version. My opinion, you should just have to look at field name.
I shouldn't have to look at the field name, this violates basic parsing rules and is tied to specific kernel versions, see first comment, there is an easy solution.
Quoted 2 lines
I've got no problems eliminating all of the %s in the code and making everything go through
audit_log_string()
That would be a big step forward and would solve many of the problems, thanks. Just be aware audit_log_string has to use a different encoding other than bare hexadecimal.
Quoted 3 lines
My take, if the field name is something that can be a string you either get field="string" or you get field=737472696e67. Either way its easy to recognize. Quotes means string, no quotes means hex.
How does the parser know the field can be a string? See above.
Signature · 1 line
John Dennis <[email protected]>
At one time we talked about converting to a binary record format. Maybe some structure would be helpful in defining and enforcing rules for what information in the records can/should be and what it all means.
-- ljk
Quoted 223 lines · John Dennis
The format of audit messages from the kernel is a mess. The bottom line is one cannot parse the audit messages without special case knowledge of each audit message because the data formatting does not follow any regular rules.
I don't know how it got this way, but it really needs to be fixed.
The primary offense is string formatting, specifically the use or non-use of the functions
audit_log_hex()audit_log_untrustedstring(), andaudit_log_n_untrustedstring(); depending on circumstances.The net result is a field value might be one of the following cases:
1) a string without quotes (maybe a string, maybe an int, etc.)
2) a string enclosed in quotes (implies a string with no escaped chars)
3) a string which is represented as a sequence of hex values (not enclosed in quotes, but how do you distinguish this from case 1?)
Given the name=value formatting it is absolutely impossible to correctly interpret the value component unless you know how
audit_log_formatwas invoked to generate the name=value pair. This will be dependent on the kernel version, the field name, and the audit record type. To be specific, during parsing only case 2 is unambiguous. You cannot determine between case 1 and case
3. Heuristics based on each character being in the hexadecimal character set fail for a significant subset of data, thus you don't know if the value is a string encoded in hexadecimal which needs to be decoded or a string which happens to be composed of hexadecimal characters but is not encoded.Thus we have the situation where to correctly parse the name=value pair one must know the audit record type, the field name, and the kernel version. That is just plain CRAZY and UNNECESSARY. Trying hide this logic in auparse is just a band-aid over the problem compounded by the fact auparse does not always get it right either. This is in conjunction with the fact auparse has no way to know the kernel version of the audit data it is attempting to parse (nor should it even have tables based on kernel version).
The answer is to make the output parsable without special case knowledge. It would appear many of these problems were introduced with the functions
audit_log_hex()audit_log_untrustedstring(), andaudit_log_n_untrustedstring()which attempt to correct for a double quote, white space, or non-printable character in the output string. However these are not used uniformly nor do they follow any common approach for string representations in user land (why not?).All field values without exception need to be enclosed in quotes to delimit the value. Special characters inside the quotes need to be escaped, following some standard convention. Please, lets not invent a new encoding, this problem has already been solved elsewhere many times before!
Also note the function
audit_log_n_untrustedstring()inaudit.chas a bug and ignores the len parameter (it iterates till it finds a NULL terminator even though it's supposed to stop after n chars).Suggested Fix:
--------------Most of these problems can easily be fixed if there is exactly one central place to format an audit field value. The function
audit_log_vformat()could very easily ensure consistent formatting via % format specifiers in the format string, e.g.:audit_log_format("n=%d path=%s", n, path);Building audit output piecewise would be deprecated, e.g. these types of sequences would be eliminated.
audit_log_format(ab, " n=%d", n); audit_log_format(ab, " name="); audit_log_foo();and replaced with:
audit_log_format(ab, " n=%d name=%s", foo_to_string(foo));Whenever
audit_log_vformat()encounters a % format specifier it formats to a string, then it converts the string to an escaped quoted string, and then inserts the escaped quoted string into the buffer (e.g. n="123" name="foo bar\n" )This way the formatting is consistent, easy to apply, and is never special cased by the caller.
There are no performance penalties of any note, calling a routine to escape only needs to be done when the format specifier is %s. Currently this is already done for a subset of output strings, so all we're doing is removing the responsibility for escaping from the caller and doing it consistently instead of in a subset of cases.
I don't really care what the encoding is. I only care that it is an encoding with wide support. Backslash quoting is very popular, familiar and has many implementations. The MIME quoted-printable transfer encoding would be another option but might pose some problems with line endings. I think backslash quoting would be a good choice. I suspect everyone reading this message already knows exactly how to interpret a string with backslash escapes.
Auparse is not the answer:
--------------------------Auparse is not the answer to irregular kernel audit message formatting. First of all it forces auparse to have special case logic which is not 100% robust and is tied to the kernel source code version.
Second, in it's current implementation auparse confuses transfer decoding and substitution, two entirely different concepts needing to be applied in entirely different circumstances, but which have been conflated.
auparse_get_field_str()returns the field value in it's encoded form, this is almost never of value to the caller. The caller wants the field value to be unencoded so it can operate on it. If you want the field value to be unencoded you have to callauparse_interpret_field(). Butauparse_interpret_field()performs two distinctly different operations, it both decodes AND performs contextual substitution. Contextual substitution only has meaning when applied on the same host and at approximately the same time as when the audit record was generated. Contextual substitution is mainly of value for human readable output, it is difficult to utilize with automated machine processing. At the moment it is not possible to get a decoded value from auparse without it also performing undesired substitution.While we're at it:
------------------If we do fix the format of audit messages we might as well fix some other inconsistencies at the same time.
1) The initial part of AVC messages do not follow the standard name=value formatting used everywhere else in audit.
a) It includes the string "avc:" which is redundant with the audit record type (e.g. type=AVC), the string "avc:" should be removed, it serves no purpose and only makes parsing much harder because of the inconsistency.
b) denied|granted are bare words without a field name, it should be seresult="denied", once again to avoid special case parsing.
c) The list of operations are enclosed in curly braces {} without a field name, this should be seperms=xxx, where xxx is a list. The use of curly braces to encode a list in audit data is unique. We should define how any audit message should encode a list of values and use that consistently for all audit data. While one could define a syntax such as "[value1, value2]" or some such, it might be informative to look at how other transfer mechanisms such as structured markup and ldap handle this case. They both utilize the concept of multi-valued attributes. Thus there is no list structure, but an attribute is allowed to repeat itself and in the process implicitly creates a list of values for the attribute. Thus {read write} might be represented as seperms="read" seperms="write". This regularity makes parsing much easier, it avoids special case syntax.
2) (Note, this is not a kernel issue) The host data is currently prepended to the audit record with the format host=xxx. Is this an encoded string or not? It should be encoded and it should be encoded in exactly the same format as the name/value pairs in the audit records. The same holds true for the record type, it should follow the same syntax as every other name/value pair.
3) The string "audit(ssssss.mmmm:iiii):" is a critical delimiter, it separates record properties (e.g. host, type, timestamp) from record data, which must be a sequence of name="value" pairs. But the time stamp should really follow the name/value pair encoding used elsewhere.
Desired syntax:
---------------Records consist of a sequence of name="value" pairs.
Ordering of name/value pairs is significant for multi-valued attributes (i.e. where name appears more than once), insignificant otherwise.
The value MUST be enclosed in double quotes with interior characters properly escaped.
White space between name, '=', and "value" is insignificant and ignored.
The audit record is partitioned into two parts
a) record properties (i.e. host, record type, timestamp)
b) record data
The partition of properties and data occurs at a colon delimiter, i.e. properties : data
The current formatting of the record timestamp
(e.g. audit(ssss.mmm:iii) is inconsistent with
all other name/value pairs. It should be "seconds="sss" milliseconds="mmm" serial="iii", this allows parsing to be regular and consistent.Thus an audit record with consistent syntax would look like this, where brackets [] indicate optional components:
[host=""] type="" seconds="" milliseconds="" serial="" : name="" [name=""]
What has to change and what's optional:
---------------------------------------The formatting of name/value pairs in the kernel must be fixed, it is simply impossible to correctly parse in it's current state.
The rest of the suggested changes are syntactic sugar which would make parsing easier because of regular syntax, but they are not critical. We could retain the existing formats if backwards compatibility is felt to trump syntactic cleanliness and ease in parsing. It's a judgment call over when and how to introduce change and the anticipated impact.
Quoted 1 line · On Thursday 31 January 2008 16:11:38 Linda Knippers wrote:
At one time we talked about converting to a binary record format.
At this point, I want things stable. I've spent the last 3 years working on the foundation to the audit system and we need to focus higher up the stack for a while. There's all kinds of neat things we can do if we don't keep reworking the bottom layer. :)
-Steve
Quoted 7 lines · Steve Grubb
Quoted 1 line · On Thursday 31 January 2008 16:11:38 Linda Knippers wrote:
At one time we talked about converting to a binary record format.
At this point, I want things stable. I've spent the last 3 years working on the foundation to the audit system and we need to focus higher up the stack for a while. There's all kinds of neat things we can do if we don't keep reworking the bottom layer. :)
... Neat things like building castles on the sand? ;)
(Sorry, couldn't resist!)
Signature · 2 lines
paul moore linux security @ hp
Quoted 4 lines · --- Paul Moore
Quoted 3 lines · Steve Grubb
Quoted 1 line · On Thursday 31 January 2008 16:11:38 Linda Knippers wrote:
At one time we talked about converting to a binary record format.
Quoted 8 lines
Quoted 4 lines
At this point, I want things stable. I've spent the last 3 years working on the foundation to the audit system and we need to focus higher up the stack for a while. There's all kinds of neat things we can do if we don't keep reworking the bottom layer. :)
... Neat things like building castles on the sand? ;)
(Sorry, couldn't resist!)
There's also the conversion from SELinux interfaces to LSM interfaces that I'll be revisiting before long. Always a pain to have to do something like that during a rewrite.
Casey Schaufler
[email protected]