Bug 194562 - Crash when formatting paragraph
Crash when formatting paragraph
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: mc (Show other bugs)
5
i386 Linux
medium Severity medium
: ---
: ---
Assigned To: Jindrich Novy
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2006-06-14 00:32 EDT by Nickolay V. Shmyrev
Modified: 2013-07-02 19:16 EDT (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-07-13 07:18:28 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
Forgot the attachment (9.99 KB, application/x-gzip)
2006-06-14 00:35 EDT, Nickolay V. Shmyrev
no flags Details
Correct testcase (53.01 KB, application/x-gzip)
2006-06-14 10:12 EDT, Nickolay V. Shmyrev
no flags Details
Fixes the wordproc.c segfault. (1.07 KB, patch)
2006-06-16 08:15 EDT, Jindrich Novy
no flags Details | Diff
Fixes the wordproc.c segfault. (1.11 KB, patch)
2006-06-16 09:49 EDT, Jindrich Novy
no flags Details | Diff
Rewritten next_word_start(). (1.42 KB, patch)
2006-06-16 12:11 EDT, Jindrich Novy
no flags Details | Diff

  None (edit)
Description Nickolay V. Shmyrev 2006-06-14 00:32:43 EDT
With Fedora's 5 stock mc-4.6.1a rpm I have a following crash when formatting
paragraph.

(gdb) bt
#0 next_word_start (t=0x81fc138, q=Variable "q" is not available.
) at wordproc.c:227语言编写的插件,Gimp 还能够使用脚本。大部分现有的脚本都是用
S#1 0x080a975d in format_paragraph (edit=0x81ea598, force=1) at wordproc.c:280
#2 0x0809a035 in edit_execute_cmd (edit=0x81ea598, command=416,
char_for_insertion=4294967295) at edit.c:2666
#3 0x0809aa05 in edit_execute_key_command (edit=0x81ea598, command=416,
char_for_insertion=4294967295) at edit.c:2250
#4 0x080a268b in edit_callback (w=0x81ea598, msg=WIDGET_KEY, parm=8304)
at editwidget.c:362 4Замена 5Копия 6Перемес7Поиск 8Удалить9МенюMC 10Вы
#5 0x080599fb in dlg_process_event (h=0x81ef0a8, key=112, event=0xbff84d70)
at dialog.c:659
#6 0x08059d0d in run_dlg (h=0x81ef0a8) at dialog.c:785
#7 0x080a22a4 in edit_file (_file=0x81b0850 "concepts.xml.save", line=0)
at editwidget.c:225
#8 0x0806fe7b in do_nc () at main.c:1757
#9 0x08070855 in main (argc=Cannot access memory at address 0x0
) at main.c:2293
#10 0x00af77e4 in __libc_start_main () from /lib/libc.so.6
#11 0x0804d481 in _start ()

The file that causes the crash. You have to put cursor on last in the text
(line 2303 for example) and press Alt+p.

Was submitted to mc bugzilla also 

http://savannah.gnu.org/bugs/?func=detailitem&item_id=16829
Comment 1 Nickolay V. Shmyrev 2006-06-14 00:35:47 EDT
Created attachment 130797 [details]
Forgot the attachment
Comment 2 Jindrich Novy 2006-06-14 10:01:53 EDT
I need to ask more information to reproduce it. I extracted the attachment, but
there's no line number 2303 since it contains only 391 lines of XML. I looked at
the savannah bug where you say that you put cursor on the last character at line
starting with <para lang="ru"> and press alt-p. Then you see the crash.

I tried to reproduce on the last <para lang="ru"> tag in the file without the
segfault so I tried it several times at random positions getting the text
reorganized to a paragraph, not fully correctly, but without a segfault. To be
honest I tried to reproduce it in the rawhide mc.

Could you point me to a particular line/position to reproduce it?
Comment 3 Nickolay V. Shmyrev 2006-06-14 10:12:23 EDT
Created attachment 130852 [details]
Correct testcase

