remote shell panel confused by filenames with '%'

[yury_t]

When opening remote dir in remote shell panel, file with names containing '%' are shown as errors and inaccessible (red highlights with leading '?').

[slavazanko]

I confirm the bug.

Steps to reproduce:
1) touch /tmp/%p.test.txt
2) start mc and type: cd ​sh://127.0.0.1/tmp

Expected result: should be visible the '%d.test.txt' file in list of files
Actual results: the '%p' part of file name was substituted by 0. In my case I see: 0.test.txt

[andrew_b]

Ticket #3143 has been marked as a duplicate of this ticket.

[ossi]

as pointed out in the duplicate (whoops :}), this problem poses a target for format string attacks: while messing up the display with %p is pretty harmless, %n has been used to create exploits before. this makes the VFS unsuitable for browsing any untrusted data, which includes directories of other users on otherwise completely trusted machines.

[andrew_b]

Patch of Dieter Werner

[andrew_b]

Dieter Werner <d_werner gmx/net> sent me the patch-ls-ub.diff​ patch in private e-mail. Please review and check it.

[ossi]

fwiw, that it is in fact already in the perl code makes it less critical than i expected (an %n attack attempt will just yield a warning). i wonder how many similar bugs exist, though ...

[andrew_b]

Branch: 2983_fish_filename_percent
changeset:4ceb42013925a4b264b5f3bd04682d402063084d

[andrew_b]

Merged to master: [4ceb42013925a4b264b5f3bd04682d402063084d]

[dnh]

Replying to andrew_b:

Merged to master: [4ceb42013925a4b264b5f3bd04682d402063084d]

Generally: NO variable at all should ever be part of the format string of
any printf or similar (strftime) formatting function EVER. It's the same
thing as variables in SQL-statements vs. prepared statements.

As fefe stated recently: if you're checking for exploits, you start with
grepping for 'system', 'printf', etc ... We should try to do that, too.

I'll add a attachment against master.

[dnh]

vfs_fish_helpers_ls-percent.diff

[andrew_b]

Replying to dnh:

Replying to andrew_b:

Merged to master: [4ceb42013925a4b264b5f3bd04682d402063084d]

Generally: NO variable at all should ever be part of the format string of
any printf or similar (strftime) formatting function EVER.

Ok. The same issue is in get:22.

I'll add a attachment against master.

Is "%ld" instead of "%i" should be used for "$size"?

[dnh]

Generally: NO variable at all should ever be part of the format string of
any printf or similar (strftime) formatting function EVER.

Ok. The same issue is in get:22.

"get:" ? What's that? ;)

I'll add a attachment against master.

Is "%ld" instead of "%i" should be used for "$size"?

"%ld" does not suffice for >4G sizes on 32bit platforms, IIRC. So it should be "%lld". Not sure how big off_t is everywhere ("%z" for size_t might be a candidate then). And not sure if "%lld" is supported everywhere that mc targets. Time for macros like FMTSTR_OFF_T / configure-replaceable-variables in scripts like @FMTSTR_OFF_T@ or something like that ... Not nice. I might be tempted to just try "%lld", test as much as is easy(-ily feasible) (we build mc for ARMv7l/aarch64 and ppc/ppc64 too) and then wait for bugreports ... But then, I'm not mc (code-)maintainer, just a packager ;)

[andrew_b]

Replying to dnh:

BTW: what about <​http://www.midnight-commander.org/ticket/3128> ?

I'm unable to reproduce that.

[andrew_b]

Replying to dnh:

"get:" ? What's that? ;)

This is script to get a file from remote host.

Is "%ld" instead of "%i" should be used for "$size"?

"%ld" does not suffice for >4G sizes on 32bit platforms, IIRC. So it should be "%lld".

I checked that before. "%lld" is not supported in my distro with perl-5.12.5.

[dnh]

Replying to andrew_b:

Replying to dnh:

"get:" ? What's that? ;)

This is script to get a file from remote host.

Ah.

Is "%ld" instead of "%i" should be used for "$size"?

"%ld" does not suffice for >4G sizes on 32bit platforms, IIRC. So it should be "%lld".

I checked that before. "%lld" is not supported in my distro with perl-5.12.5.

What libc? Perl itself has that for ages IIRC (5.005 even?)

$ perldoc -f sprintf
.. size

q, L, or ll interpret integer as C type "long long", "unsigned long long", or "quad" (typically 64-bit integers)

Generally, we should find out what format-string should be used for ints >32bit and figure that out in configure etc. In this special case, as the data is coming from a stat call, we can get away with a '%s' format string:

$ perl -Mstrict -we 'my $n=2**33; printf("%lld %s\n", $n, $n);'
8589934592 8589934592

Here, %i and %ld also work in perl, but this is a 64bit platform + perl.
Oh well. HTH.

[ossi]

the last comment belongs into the other ticket and should be just deleted here.

i don't know what you are making a fuss about. if the contents of a variable are strictly defined, it is perfectly reasonable to include it in the format string, and for numbers this is certainly the case. you need more than dogmatism to be convincing.

[andrew_b]

Replying to ossi:

the last comment belongs into the other ticket and should be just deleted here.

Ok. I add comment to the #3128 ticket.

[andrew_b]

Replying to dnh:

Generally: NO variable at all should ever be part of the format string of
any printf or similar (strftime) formatting function EVER.

The Perl code in the ls helper is valid (​http://www.perlmonks.org/?node_id=918845, for example). We do not expected any strange values for uid, gid and size returned from lstat().