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
Created attachment 130797 [details] Forgot the attachment
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?
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.
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.
Created attachment 131038 [details] Fixes the wordproc.c segfault. Buffer overflow, the upstream code misses upper boundary checks in loops in wordproc.c.
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?
Yes, otherwise the return status is undefined, thanks.
Created attachment 131043 [details] Fixes the wordproc.c segfault. The correct patch.
Does this overflow have any security implications? Could you have the security team take a look at it?
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.
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.
Mark, Josh, I'm adding you to the Cc list just to make sure you know that something happened in mcedit.
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; }
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.
Ok. I've checked in your fix too. I'am too tired now - going to bed.
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.
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.
Fixed in the released FC update.