Bug 194562
Summary: | Crash when formatting paragraph | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Nickolay V. Shmyrev <nshmyrev> | ||||||||||||
Component: | mc | Assignee: | Jindrich Novy <jnovy> | ||||||||||||
Status: | CLOSED ERRATA | QA Contact: | |||||||||||||
Severity: | medium | Docs Contact: | |||||||||||||
Priority: | medium | ||||||||||||||
Version: | 5 | CC: | bressers, leonard-rh-bugzilla, mjc, pknirsch, ptsekov | ||||||||||||
Target Milestone: | --- | ||||||||||||||
Target Release: | --- | ||||||||||||||
Hardware: | i386 | ||||||||||||||
OS: | Linux | ||||||||||||||
Whiteboard: | |||||||||||||||
Fixed In Version: | Doc Type: | Bug Fix | |||||||||||||
Doc Text: | Story Points: | --- | |||||||||||||
Clone Of: | Environment: | ||||||||||||||
Last Closed: | 2006-07-13 11:18:28 UTC | Type: | --- | ||||||||||||
Regression: | --- | Mount Type: | --- | ||||||||||||
Documentation: | --- | CRM: | |||||||||||||
Verified Versions: | Category: | --- | |||||||||||||
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |||||||||||||
Cloudforms Team: | --- | Target Upstream Version: | |||||||||||||
Embargoed: | |||||||||||||||
Attachments: |
|
Description
Nickolay V. Shmyrev
2006-06-14 04:32:43 UTC
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. |