[wdiff-bugs] wdiff pager handling

[Martin von Gagern]

Hi!

For the record, I'm forwarding a conversation I had with Santiago Vila,
the Debian wdiff maintainer. It has lead to the improved auto-pager
features of todays 0.6.1 release.

Greetings,
 Martin von Gagern


-------- Original Message --------
Subject: Re: wdiff 0.6.0 released
Date: Tue, 30 Mar 2010 17:03:29 +0200
From: Martin von Gagern <address@hidden>
To: Santiago Vila <address@hidden>

On 30.03.2010 16:16, Santiago Vila wrote:
> On Tue, 30 Mar 2010, Martin von Gagern wrote:
> 
>> - http://patch-tracker.debian.org/patch/series/view/wdiff/0.5-21/04
>> Unfortunately the Bug-Debian link given here seems unavailable, so I
>> have to guess about the motivation. Is this patch aiming for the case
>> where users have a pager which is a symlink to less, but doesn't contain
>> less in its file name?
> 
> Yes, exactly. This is from sensible-pager(1) in a Debian system:
> 
>        sensible-editor,  sensible-pager  and  sensible-browser  make  sensible
>        decisions  on  which  editor,  pager,  and web browser to call, respec-
>        tively.  Programs in Debian can use these scripts as their default edi-
>        tor, pager, or web browser or emulate their behavior.
> 
> This is the way manpages are shown using "less" if it's available, or "more"
> if it is not, automatically.

If the manpage is correct, sensible-pager is a script, not a symlink. So
if people have PAGER=sensible-pager this won't work. On the other hand,
at least on the Ubuntu I currently have access to, sensible-pager does
evaluate the PAGER environment variable, so if you set
PAGER=sensible-pager, you will probably end up in an infinite loop.

However, the default behaviour of sensible-pager in the absence of PAGER
seems more like the thing: it defaults to pager, where /usr/bin/pager
symlinks to /etc/alternatives/pager symlinks to /usr/bin/less. So yes,
if people have PAGER=/usr/bin/pager, then this patch might help. If, on
the other hand, users were to set PAGER=pager, I somehow doubt that
things would work, as realpath(3) doesn't seem to care about the PATH
environment variable.

Now that I think about it, this might even be a security problem: if
some user has e.g. PAGER=less, and some attacker tricks him into
wdiffing something in a directory where less is a script created by the
attacker for this occasion, then the original wdiff will still execvp
the less program available on the PATH, whereas the patched wdiff will
convert less to an absolute path in the current working directory and
execute that instead, thereby executing a script written by the attacker
with the privileges of the victim. Bad idea! Certainly worth a security
fix, and maybe even a CVE. Never filed a CVE before. Do you know the
appropriate procedure for this? Or do you believe this can be passed to
Debian and all its derivatives without such a central report?

I'm posting this to you personally and using encryption, so that you can
address the problem in a way suitable for security issues. Once the
fixes are out, I'll probably forward a copy to the wdiff-bugs mailing
list, for the record.

On the whole, I already consider the inspection of PAGER to detect less
to be a somewhat ugly heuristics. As debian users can set
PAGER=/usr/bin/less or pass --less-mode (-l) to wdiff, having this
autodetection is not essential. So until someone comes up with a patch
that is both secure and keeps the footprint reasonably down, I don't
think I'll address this oficially.

>> Considering it for inclusion, although I hate the concept of PATH_MAX.
> 
> The PATH_MAX thing is just how Charles C. Fu implemented it, but you
> are of course free to rewrite it in a way that is more suitable for a
> GNU package (I remember the days when many Debian packages failed to build
> on the Hurd because of PATH_MAX not being defined there).

Yes, I'd have to let realpath allocate the buffer. Should be feasible.
Unfortunately I don't see a variant of realpath that does take the PATH
into account, and dealing with the PATH myself feels like too much
trouble for little gain.

> As wdiff 0.6.0 is the first release where manpage, behaviour and
> output --help matches at each other, I would prefer to keep -v.

FYI: manpage is generated from --help using help2man, so unless someone
garbled timestamps during the release process, these two will always agree.

Greetings,
 Martin


-------- Original Message --------
Subject: Re: wdiff 0.6.0 released
Date: Tue, 30 Mar 2010 17:17:29 +0200
From: Martin von Gagern <address@hidden>
To: Santiago Vila <address@hidden>

On 30.03.2010 17:03, Martin von Gagern wrote:
> Now that I think about it, this might even be a security problem: if
> some user has e.g. PAGER=less, and some attacker tricks him into
> wdiffing something in a directory where less is a script created by the
> attacker for this occasion, then the original wdiff will still execvp
> the less program available on the PATH, whereas the patched wdiff will
> convert less to an absolute path in the current working directory and
> execute that instead, thereby executing a script written by the attacker
> with the privileges of the victim. Bad idea! Certainly worth a security
> fix, and maybe even a CVE. Never filed a CVE before. Do you know the
> appropriate procedure for this? Or do you believe this can be passed to
> Debian and all its derivatives without such a central report?

I might have been overreacting here: realpath is only applied to the
PAGER_PROGRAM compiled into wdiff, not to the PAGER environment
variable. The default build for 0.6.0 doesn't even set PAGER_PROGRAM.

Would I be correct to assume that Debian builds either set this macro,
or that the patch is completely without effect? Judging from
wdiff_0.5-21.debian.tar.gz I'd assume the latter.

Nevertheless, if anyone should accidentially #define PAGER_PROGRAM
"less" at compile-time, this could cause trouble, so even if Debian is
on the safe side, applying the patch in general might call for trouble.

Greetings,
 Martin

signature.asc
Description: OpenPGP digital signature