[PATCH 1/2] auparse: Remove quotes from parsed fields

[PATCH 1/2] auparse: Remove quotes from parsed fields

[Marcelo Cerri]

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

[PATCH 2/2] auvirt: Remove workaround for VM name searching

[Marcelo Cerri]

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

Re: [PATCH 2/2] auvirt: Remove workaround for VM name searching

[Steve Grubb]

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

Re: [PATCH 2/2] auvirt: Remove workaround for VM name searching

[Marcelo Cerri]

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
>

Re: [PATCH 2/2] auvirt: Remove workaround for VM name searching

[Steve Grubb]

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

Re: [PATCH 2/2] auvirt: Remove workaround for VM name searching

[Marcelo Cerri]

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
>

Re: [PATCH 2/2] auvirt: Remove workaround for VM name searching

[Steve Grubb]

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

Re: [PATCH 1/2] auparse: Remove quotes from parsed fields

[Steve Grubb]

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--;
>  			}