Bug 194562 - Crash when formatting paragraph
Summary: Crash when formatting paragraph
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: mc
Version: 5
Hardware: i386
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Jindrich Novy
QA Contact:
URL:
Whiteboard:
Keywords:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2006-06-14 04:32 UTC by Nickolay V. Shmyrev
Modified: 2013-07-02 23:16 UTC (History)
5 users (show)

(edit)
Clone Of:
(edit)
Last Closed: 2006-07-13 11:18:28 UTC


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

Description Nickolay V. Shmyrev 2006-06-14 04:32:43 UTC
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 04:35:47 UTC
Created attachment 130797 [details]
Forgot the attachment

Comment 2 Jindrich Novy 2006-06-14 14:01:53 UTC
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 14:12:23 UTC
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 14:36:05 UTC
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 12:15:54 UTC
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 13:44:30 UTC
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 13:48:51 UTC
Yes, otherwise the return status is undefined, thanks.

Comment 8 Jindrich Novy 2006-06-16 13:49:55 UTC
Created attachment 131043 [details]
Fixes the wordproc.c segfault.

The correct patch.

Comment 9 Leonard den Ottolander 2006-06-16 14:17:22 UTC
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 15:47:54 UTC
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 16:11:36 UTC
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 16:15:53 UTC
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 18:36:32 UTC
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 19:32:25 UTC
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 19:54:19 UTC
Ok. I've checked in your fix too. I'am too tired now - going to bed.

Comment 16 Josh Bressers 2006-06-16 21:30:51 UTC
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 12:25:19 UTC
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 11:18:28 UTC
Fixed in the released FC update.


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