Oh, sorry, it was wrong file. Here is mine, ungzip if first. The line number is
the same - 2303. Another problem with it - if I am trying to reproduce the
crash under one user it works fine. It crashes only under root.
Comment 4 Jindrich Novy 2006-06-14 10:36:05 EDT
Ok, reproduced that after some effort in the word "также" having cursor right
before "ж". The user under which you are running mc shouldn't affect a buffer
overrun likely causes the segfault.
Comment 5 Jindrich Novy 2006-06-16 08:15:54 EDT
Created attachment 131038 [details]
Fixes the wordproc.c segfault.

Buffer overflow, the upstream code misses upper boundary checks in loops in
wordproc.c.
Comment 6 Leonard den Ottolander 2006-06-16 09:44:30 EDT
Don't we need an added return -1 in next_word_start() after the loop in case it
is terminated due to i being equal to size?
Comment 7 Jindrich Novy 2006-06-16 09:48:51 EDT
Yes, otherwise the return status is undefined, thanks.
Comment 8 Jindrich Novy 2006-06-16 09:49:55 EDT
Created attachment 131043 [details]
Fixes the wordproc.c segfault.

The correct patch.
Comment 9 Leonard den Ottolander 2006-06-16 10:17:22 EDT
Does this overflow have any security implications? Could you have the security
team take a look at it?
Comment 10 Pavel Tsekov 2006-06-16 11:47:54 EDT
What about the inner for loop in next_word_start() ? Btw I don't like the whole 
loop in a loop setup in next_word_start() - it seems quite unnecessary.
Comment 11 Jindrich Novy 2006-06-16 12:11:36 EDT
Created attachment 131054 [details]
Rewritten next_word_start().

Pavel, yes to rewrite it seems to be the right way to follow. It actually makes
it much more comprehensive (and hopefully bugless).

Leonard, I'll Cc security guys here, but IMO the security impact is pretty
minimal, as alt-p isn't used very frequently by the mc people.
Comment 12 Jindrich Novy 2006-06-16 12:15:53 EDT
Mark, Josh, I'm adding you to the Cc list just to make sure you know that
something happened in mcedit.
Comment 13 Leonard den Ottolander 2006-06-16 14:36:32 EDT
Indeed we missed the test i < size in the inner loop.

However, current patch is not good. You forgot that we first want to see a
space. It should be something like:

next_word_start (unsigned char *t, int q, int size)
{
    int i;
    int havespace = 0;
    for (i = q; i < size; i++) {
    	if ( t[i] == '\n')
    	    return -1;
	if ( t[i] == ' ' || t[i] == '\t')
	    havespace = 1;
    	else if (havespace == 1)
    	    return i;
     }
    return -1;
}
Comment 14 Pavel Tsekov 2006-06-16 15:32:25 EDT
I've checked in a variation of Jindrich's patch:

http://cvs.savannah.gnu.org/viewcvs/mc/mc/edit/wordproc.c?sortby=date&r2=1.
21&r1=1.20&diff_format=u

Leonard, I saw your comment just after commiting the patch.
Comment 15 Pavel Tsekov 2006-06-16 15:54:19 EDT
Ok. I've checked in your fix too. I'am too tired now - going to bed.
Comment 16 Josh Bressers 2006-06-16 17:30:51 EDT
I don't this should be considered a security issue.  I did some investigation
regarding this issue today, it seems to be an OOB read that's crashing mcedit. 
The for loop keeps incrementing i, then reading the value of t[i], which
eventually (i = 34970 for example) causes an OOB read error and crashes mcedit.

Since mcedit is a user run program, and the user has to hit alt-p themselves,
this strikes me as one of those "don't do that" bugs.

I'm going to consider this not a secuirty issue unless someone can tell me I'm
wrong.

Thanks.
Comment 17 Leonard den Ottolander 2006-06-17 08:25:19 EDT
Josh, I suppose you are right. I hadn't considered the fact that this is a read
loop, not a write loop. Thanks for taking a look.
Comment 18 Jindrich Novy 2006-07-13 07:18:28 EDT
Fixed in the released FC update.

Note You need to log in before you can comment on or make changes to this bug.