Re: patching use-after-free and innocent memory leak in pfctl_optimzie.c

[Alexandr Nedvedicky]

On Fri, Nov 24, 2017 at 07:22:58PM +0100, Alexander Bluhm wrote:
> On Fri, Nov 24, 2017 at 01:11:08PM +0100, Alexandr Nedvedicky wrote:
> > the patch below is part of larger diff [1] I've sent earlier.  Leonardo 
> > seen a
> > pfctl.core, when pfctl_optimize failed to create a radix table. The use 
> > after
> > free happens in superblock_free() at 1621:
> 
> I have seen exactly the same crash this week.  My analysis came to
> the same result as yours.  But while I was still considering whether
> a reference count would be overkill for such a short-lived tool,
> you just fixed the bug.  Thanks!

thanks for looking at my changes. I had same doubts if I should go
for reference count overkill. Then finally the passion for correct
code won.

> 
> > @@ -315,9 +317,10 @@ pfctl_optimize_ruleset(struct pfctl *pf, struct 
> > pf_ruleset *rs)
> > err(1, "calloc");
> > memcpy(r, >por_rule, sizeof(*r));
> > TAILQ_INSERT_TAIL(rs->rules.active.ptr, r, entries);
> > -   free(por);
> > +   pf_opt_table_unref(por->por_src_tbl);
> > +   pf_opt_table_unref(por->por_dst_tbl);
> > }
> > -   free(block);
> > +   superblock_free(pf, block);
> > }
> >  
> > return (0);
> 
> I think you must not remove the free(por) line.  It is correct in
> your larger diff, but here you leak memory.

good catch, you are right.

thanks a lot
regards
sasha

Re: patching use-after-free and innocent memory leak in pfctl_optimzie.c

[Alexander Bluhm]

On Fri, Nov 24, 2017 at 01:11:08PM +0100, Alexandr Nedvedicky wrote:
> the patch below is part of larger diff [1] I've sent earlier.  Leonardo seen a
> pfctl.core, when pfctl_optimize failed to create a radix table. The use after
> free happens in superblock_free() at 1621:

I have seen exactly the same crash this week.  My analysis came to
the same result as yours.  But while I was still considering whether
a reference count would be overkill for such a short-lived tool,
you just fixed the bug.  Thanks!

> @@ -315,9 +317,10 @@ pfctl_optimize_ruleset(struct pfctl *pf, struct 
> pf_ruleset *rs)
>   err(1, "calloc");
>   memcpy(r, >por_rule, sizeof(*r));
>   TAILQ_INSERT_TAIL(rs->rules.active.ptr, r, entries);
> - free(por);
> + pf_opt_table_unref(por->por_src_tbl);
> + pf_opt_table_unref(por->por_dst_tbl);
>   }
> - free(block);
> + superblock_free(pf, block);
>   }
>  
>   return (0);

I think you must not remove the free(por) line.  It is correct in
your larger diff, but here you leak memory.

With that fixed, OK bluhm@

patching use-after-free and innocent memory leak in pfctl_optimzie.c

[Alexandr Nedvedicky]

Hello,

the patch below is part of larger diff [1] I've sent earlier.  Leonardo seen a
pfctl.core, when pfctl_optimize failed to create a radix table. The use after
free happens in superblock_free() at 1621:

1618 while ((por = TAILQ_FIRST(>sb_rules))) {
1619 TAILQ_REMOVE(>sb_rules, por, por_entry);
1620 if (por->por_src_tbl) {
1621 if (por->por_src_tbl->pt_buf) {
1622 pfr_buf_clear(por->por_src_tbl->pt_buf);
1623 free(por->por_src_tbl->pt_buf);
1624 }
1625 free(por->por_src_tbl);

in this case the superblock_free() is invoked from pfctl_optimize_ruleset():
 337 }
 338 free(por);
 339 }
 340 while ((block = TAILQ_FIRST())) {
 341 TAILQ_REMOVE(, block, sb_entry);
 342 superblock_free(pf, block);
 343 }
 344 return (1);
 345 }
 346 

