man double free

classic Classic list List threaded Threaded
18 messages Options
Reply | Threaded
Open this post in threaded view
|

man double free

Abel Romero Prez
I've got a: man(13835) in free(): bogus pointer (double free?) 0x22c43c2813b

To check please, add the following function to .kshrc and run . ./.kshrc:


function man {
     set -A array "$@"
     tag=${array[$#-1]}
     PAGER="" MANPAGER="" /usr/bin/man -T html -c pfctl $@ >
/tmp/man.html | lynx /tmp/man.html#$tag
     #PAGER="" MANPAGER="" /usr/bin/man -T html -c $@ | lynx -stdin
}

Then launch on prompt: man id


The result if exploited is on screenshot, but on console as follows:

foo$ man id
Abort trap
foo$


man_dfree_cut.png (228K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: man double free

Otto Moerbeek
On Thu, Jun 11, 2020 at 03:15:55PM +0200, Romero Pérez, Abel wrote:

> I've got a: man(13835) in free(): bogus pointer (double free?) 0x22c43c2813b
>
> To check please, add the following function to .kshrc and run . ./.kshrc:
>
>
> function man {
>     set -A array "$@"
>     tag=${array[$#-1]}
>     PAGER="" MANPAGER="" /usr/bin/man -T html -c pfctl $@ > /tmp/man.html |
> lynx /tmp/man.html#$tag
>     #PAGER="" MANPAGER="" /usr/bin/man -T html -c $@ | lynx -stdin
> }
>
> Then launch on prompt: man id
>
>
> The result if exploited is on screenshot, but on console as follows:
>
> foo$ man id
> Abort trap
> foo$
>

This already trips the bug;

        man -T html -c pfctl id

No need for a custom man function. No clue yet why.

        -Otto

Reply | Threaded
Open this post in threaded view
|

Re: man double free

Abel Romero Prez


On 2020-06-11 15:59, Otto Moerbeek wrote:

> On Thu, Jun 11, 2020 at 03:15:55PM +0200, Romero Pérez, Abel wrote:
>
>> I've got a: man(13835) in free(): bogus pointer (double free?) 0x22c43c2813b
>>
>> To check please, add the following function to .kshrc and run . ./.kshrc:
>>
>>
>> function man {
>>      set -A array "$@"
>>      tag=${array[$#-1]}
>>      PAGER="" MANPAGER="" /usr/bin/man -T html -c pfctl $@ > /tmp/man.html |
>> lynx /tmp/man.html#$tag
>>      #PAGER="" MANPAGER="" /usr/bin/man -T html -c $@ | lynx -stdin
>> }
>>
>> Then launch on prompt: man id
>>
>>
>> The result if exploited is on screenshot, but on console as follows:
>>
>> foo$ man id
>> Abort trap
>> foo$
>>
>
> This already trips the bug;
>
> man -T html -c pfctl id
>
> No need for a custom man function. No clue yet why.
>
> -Otto
>
Confirmed, it exploits also with your cmd-line.

Reply | Threaded
Open this post in threaded view
|

Re: man double free

Abel Romero Prez


On 2020-06-11 16:13, Romero Pérez, Abel wrote:

>
>
> On 2020-06-11 15:59, Otto Moerbeek wrote:
>> On Thu, Jun 11, 2020 at 03:15:55PM +0200, Romero Pérez, Abel wrote:
>>
>>> I've got a: man(13835) in free(): bogus pointer (double free?)
>>> 0x22c43c2813b
>>>
>>> To check please, add the following function to .kshrc and run .
>>> ./.kshrc:
>>>
>>>
>>> function man {
>>>      set -A array "$@"
>>>      tag=${array[$#-1]}
>>>      PAGER="" MANPAGER="" /usr/bin/man -T html -c pfctl $@ >
>>> /tmp/man.html |
>>> lynx /tmp/man.html#$tag
>>>      #PAGER="" MANPAGER="" /usr/bin/man -T html -c $@ | lynx -stdin
>>> }
>>>
>>> Then launch on prompt: man id
>>>
>>>
>>> The result if exploited is on screenshot, but on console as follows:
>>>
>>> foo$ man id
>>> Abort trap
>>> foo$
>>>
>>
>> This already trips the bug;
>>
>>     man -T html -c pfctl id
>>
>> No need for a custom man function. No clue yet why.
>>
>>     -Otto
>>
> Confirmed, it exploits also with your cmd-line.
It seems to crash only when a binary on $PATH is specified as 2nd man
entry (for example id).

-Abel.

Reply | Threaded
Open this post in threaded view
|

Re: man double free

Klemens Nanni-2
In reply to this post by Otto Moerbeek
On Thu, Jun 11, 2020 at 03:59:09PM +0200, Otto Moerbeek wrote:
> This already trips the bug;
>
> man -T html -c pfctl id
>
> No need for a custom man function. No clue yet why.
This is in mandoc's HTML parser, but only happens for multiple manuals
in html.c:html_reset_internal():

164             while ((tag = h->tag) != NULL) {
165                     h->tag = tag->next;
166                     free(tag);
167             }

Note that it crashes differently depending on the optimization level:

        $ cd /usr/src/usr.bin/mandoc
        $ make DEBUG=-O0
        $ ./obj/mandoc -Thtml `man -w id cat` >/dev/null ; echo $?
        0

        $ make DEBUG=-O1
        $ ./obj/mandoc -Thtml `man -w id cat` >/dev/null
        Segmentation fault (core dumped)

        $ make DEBUG=-O2
        $ ./obj/mandoc -Thtml `man -w id cat` >/dev/null
        mandoc(32092) in free(): bogus pointer (double free?) 0x6641bab613b
        Abort trap (core dumped)

Need to run now, but wanted to share what seems to be the right direction.

Reply | Threaded
Open this post in threaded view
|

Re: man double free

Abel Romero Prez


On 2020-06-11 16:45, Klemens Nanni wrote:

> On Thu, Jun 11, 2020 at 03:59:09PM +0200, Otto Moerbeek wrote:
>> This already trips the bug;
>>
>> man -T html -c pfctl id
>>
>> No need for a custom man function. No clue yet why.
> This is in mandoc's HTML parser, but only happens for multiple manuals
> in html.c:html_reset_internal():
>
> 164             while ((tag = h->tag) != NULL) {
> 165                     h->tag = tag->next;
> 166                     free(tag);
> 167             }
>
> Note that it crashes differently depending on the optimization level:
>
> $ cd /usr/src/usr.bin/mandoc
> $ make DEBUG=-O0
> $ ./obj/mandoc -Thtml `man -w id cat` >/dev/null ; echo $?
> 0
>
> $ make DEBUG=-O1
> $ ./obj/mandoc -Thtml `man -w id cat` >/dev/null
> Segmentation fault (core dumped)
>
> $ make DEBUG=-O2
> $ ./obj/mandoc -Thtml `man -w id cat` >/dev/null
> mandoc(32092) in free(): bogus pointer (double free?) 0x6641bab613b
> Abort trap (core dumped)
>
> Need to run now, but wanted to share what seems to be the right direction.
>
Compile with -O0 to fix temporally the bug.
But, I also want to note that a binary is not need to be specified, can
be a just a file... (as second man entry).

Reply | Threaded
Open this post in threaded view
|

Re: man double free

Otto Moerbeek
On Thu, Jun 11, 2020 at 04:53:25PM +0200, Romero Pérez, Abel wrote:

>
>
> On 2020-06-11 16:45, Klemens Nanni wrote:
> > On Thu, Jun 11, 2020 at 03:59:09PM +0200, Otto Moerbeek wrote:
> > > This already trips the bug;
> > >
> > > man -T html -c pfctl id
> > >
> > > No need for a custom man function. No clue yet why.
> > This is in mandoc's HTML parser, but only happens for multiple manuals
> > in html.c:html_reset_internal():
> >
> > 164             while ((tag = h->tag) != NULL) {
> > 165                     h->tag = tag->next;
> > 166                     free(tag);
> > 167             }
> >
> > Note that it crashes differently depending on the optimization level:
> >
> > $ cd /usr/src/usr.bin/mandoc
> > $ make DEBUG=-O0
> > $ ./obj/mandoc -Thtml `man -w id cat` >/dev/null ; echo $?
> > 0
> >
> > $ make DEBUG=-O1
> > $ ./obj/mandoc -Thtml `man -w id cat` >/dev/null
> > Segmentation fault (core dumped)
> >
> > $ make DEBUG=-O2
> > $ ./obj/mandoc -Thtml `man -w id cat` >/dev/null
> > mandoc(32092) in free(): bogus pointer (double free?) 0x6641bab613b
> > Abort trap (core dumped)
> >
> > Need to run now, but wanted to share what seems to be the right direction.
> >
> Compile with -O0 to fix temporally the bug.
> But, I also want to note that a binary is not need to be specified, can be a
> just a file... (as second man entry).
>

This fixes it for me,

        -Otto

Index: main.c
===================================================================
RCS file: /cvs/src/usr.bin/mandoc/main.c,v
retrieving revision 1.247
diff -u -p -r1.247 main.c
--- main.c 24 Feb 2020 21:15:05 -0000 1.247
+++ main.c 11 Jun 2020 15:06:43 -0000
@@ -872,7 +872,7 @@ parse(struct mparse *mp, int fd, const c
  if (outst->outdata == NULL)
  outdata_alloc(outst, outconf);
  else if (outst->outtype == OUTT_HTML)
- html_reset(outst);
+ html_reset(outst->outdata);
 
  mandoc_xr_reset();
  meta = mparse_result(mp);

Reply | Threaded
Open this post in threaded view
|

Re: man double free

Abel Romero Prez


On 2020-06-11 17:07, Otto Moerbeek wrote:

> On Thu, Jun 11, 2020 at 04:53:25PM +0200, Romero Pérez, Abel wrote:
>
>>
>>
>> On 2020-06-11 16:45, Klemens Nanni wrote:
>>> On Thu, Jun 11, 2020 at 03:59:09PM +0200, Otto Moerbeek wrote:
>>>> This already trips the bug;
>>>>
>>>> man -T html -c pfctl id
>>>>
>>>> No need for a custom man function. No clue yet why.
>>> This is in mandoc's HTML parser, but only happens for multiple manuals
>>> in html.c:html_reset_internal():
>>>
>>> 164             while ((tag = h->tag) != NULL) {
>>> 165                     h->tag = tag->next;
>>> 166                     free(tag);
>>> 167             }
>>>
>>> Note that it crashes differently depending on the optimization level:
>>>
>>> $ cd /usr/src/usr.bin/mandoc
>>> $ make DEBUG=-O0
>>> $ ./obj/mandoc -Thtml `man -w id cat` >/dev/null ; echo $?
>>> 0
>>>
>>> $ make DEBUG=-O1
>>> $ ./obj/mandoc -Thtml `man -w id cat` >/dev/null
>>> Segmentation fault (core dumped)
>>>
>>> $ make DEBUG=-O2
>>> $ ./obj/mandoc -Thtml `man -w id cat` >/dev/null
>>> mandoc(32092) in free(): bogus pointer (double free?) 0x6641bab613b
>>> Abort trap (core dumped)
>>>
>>> Need to run now, but wanted to share what seems to be the right direction.
>>>
>> Compile with -O0 to fix temporally the bug.
>> But, I also want to note that a binary is not need to be specified, can be a
>> just a file... (as second man entry).
>>
>
> This fixes it for me,
>
> -Otto
>
> Index: main.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/mandoc/main.c,v
> retrieving revision 1.247
> diff -u -p -r1.247 main.c
> --- main.c 24 Feb 2020 21:15:05 -0000 1.247
> +++ main.c 11 Jun 2020 15:06:43 -0000
> @@ -872,7 +872,7 @@ parse(struct mparse *mp, int fd, const c
>   if (outst->outdata == NULL)
>   outdata_alloc(outst, outconf);
>   else if (outst->outtype == OUTT_HTML)
> - html_reset(outst);
> + html_reset(outst->outdata);
>  
>   mandoc_xr_reset();
>   meta = mparse_result(mp);
>
Only one comment, don't use -O0 flag as optimization (disabled) to hunt
more bugs of this kind.

Thanks by the patch.

Reply | Threaded
Open this post in threaded view
|

Re: man double free

Otto Moerbeek
On Thu, Jun 11, 2020 at 05:15:28PM +0200, Romero Pérez, Abel wrote:

>
>
> On 2020-06-11 17:07, Otto Moerbeek wrote:
> > On Thu, Jun 11, 2020 at 04:53:25PM +0200, Romero Pérez, Abel wrote:
> >
> > >
> > >
> > > On 2020-06-11 16:45, Klemens Nanni wrote:
> > > > On Thu, Jun 11, 2020 at 03:59:09PM +0200, Otto Moerbeek wrote:
> > > > > This already trips the bug;
> > > > >
> > > > > man -T html -c pfctl id
> > > > >
> > > > > No need for a custom man function. No clue yet why.
> > > > This is in mandoc's HTML parser, but only happens for multiple manuals
> > > > in html.c:html_reset_internal():
> > > >
> > > > 164             while ((tag = h->tag) != NULL) {
> > > > 165                     h->tag = tag->next;
> > > > 166                     free(tag);
> > > > 167             }
> > > >
> > > > Note that it crashes differently depending on the optimization level:
> > > >
> > > > $ cd /usr/src/usr.bin/mandoc
> > > > $ make DEBUG=-O0
> > > > $ ./obj/mandoc -Thtml `man -w id cat` >/dev/null ; echo $?
> > > > 0
> > > >
> > > > $ make DEBUG=-O1
> > > > $ ./obj/mandoc -Thtml `man -w id cat` >/dev/null
> > > > Segmentation fault (core dumped)
> > > >
> > > > $ make DEBUG=-O2
> > > > $ ./obj/mandoc -Thtml `man -w id cat` >/dev/null
> > > > mandoc(32092) in free(): bogus pointer (double free?) 0x6641bab613b
> > > > Abort trap (core dumped)
> > > >
> > > > Need to run now, but wanted to share what seems to be the right direction.
> > > >
> > > Compile with -O0 to fix temporally the bug.
> > > But, I also want to note that a binary is not need to be specified, can be a
> > > just a file... (as second man entry).
> > >
> >
> > This fixes it for me,
> >
> > -Otto
> >
> > Index: main.c
> > ===================================================================
> > RCS file: /cvs/src/usr.bin/mandoc/main.c,v
> > retrieving revision 1.247
> > diff -u -p -r1.247 main.c
> > --- main.c 24 Feb 2020 21:15:05 -0000 1.247
> > +++ main.c 11 Jun 2020 15:06:43 -0000
> > @@ -872,7 +872,7 @@ parse(struct mparse *mp, int fd, const c
> >   if (outst->outdata == NULL)
> >   outdata_alloc(outst, outconf);
> >   else if (outst->outtype == OUTT_HTML)
> > - html_reset(outst);
> > + html_reset(outst->outdata);
> >   mandoc_xr_reset();
> >   meta = mparse_result(mp);
> >
> Only one comment, don't use -O0 flag as optimization (disabled) to hunt more
> bugs of this kind.

I have no clue what you mean by above sentence. If code has a bug,
optmization level might cause the bug to be hidden or exposed; it can
work both ways.

        -Otto

>
> Thanks by the patch.
>

Reply | Threaded
Open this post in threaded view
|

Re: man double free

Ingo Schwarze
In reply to this post by Otto Moerbeek
Hi Otto,

Otto Moerbeek wrote on Thu, Jun 11, 2020 at 05:07:17PM +0200:

> This fixes it for me,

Indeed, i came up with exactly the same diff and had already
written this commit message, but was still testing:

  Fix a regression in rev. 1.238 (2019/07/26):
  Pass the right object to html_reset() or it will crash
  when rendering more than one manual page to HTML in a row.
  Bug reported by Abel Romero Perez <romeroperezabel at gmail dot com>.

Given that you published the diff earlier, please commit, OK schwarze@.


This teaches once again that having type safety is really useful
and artificially circumventing it tends to invite trouble.

On first sight, it seems surprising that such a blatant regression
could survive almost a year.  Then again, the reason probably
is that

  <html>...</html><html>...</html>

isn't really valid HTML in the first place, so i guess few people
are using -T html with more than one input file at a time, and hence
the bug doesn't really bite in practically relevant use cases.

Yours,
  Ingo


> Index: main.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/mandoc/main.c,v
> retrieving revision 1.247
> diff -u -p -r1.247 main.c
> --- main.c 24 Feb 2020 21:15:05 -0000 1.247
> +++ main.c 11 Jun 2020 15:06:43 -0000
> @@ -872,7 +872,7 @@ parse(struct mparse *mp, int fd, const c
>   if (outst->outdata == NULL)
>   outdata_alloc(outst, outconf);
>   else if (outst->outtype == OUTT_HTML)
> - html_reset(outst);
> + html_reset(outst->outdata);
>  
>   mandoc_xr_reset();
>   meta = mparse_result(mp);
>

Reply | Threaded
Open this post in threaded view
|

Re: man double free

Theo de Raadt-2
In reply to this post by Otto Moerbeek
Otto Moerbeek <[hidden email]> wrote:

> On Thu, Jun 11, 2020 at 05:15:28PM +0200, Romero Pérez, Abel wrote:
>
> >
> >
> > On 2020-06-11 17:07, Otto Moerbeek wrote:
> > > On Thu, Jun 11, 2020 at 04:53:25PM +0200, Romero Pérez, Abel wrote:
> > >
> > > >
> > > >
> > > > On 2020-06-11 16:45, Klemens Nanni wrote:
> > > > > On Thu, Jun 11, 2020 at 03:59:09PM +0200, Otto Moerbeek wrote:
> > > > > > This already trips the bug;
> > > > > >
> > > > > > man -T html -c pfctl id
> > > > > >
> > > > > > No need for a custom man function. No clue yet why.
> > > > > This is in mandoc's HTML parser, but only happens for multiple manuals
> > > > > in html.c:html_reset_internal():
> > > > >
> > > > > 164             while ((tag = h->tag) != NULL) {
> > > > > 165                     h->tag = tag->next;
> > > > > 166                     free(tag);
> > > > > 167             }
> > > > >
> > > > > Note that it crashes differently depending on the optimization level:
> > > > >
> > > > > $ cd /usr/src/usr.bin/mandoc
> > > > > $ make DEBUG=-O0
> > > > > $ ./obj/mandoc -Thtml `man -w id cat` >/dev/null ; echo $?
> > > > > 0
> > > > >
> > > > > $ make DEBUG=-O1
> > > > > $ ./obj/mandoc -Thtml `man -w id cat` >/dev/null
> > > > > Segmentation fault (core dumped)
> > > > >
> > > > > $ make DEBUG=-O2
> > > > > $ ./obj/mandoc -Thtml `man -w id cat` >/dev/null
> > > > > mandoc(32092) in free(): bogus pointer (double free?) 0x6641bab613b
> > > > > Abort trap (core dumped)
> > > > >
> > > > > Need to run now, but wanted to share what seems to be the right direction.
> > > > >
> > > > Compile with -O0 to fix temporally the bug.
> > > > But, I also want to note that a binary is not need to be specified, can be a
> > > > just a file... (as second man entry).
> > > >
> > >
> > > This fixes it for me,
> > >
> > > -Otto
> > >
> > > Index: main.c
> > > ===================================================================
> > > RCS file: /cvs/src/usr.bin/mandoc/main.c,v
> > > retrieving revision 1.247
> > > diff -u -p -r1.247 main.c
> > > --- main.c 24 Feb 2020 21:15:05 -0000 1.247
> > > +++ main.c 11 Jun 2020 15:06:43 -0000
> > > @@ -872,7 +872,7 @@ parse(struct mparse *mp, int fd, const c
> > >   if (outst->outdata == NULL)
> > >   outdata_alloc(outst, outconf);
> > >   else if (outst->outtype == OUTT_HTML)
> > > - html_reset(outst);
> > > + html_reset(outst->outdata);
> > >   mandoc_xr_reset();
> > >   meta = mparse_result(mp);
> > >
> > Only one comment, don't use -O0 flag as optimization (disabled) to hunt more
> > bugs of this kind.
>
> I have no clue what you mean by above sentence. If code has a bug,
> optmization level might cause the bug to be hidden or exposed; it can
> work both ways.

The person who didn't fix the bug is giving you advice about fixing the bug.

Reply | Threaded
Open this post in threaded view
|

Re: man double free

Abel Romero Prez
Yes, thank you.

I suggest only to have a look into better measures of security by
researching optimization flags, to find an equilibrium of optimization
and security.

But I said to forget it because it was hard to explain.

On 2020-06-11 18:06, Theo de Raadt wrote:

> Otto Moerbeek <[hidden email]> wrote:
>
>> On Thu, Jun 11, 2020 at 05:15:28PM +0200, Romero Pérez, Abel wrote:
>>
>>>
>>>
>>> On 2020-06-11 17:07, Otto Moerbeek wrote:
>>>> On Thu, Jun 11, 2020 at 04:53:25PM +0200, Romero Pérez, Abel wrote:
>>>>
>>>>>
>>>>>
>>>>> On 2020-06-11 16:45, Klemens Nanni wrote:
>>>>>> On Thu, Jun 11, 2020 at 03:59:09PM +0200, Otto Moerbeek wrote:
>>>>>>> This already trips the bug;
>>>>>>>
>>>>>>> man -T html -c pfctl id
>>>>>>>
>>>>>>> No need for a custom man function. No clue yet why.
>>>>>> This is in mandoc's HTML parser, but only happens for multiple manuals
>>>>>> in html.c:html_reset_internal():
>>>>>>
>>>>>> 164             while ((tag = h->tag) != NULL) {
>>>>>> 165                     h->tag = tag->next;
>>>>>> 166                     free(tag);
>>>>>> 167             }
>>>>>>
>>>>>> Note that it crashes differently depending on the optimization level:
>>>>>>
>>>>>> $ cd /usr/src/usr.bin/mandoc
>>>>>> $ make DEBUG=-O0
>>>>>> $ ./obj/mandoc -Thtml `man -w id cat` >/dev/null ; echo $?
>>>>>> 0
>>>>>>
>>>>>> $ make DEBUG=-O1
>>>>>> $ ./obj/mandoc -Thtml `man -w id cat` >/dev/null
>>>>>> Segmentation fault (core dumped)
>>>>>>
>>>>>> $ make DEBUG=-O2
>>>>>> $ ./obj/mandoc -Thtml `man -w id cat` >/dev/null
>>>>>> mandoc(32092) in free(): bogus pointer (double free?) 0x6641bab613b
>>>>>> Abort trap (core dumped)
>>>>>>
>>>>>> Need to run now, but wanted to share what seems to be the right direction.
>>>>>>
>>>>> Compile with -O0 to fix temporally the bug.
>>>>> But, I also want to note that a binary is not need to be specified, can be a
>>>>> just a file... (as second man entry).
>>>>>
>>>>
>>>> This fixes it for me,
>>>>
>>>> -Otto
>>>>
>>>> Index: main.c
>>>> ===================================================================
>>>> RCS file: /cvs/src/usr.bin/mandoc/main.c,v
>>>> retrieving revision 1.247
>>>> diff -u -p -r1.247 main.c
>>>> --- main.c 24 Feb 2020 21:15:05 -0000 1.247
>>>> +++ main.c 11 Jun 2020 15:06:43 -0000
>>>> @@ -872,7 +872,7 @@ parse(struct mparse *mp, int fd, const c
>>>>     if (outst->outdata == NULL)
>>>>     outdata_alloc(outst, outconf);
>>>>     else if (outst->outtype == OUTT_HTML)
>>>> - html_reset(outst);
>>>> + html_reset(outst->outdata);
>>>>     mandoc_xr_reset();
>>>>     meta = mparse_result(mp);
>>>>
>>> Only one comment, don't use -O0 flag as optimization (disabled) to hunt more
>>> bugs of this kind.
>>
>> I have no clue what you mean by above sentence. If code has a bug,
>> optmization level might cause the bug to be hidden or exposed; it can
>> work both ways.
>
> The person who didn't fix the bug is giving you advice about fixing the bug.
>

Reply | Threaded
Open this post in threaded view
|

Re: man double free

Theo de Raadt-2
Romero Pérez, Abel <[hidden email]> wrote:

> Yes, thank you.
>
> I suggest only to have a look into better measures of security by
> researching optimization flags, to find an equilibrium of optimization
> and security.

Romero, that is bullshit.

Reply | Threaded
Open this post in threaded view
|

Re: man double free

Klemens Nanni-2
In reply to this post by Otto Moerbeek
On Thu, Jun 11, 2020 at 05:07:17PM +0200, Otto Moerbeek wrote:
> This fixes it for me,
This looks like a simple mistake introduced back in main.c r1.222:

        date: 2019/03/03 13:01:47;  author: schwarze;  state: Exp;  lines: +3 -1;
        Reset HTML formatter state, in particular the id_unique hash,
        after processing each manual page, such that the next page
        starts from a clean state and doesn't continue suffix numbering.

        Issue found while looking at https://github.com/Debian/debiman/issues/48
        which was brought up by Orestis Ioannou <oorestisime at github>.

outst is on the stack and html_reset_internal() expects a struct html
pointer, but this obviously mismatches and eventually free()s stack
memory.

820         if (outst->had_output && outst->outtype <= OUTT_UTF8) {
821                 if (outst->outdata == NULL)
822                         outdata_alloc(outst, &conf->output);
823                 terminal_sepline(outst->outdata);
824         }  
826         if (resp->form == FORM_SRC)                                                                                                                                          
827                 parse(mp, fd, resp->file, outst, &conf->output);                                                                                                                          
828         else {  

outdata_alloc() properly allocates a struct html with html_alloc() in
our case which must be reset later in parse() through html_reset().

Pretty sure your diff is correct, but won't hurt to hear from Ingo
before committing.

OK kn

Reply | Threaded
Open this post in threaded view
|

Re: man double free

Klemens Nanni-2
In reply to this post by Ingo Schwarze
On Thu, Jun 11, 2020 at 06:03:20PM +0200, Ingo Schwarze wrote:
> Indeed, i came up with exactly the same diff and had already
> written this commit message, but was still testing:
>
>   Fix a regression in rev. 1.238 (2019/07/26):
>   Pass the right object to html_reset() or it will crash
>   when rendering more than one manual page to HTML in a row.
>   Bug reported by Abel Romero Perez <romeroperezabel at gmail dot com>.
Wasn't this just reshuffling the already existing html_reset() which
was introduced in r1.222?

Reply | Threaded
Open this post in threaded view
|

Re: man double free

Ingo Schwarze
In reply to this post by Theo de Raadt-2
Hi,

Theo de Raadt wrote on Thu, Jun 11, 2020 at 10:12:47AM -0600:
> Romero Perez, Abel <[hidden email]> wrote:

>> I suggest only to have a look into better measures of security by
>> researching optimization flags, to find an equilibrium of optimization
>> and security.

> Romero, that is bullshit.

However, there is something i ought to do to make such bugs less
likely: Remove the last vestigial type-unsafe pointer handling.
That was designed a decade ago with an excessive focus on flexibility
when the scope of the program was not yet clear.  A typical example
of over-abstraction.  When you don't know yet how general your code
might need to be, write specific code first.  If it turns out
additional situations need to be handled, consider generalizing it
(and again, don't go overboard).  Never invent abstractions "because
just in case".

If we would need many dozens of different output formats, and people
would want to plug in new ones at run time or something crazy like
that, the abstraction implemented with these void pointers might
have a point.  But now that we know that less than a dozen output
formats are really needed, and that they are all very stable, there
are very likely ways to improve this code, making it more robust
and less error-prone.

Yours,
  Ingo

Reply | Threaded
Open this post in threaded view
|

Re: man double free

Theo de Raadt-2
Ingo Schwarze <[hidden email]> wrote:

> Hi,
>
> Theo de Raadt wrote on Thu, Jun 11, 2020 at 10:12:47AM -0600:
> > Romero Perez, Abel <[hidden email]> wrote:
>
> >> I suggest only to have a look into better measures of security by
> >> researching optimization flags, to find an equilibrium of optimization
> >> and security.
>
> > Romero, that is bullshit.
>
> However, there is something i ought to do to make such bugs less
> likely: Remove the last vestigial type-unsafe pointer handling.
> That was designed a decade ago with an excessive focus on flexibility
> when the scope of the program was not yet clear.  A typical example
> of over-abstraction.  When you don't know yet how general your code
> might need to be, write specific code first.  If it turns out
> additional situations need to be handled, consider generalizing it
> (and again, don't go overboard).  Never invent abstractions "because
> just in case".
>
> If we would need many dozens of different output formats, and people
> would want to plug in new ones at run time or something crazy like
> that, the abstraction implemented with these void pointers might
> have a point.  But now that we know that less than a dozen output
> formats are really needed, and that they are all very stable, there
> are very likely ways to improve this code, making it more robust
> and less error-prone.

No way Ingo, you should be carefully use the compiler -O option!!!!!
It is the way to security, expert Romero has spoken!

Reply | Threaded
Open this post in threaded view
|

Re: man double free

Ingo Schwarze
In reply to this post by Klemens Nanni-2
Hi Klemens,

Klemens Nanni wrote on Thu, Jun 11, 2020 at 06:32:46PM +0200:
> On Thu, Jun 11, 2020 at 06:03:20PM +0200, Ingo Schwarze wrote:

>> Indeed, i came up with exactly the same diff and had already
>> written this commit message, but was still testing:
>>
>>   Fix a regression in rev. 1.238 (2019/07/26):
>>   Pass the right object to html_reset() or it will crash
>>   when rendering more than one manual page to HTML in a row.
>>   Bug reported by Abel Romero Perez <romeroperezabel at gmail dot com>.

> Wasn't this just reshuffling the already existing html_reset() which
> was introduced in r1.222?

Oh right, i only looked far enough to see that the argument
of html_reset() changed in that commit, but you are right that
the previous argument (curp) wasn't correct either.  So it seems
it was even broken a few months longer.  No matter, now at least
it has the right argument.

Thanks for having another look,
  Ingo