11:04 a.m.
Auparse just removes single quotes at the end of a field value and leaves
quotes at the beginning. With this patch, auparse removes quotes at the
beggining of a parsed field value and handles double quotes at the same way as
single quotes.
This is a simple test program to reproduce the problem:
-----
int main() {
const char *buffer= "type=VIRT_RESOURCE msg=audit(1327574186.046:174): user pid=6748
uid=0 auid=500 ses=1 subj=unconfined_u:unconfined_r:unconfined_t:s0 msg='virt=kvm
resrc=net reason=start vm=\"CentOS\" uuid=fb4149f5-9ff6-4095-f6d3-a1d03936fdfa
old-net='?' new-net='52:54:00:DB:AE:B4 test':
exe=\"/usr/sbin/libvirtd\" hostname=? addr=? terminal=?
res=success'\n";
auparse_state_t *au = auparse_init(AUSOURCE_BUFFER, buffer);
if (au == NULL) return -1;
while (auparse_next_event(au) > 0) {
printf("%s\n", auparse_find_field(au, "new-net"));
}
auparse_destroy(au);
return 0;
}
-----
---
auparse/ellist.c | 6 +++++-
1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/auparse/ellist.c b/auparse/ellist.c
index eafcfee..8c3061d 100644
--- a/auparse/ellist.c
+++ b/auparse/ellist.c
@@ -137,6 +137,9 @@ static int parse_up_record(rnode* r)
// Remove beginning cruft of name
if (*ptr == '(')
ptr++;
+ // Remove quotes
+ if (*val == '\'' || *val == '"')
+ val++;
n.name = strdup(ptr);
n.val = strdup(val);
// Remove trailing punctuation
@@ -149,7 +152,8 @@ static int parse_up_record(rnode* r)
n.val[len-1] = 0;
len--;
}
- if (len && n.val[len-1] == '\'') {
+ if (len && (n.val[len - 1] == '\''
+ || n.val[len - 1] == '"')) {
n.val[len-1] = 0;
len--;
}
--
1.7.1
11:04 a.m.
New subject: [PATCH 2/2] auvirt: Remove workaround for VM name searching
Auvirt adds quotes to the given VM name when creating the search criteria. With
the previous patch, this workaround is no longer needed and this patch removes it.
---
tools/auvirt/auvirt.c | 19 +------------------
1 files changed, 1 insertions(+), 18 deletions(-)
diff --git a/tools/auvirt/auvirt.c b/tools/auvirt/auvirt.c
index c04780a..11d6e97 100644
--- a/tools/auvirt/auvirt.c
+++ b/tools/auvirt/auvirt.c
@@ -312,24 +312,7 @@ int create_search_criteria(auparse_state_t *au)
}
}
if (vm) {
- /*
- * If a field has its value quoted in the audit log, for
- * example:
- * vm="guest-name"
- *
- * auparse will consider the field value with quotes when
- * matching a rule. For example, using the example above the
- * following rule will not match:
- * ausearch_add_item(au, "vm", "=", "guest-name",
how);
- *
- * But this rule will match:
- * ausearch_add_item(au, "vm", "=",
"\"guest-name\"", how);
- *
- * TODO use a better approach for this problem...
- */
- snprintf(expr, sizeof(expr), "\"%s\"", vm);
- if (ausearch_add_item(au, "vm", "=", expr,
- AUSEARCH_RULE_AND)) {
+ if (ausearch_add_item(au, "vm", "=", vm, AUSEARCH_RULE_AND)) {
fprintf(stderr, "Criteria error: id\n");
return 1;
}
--
1.7.1
1:06 p.m.
New subject: [PATCH 2/2] auvirt: Remove workaround for VM name searching
On Wednesday, February 08, 2012 12:04:58 PM Marcelo Cerri wrote:
Auvirt adds quotes to the given VM name when creating the search
criteria.
With the previous patch, this workaround is no longer needed and this
patch removes it.
What you are seeing here is actually a different problem. The description you
have:
using the example above the following rule will not match:
ausearch_add_item(au, "vm", "=", "guest-name", how);
But this rule will match:
ausearch_add_item(au, "vm", "=",
"\"guest-name\"", how);
describes the following issue. If you look at the vm field type, it has this
realtionship in typetab.h:
_S(AUPARSE_TYPE_ESCAPED, "vm"
Which means that if you are not getting a hit, the search algorithm might need
fixing. If the searched field type is escaped, the algorithm should escape the
field and then do the match. For example, what if you have a vm name of "test
run". It will wind up being escaped and looking like hex encoded ascii. This is
much worse than just adding quotes.
So, I think the best solution is make this invisible to the outside world. The
function call ausearch_add_item() should do a type lookup of the field and then
escape the value if the returned type is AUPARSE_TYPE_ESCAPED.
On output, your program probably wants to call auparse_get_field_type() and if
its AUPARSE_TYPE_ESCAPED, then call auparse_interpret_field() and output that.
-Steve
7:22 a.m.
New subject: [PATCH 2/2] auvirt: Remove workaround for VM name searching
Thanks for your explanation. I hadn't notice how escaped fields work.
Regarding the search algorithm fix, sorry but it is not clear to me
where you meant to say to add the type check and the escape. Did you
mean inside the ausearch_add_item or in the function which is calling
the ausearch_add_item function?
I'll submit a patch to libvirt instead and then update auvirt.
Regards,
Marcelo
On 02/08/2012 05:06 PM, Steve Grubb wrote:
On Wednesday, February 08, 2012 12:04:58 PM Marcelo Cerri wrote:
> Auvirt adds quotes to the given VM name when creating the search criteria.
> With the previous patch, this workaround is no longer needed and this
> patch removes it.
What you are seeing here is actually a different problem. The description you
have:
using the example above the following rule will not match:
ausearch_add_item(au, "vm", "=", "guest-name", how);
But this rule will match:
ausearch_add_item(au, "vm", "=",
"\"guest-name\"", how);
describes the following issue. If you look at the vm field type, it has this
realtionship in typetab.h:
_S(AUPARSE_TYPE_ESCAPED, "vm"
Which means that if you are not getting a hit, the search algorithm might need
fixing. If the searched field type is escaped, the algorithm should escape the
field and then do the match. For example, what if you have a vm name of "test
run". It will wind up being escaped and looking like hex encoded ascii. This is
much worse than just adding quotes.
So, I think the best solution is make this invisible to the outside world. The
function call ausearch_add_item() should do a type lookup of the field and then
escape the value if the returned type is AUPARSE_TYPE_ESCAPED.
On output, your program probably wants to call auparse_get_field_type() and if
its AUPARSE_TYPE_ESCAPED, then call auparse_interpret_field() and output that.
-Steve
7:35 a.m.
New subject: [PATCH 2/2] auvirt: Remove workaround for VM name searching
On Thursday, February 09, 2012 08:22:34 AM Marcelo Cerri wrote:
Thanks for your explanation. I hadn't notice how escaped fields
work.
Regarding the search algorithm fix, sorry but it is not clear to me
where you meant to say to add the type check and the escape. Did you
mean inside the ausearch_add_item or in the function which is calling
the ausearch_add_item function?
I think its best to put it inside the function so that app writers do not have
to think about it. They just pass a string and its fixed up. I was also thinking
about the alternative, which is to decode the fields during search and then
compare. But this would be slower because we decode every field value whether it
matches or not. So, we can just encode the item being searched for and then
compare raw values. I suppose the man page should clarify this for app writers
just in case.
I'll submit a patch to libvirt instead and then update auvirt.
I wished I caught that sooner, too. As for auvirt, since you know vm is an
escaped field, you don't actually need to put the "if" statement to check
its
type. You can just call the interpret function unconditionally and use its
output.
Thanks,
-Steve
11:51 a.m.
New subject: [PATCH 2/2] auvirt: Remove workaround for VM name searching
On 02/09/2012 11:35 AM, Steve Grubb wrote:
On Thursday, February 09, 2012 08:22:34 AM Marcelo Cerri wrote:
> Thanks for your explanation. I hadn't notice how escaped fields work.
>
> Regarding the search algorithm fix, sorry but it is not clear to me
> where you meant to say to add the type check and the escape. Did you
> mean inside the ausearch_add_item or in the function which is calling
> the ausearch_add_item function?
I think its best to put it inside the function so that app writers do not have
to think about it. They just pass a string and its fixed up. I was also thinking
about the alternative, which is to decode the fields during search and then
compare. But this would be slower because we decode every field value whether it
matches or not. So, we can just encode the item being searched for and then
compare raw values. I suppose the man page should clarify this for app writers
just in case.
Digging into auparse source code, I noticed there is an "interpreted"
version of ausearch_add_item (ausearch_add_interpreted_item). I could
get matches for the "vm" field using this function. Do you think that
it's still necessary to change ausearch_add_item?
> I'll submit a patch to libvirt instead and then update auvirt.
I wished I caught that sooner, too. As for auvirt, since you know vm is an
escaped field, you don't actually need to put the "if" statement to check
its
type. You can just call the interpret function unconditionally and use its
output.
Probably it'll also be necessary to add the "old-net" and
"new-net"
fields to the typetab.h file. If a field isn't in typetab.h, what type
is considered for it? Is it considered just a regular string?
Thanks,
-Steve
12:04 p.m.
New subject: [PATCH 2/2] auvirt: Remove workaround for VM name searching
On Thursday, February 09, 2012 12:51:24 PM Marcelo Cerri wrote:
On 02/09/2012 11:35 AM, Steve Grubb wrote:
> On Thursday, February 09, 2012 08:22:34 AM Marcelo Cerri wrote:
>> Thanks for your explanation. I hadn't notice how escaped fields work.
>>
>> Regarding the search algorithm fix, sorry but it is not clear to me
>> where you meant to say to add the type check and the escape. Did you
>> mean inside the ausearch_add_item or in the function which is calling
>> the ausearch_add_item function?
>
> I think its best to put it inside the function so that app writers do not
> have to think about it. They just pass a string and its fixed up. I was
> also thinking about the alternative, which is to decode the fields
> during search and then compare. But this would be slower because we
> decode every field value whether it matches or not. So, we can just
> encode the item being searched for and then compare raw values. I
> suppose the man page should clarify this for app writers just in case.
Digging into auparse source code, I noticed there is an "interpreted"
version of ausearch_add_item (ausearch_add_interpreted_item). I could
get matches for the "vm" field using this function.
Sure. That makes it easier. :)
Do you think that it's still necessary to change
ausearch_add_item?
I guess not.
>> I'll submit a patch to libvirt instead and then update
auvirt.
>
> I wished I caught that sooner, too. As for auvirt, since you know vm is
> an escaped field, you don't actually need to put the "if" statement
to
> check its type. You can just call the interpret function unconditionally
> and use its output.
Probably it'll also be necessary to add the "old-net" and
"new-net"
fields to the typetab.h file.
Why? They look like MAC addresses to me.
If a field isn't in typetab.h, what type is considered for it? Is
it considered
just a regular string?
Yes. Generally to need to be in the type tab there might need to some kind of
transformation from a binary form into a more readable presentation. For
example, uid=500, what does 500 mean? exit=-2, what does -2 mean? In terms of
transformations, areas that I think needs more work is translating some of the
syscall parameters so ausearch output is more meaningful. But this is low on the
list of things to do.
I guess at this point you can make a simple patch to auvirt that cleans it up.
-Steve
12:54 p.m.
On Wednesday, February 08, 2012 12:04:57 PM Marcelo Cerri wrote:
Auparse just removes single quotes at the end of a field value and
leaves
quotes at the beginning. With this patch, auparse removes quotes at the
beggining of a parsed field value and handles double quotes at the same way
as single quotes.
This would seem to be a mistake in the libvirt auditing code. They should not be
adding quotes. The double quote has a special meaning, so I don't think we can
or should patch around that. The single quote just shouldn't be there.
-Steve
This is a simple test program to reproduce the problem:
-----
int main() {
const char *buffer= "type=VIRT_RESOURCE msg=audit(1327574186.046:174):
user pid=6748 uid=0 auid=500 ses=1
subj=unconfined_u:unconfined_r:unconfined_t:s0 msg='virt=kvm resrc=net
reason=start vm=\"CentOS\" uuid=fb4149f5-9ff6-4095-f6d3-a1d03936fdfa
old-net='?' new-net='52:54:00:DB:AE:B4 test':
exe=\"/usr/sbin/libvirtd\"
hostname=? addr=? terminal=? res=success'\n"; auparse_state_t *au =
auparse_init(AUSOURCE_BUFFER, buffer);
if (au == NULL) return -1;
while (auparse_next_event(au) > 0) {
printf("%s\n", auparse_find_field(au, "new-net"));
}
auparse_destroy(au);
return 0;
}
-----
---
auparse/ellist.c | 6 +++++-
1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/auparse/ellist.c b/auparse/ellist.c
index eafcfee..8c3061d 100644
--- a/auparse/ellist.c
+++ b/auparse/ellist.c
@@ -137,6 +137,9 @@ static int parse_up_record(rnode* r)
// Remove beginning cruft of name
if (*ptr == '(')
ptr++;
+ // Remove quotes
+ if (*val == '\'' || *val == '"')
+ val++;
n.name = strdup(ptr);
n.val = strdup(val);
// Remove trailing punctuation
@@ -149,7 +152,8 @@ static int parse_up_record(rnode* r)
n.val[len-1] = 0;
len--;
}
- if (len && n.val[len-1] == '\'') {
+ if (len && (n.val[len - 1] == '\''
+ || n.val[len - 1] == '"')) {
n.val[len-1] = 0;
len--;
}
4648
days inactive
4649
days old
linux-audit@lists.linux-audit.osci.io
7 comments
2 participants
participants (2)
-
Marcelo Cerri -
Steve Grubb