the buffer (por->por_src_tbl) contains pattern 0xdfdfdf, which is a clear
indication we deal with freed memory.  Fortunately it was not hard to discover
culprit in function(combine_rules():
 526 return (1);
 527 p2->por_src_tbl = p1->por_src_tbl;
 528 if (p1->por_src_tbl->pt_rulecount >=
 529 TABLE_THRESHOLD) {
 530 TAILQ_REMOVE(>sb_rules, p2,
 531 por_entry);
 532 free(p2);
 533 }
we share por_src_tbl between two different instances (p1, p2) of pf_opt_rule
objects. The least invasive fix I could come up with is to introduce simple
reference count.

One more question remains to be answered:
how does it come there are no panics on OK path? (when optimizer
succeeds to create table)
Let's check pfctl_optimize_ruleset():
 307 rs->anchor->refcnt = 0;
 308 while ((block = TAILQ_FIRST())) {
 309 TAILQ_REMOVE(, block, sb_entry);
 310 
 311 while ((por = TAILQ_FIRST(>sb_rules))) {
 312 TAILQ_REMOVE(>sb_rules, por, por_entry);
 313 por->por_rule.nr = rs->anchor->refcnt++;
 314 if ((r = calloc(1, sizeof(*r))) == NULL)
 315 err(1, "calloc");
 316 memcpy(r, >por_rule, sizeof(*r));
 317 TAILQ_INSERT_TAIL(rs->rules.active.ptr, r, 
entries);
 318 free(por);
 319 }
 320 free(block);
 321 }
line 318 frees pf_opt_rule object, without taking care of por_*_tbl members.
also line 320 is better to call superblock_free().

All those glitches are fixed in patch below.

OK?

thanks and
regards
sasha

[1] https://marc.info/?l=openbsd-tech=151148479226177=2

8<---8<---8<--8<
diff --git a/sbin/pfctl/pfctl_optimize.c b/sbin/pfctl/pfctl_optimize.c
index a1b19781756..cea34c241de 100644
--- a/sbin/pfctl/pfctl_optimize.c
+++ b/sbin/pfctl/pfctl_optimize.c
@@ -238,6 +238,8 @@ int skip_cmp_src_addr(struct pf_rule *, struct pf_rule *);
 intskip_cmp_src_port(struct pf_rule *, struct pf_rule *);
 intsuperblock_inclusive(struct superblock *, struct pf_opt_rule *);
 void   superblock_free(struct pfctl *, struct superblock *);
+struct pf_opt_tbl *pf_opt_table_ref(struct pf_opt_tbl *);
+void   pf_opt_table_unref(struct pf_opt_tbl *);
 
 
 int (*skip_comparitors[PF_SKIP_COUNT])(struct pf_rule *, struct pf_rule *);
@@ -315,9 +317,10 @@ pfctl_optimize_ruleset(struct pfctl *pf, struct pf_ruleset 
*rs)
err(1, "calloc");
memcpy(r, >por_rule, sizeof(*r));
TAILQ_INSERT_TAIL(rs->rules.active.ptr, r, entries);
-   free(por);
+   pf_opt_table_unref(por->por_src_tbl);
+   pf_opt_table_unref(por->por_dst_tbl);
}
-   free(block);
+   superblock_free(pf, block);
}
 
return (0);
@@ -325,16 +328,8 @@ pfctl_optimize_ruleset(struct pfctl *pf, struct pf_ruleset 
*rs)
 error:
while ((por = TAILQ_FIRST(_queue))) {
TAILQ_REMOVE(_queue, por, por_entry);
-   if (por->por_src_tbl) {
-   pfr_buf_clear(por->por_src_tbl->pt_buf);
-   free(por->por_src_tbl->pt_buf);
-   free(por->por_src_tbl);
-   }
-   if (por->por_dst_tbl) {
-   pfr_buf_clear(por->por_dst_tbl->pt_buf);
-   free(por->por_dst_tbl->pt_buf);
-   free(por->por_dst_tbl);
-   }
+