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
