Bug#984615: xterm: bug in CVE-2021-27135 patch in at least stretch
Awesome, thank you for the confirmation. I've rolled out the announcement and published the website update. Thanks, everyone! \o/ - u
Bug#984615: xterm: bug in CVE-2021-27135 patch in at least stretch
Utkarsh Gupta dixit: >Thanks to Thomas for his help, I've uploaded a fix for this regression >(by reverting the backport of that part of the patch which was not >necessary It’s got some memory impact, but probably neglegible here, true. > for this CVE fix). And thanks to Thorsten for his >comprehensive bug report and to Sven for further debugging and taking >a look. You’re welcome. >Thorsten, could you please test the latest upload and see if >everything works alright for you? I don’t actally have a testcase, I’ve just noticed that this is wrong when trying to backport the patch further myself. But it won’t hit the bug now. bye, //mirabilos -- „Cool, /usr/share/doc/mksh/examples/uhr.gz ist ja ein Grund, mksh auf jedem System zu installieren.“ -- XTaran auf der OpenRheinRuhr, ganz begeistert (EN: “[…]uhr.gz is a reason to install mksh on every system.”)
Bug#984615: xterm: bug in CVE-2021-27135 patch in at least stretch
On Sat, Mar 06, 2021 at 06:46:25PM +0100, Sven Joachim wrote: ... > Run xterm under valgrind and select some text. Valgrind will be very > unhappy with xterm 327-2+deb9u1 but should not show up any errors in valgrind usually has something to say, but (noting that I'm only interested in what it says when I configure --with-valgrind(*)), I get a report of ~5000 lines using these options OPTS="-v \ --num-callers=10 \ --error-limit=no \ --show-reachable=yes \ --leak-resolution=high \ --track-origins=yes \ --leak-check=yes \ --show-reachable=yes" ...and almost all of that is stuff that I can't fix without adding interfaces in X11, Xt and Xaw. (*) asan2 also has things to say, but most of that is not useful without a complete set of debug-libraries (again, X11/Xt/Xaw). -- Thomas E. Dickey https://invisible-island.net ftp://ftp.invisible-island.net signature.asc Description: PGP signature
Bug#984615: xterm: bug in CVE-2021-27135 patch in at least stretch
On Sat, Mar 06, 2021 at 06:07:43PM +, Thorsten Glaser wrote: > Sven Joachim dixit: > > >I see that this might be a problem (albeit unlikely to happen in > >practice), however I have trouble understanding exactly where a > >use-after-realloc bug comes into play. Maybe Thorsten can help me fix > >my blindness? > > The next time something is selected, the code a little further > up will check if the allocated size is sufficient, and, if so, > use screen->selection_data which was the pre-realloc address of > line. > > >> I am glad and surprised that sid is okay and there doesn't seem to be > > The code in sid completely differs (structures, variable names, etc). The renaming (selection_size) comes from patch #338, which looks like this item: Patch #338 - 2018/12/09 * amend solution for Debian #758633 to ensure that replies for bracketed paste are not sent while processing a selection for exec-formatted (Debian #913237). > >suggestion you could also apply the patches to the SaltTextAway() > >function from xterm 365e. > > If 365e is like 366 (currently in sid), you’ll have lots of fun due > to the renamed everything. 366 is current. I have some changes for 367 which I'll put out after seeing what I can do to improve performance with fwvm active-icon. > I’d rather Tom changed xterm upstream to address the realloc-failure > difference. I know he reads Debian bugreports ;-) and he’s really > busy so probably takes longer to respond. it used to be the case that downstream would ask my opinion on patches like this -- it's been a while since anyone did -- Thomas E. Dickey https://invisible-island.net ftp://ftp.invisible-island.net signature.asc Description: PGP signature
Bug#984615: xterm: bug in CVE-2021-27135 patch in at least stretch
Sven Joachim dixit: >I see that this might be a problem (albeit unlikely to happen in >practice), however I have trouble understanding exactly where a >use-after-realloc bug comes into play. Maybe Thorsten can help me fix >my blindness? The next time something is selected, the code a little further up will check if the allocated size is sufficient, and, if so, use screen->selection_data which was the pre-realloc address of line. >> I am glad and surprised that sid is okay and there doesn't seem to be The code in sid completely differs (structures, variable names, etc). >suggestion you could also apply the patches to the SaltTextAway() >function from xterm 365e. If 365e is like 366 (currently in sid), you’ll have lots of fun due to the renamed everything. I’d rather Tom changed xterm upstream to address the realloc-failure difference. I know he reads Debian bugreports ;-) and he’s really busy so probably takes longer to respond. bye, //mirabilos -- >> Why don't you use JavaScript? I also don't like enabling JavaScript in > Because I use lynx as browser. +1 -- Octavio Alvarez, me and ⡍⠁⠗⠊⠕ (Mario Lang) on debian-devel
Bug#984615: xterm: bug in CVE-2021-27135 patch in at least stretch
On 2021-03-06 02:49 +0530, Utkarsh Gupta wrote: > Hi Thorsten > > On Sat, Mar 6, 2021 at 2:25 AM Thorsten Glaser wrote: >> debian/patches/CVE-2021-27135.patch changes button.c line (after >> patching) 3747 to: >> >>line = realloc(line, screen->selection_size); >> >> But “line” is a local variable, the address of the buffer must >> be stored in the one handed out, too, so please change this to: >> >> if ((have * 2) < (size_t) j) { >> Char *next = realloc(line, have + 1); >> if (next) { >> screen->selection_data = line = next; >> screen->selection_size = have + 1; >> } >> } >> >> This also deals properly with realloc failures (since we’re >> shrinking, ignore them and just keep the older, larger area). I see that this might be a problem (albeit unlikely to happen in practice), however I have trouble understanding exactly where a use-after-realloc bug comes into play. Maybe Thorsten can help me fix my blindness? > Thanks for the very comprehensive bug report and for the patch as well! > >> I’ve not looked at jessie-ELTS or buster-security whether they >> are affected as well; sid is clean (and where I got the realloc >> failure check necessity from, although sid’s free()s the buffer >> if realloc fails; this isn’t needed @Tom). > > If this seems to be happening in stretch, I assume there's a problem > with jessie-ELTS as well. That said, buster is good because a DSA > wasn't issued and this will be fixed via a point release. I had already prepared an update for buster, but fortunately it did not happen yet, because that one also has the same bug as yours. > I am glad and surprised that sid is okay and there doesn't seem to be > a problem. Just to cross-check and ensure I get it right (for stretch > and jessie), can you send me the reproducer as well? I'd like to be > able to reproduce this before and after your patch (just to be one the > safer side) and do the same for jessie as well! Run xterm under valgrind and select some text. Valgrind will be very unhappy with xterm 327-2+deb9u1 but should not show up any errors in that regard with a correctly patched version. Instead of Thorsten's suggestion you could also apply the patches to the SaltTextAway() function from xterm 365e. Cheers, Sven
Bug#984615: xterm: bug in CVE-2021-27135 patch in at least stretch
Hi Thorsten On Sat, Mar 6, 2021 at 2:25 AM Thorsten Glaser wrote: > debian/patches/CVE-2021-27135.patch changes button.c line (after > patching) 3747 to: > >line = realloc(line, screen->selection_size); > > But “line” is a local variable, the address of the buffer must > be stored in the one handed out, too, so please change this to: > > if ((have * 2) < (size_t) j) { > Char *next = realloc(line, have + 1); > if (next) { > screen->selection_data = line = next; > screen->selection_size = have + 1; > } > } > > This also deals properly with realloc failures (since we’re > shrinking, ignore them and just keep the older, larger area). Thanks for the very comprehensive bug report and for the patch as well! > I’ve not looked at jessie-ELTS or buster-security whether they > are affected as well; sid is clean (and where I got the realloc > failure check necessity from, although sid’s free()s the buffer > if realloc fails; this isn’t needed @Tom). If this seems to be happening in stretch, I assume there's a problem with jessie-ELTS as well. That said, buster is good because a DSA wasn't issued and this will be fixed via a point release. I am glad and surprised that sid is okay and there doesn't seem to be a problem. Just to cross-check and ensure I get it right (for stretch and jessie), can you send me the reproducer as well? I'd like to be able to reproduce this before and after your patch (just to be one the safer side) and do the same for jessie as well! Thanks, again, for such a detailed bug report! :D - u
Bug#984615: xterm: bug in CVE-2021-27135 patch in at least stretch
Source: xterm Version: 327-2+deb9u1 Severity: serious Justification: introduces use-after-realloc debian/patches/CVE-2021-27135.patch changes button.c line (after patching) 3747 to: line = realloc(line, screen->selection_size); But “line” is a local variable, the address of the buffer must be stored in the one handed out, too, so please change this to: if ((have * 2) < (size_t) j) { Char *next = realloc(line, have + 1); if (next) { screen->selection_data = line = next; screen->selection_size = have + 1; } } This also deals properly with realloc failures (since we’re shrinking, ignore them and just keep the older, larger area). I’ve not looked at jessie-ELTS or buster-security whether they are affected as well; sid is clean (and where I got the realloc failure check necessity from, although sid’s free()s the buffer if realloc fails; this isn’t needed @Tom). bye, //mirabilos -- you introduced a merge commit│ % g rebase -i HEAD^^ sorry, no idea and rebasing just fscked │ Segmentation should have cloned into a clean repo │ fault (core dumped) if I rebase that now, it's really ugh │ wuahh