Ticket #2983 (closed defect: fixed)

Opened 5 years ago

Last modified 4 years ago

remote shell panel confused by filenames with '%'

Reported by: yury_t Owned by: andrew_b
Priority: trivial Milestone: 4.8.12
Component: mc-vfs Version: 4.8.4
Keywords: Cc: dnh@…
Blocked By: Blocking:
Branch state: merged Votes for changeset: committed-master

Description

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

Attachments

patch-ls-ub.diff (795 bytes) - added by andrew_b 4 years ago.
Patch of Dieter Werner
vfs_fish_helpers_ls-percent.diff (949 bytes) - added by dnh 4 years ago.
vfs_fish_helpers_ls-percent.diff

Change History

comment:1 Changed 5 years ago by 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

comment:2 Changed 5 years ago by andrew_b

  • Component changed from mc-core to mc-vfs

comment:3 Changed 4 years ago by andrew_b

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

comment:4 Changed 4 years ago by ossi

  • Priority changed from major to critical

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.

Changed 4 years ago by andrew_b

Patch of Dieter Werner

comment:5 Changed 4 years ago by 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.

comment:6 Changed 4 years ago by ossi

  • Votes for changeset set to 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 ...

comment:7 Changed 4 years ago by andrew_b

  • Owner set to andrew_b
  • Status changed from new to accepted
  • Branch state changed from no branch to on review
  • Milestone changed from Future Releases to 4.8.12

Branch: 2983_fish_filename_percent
changeset:4ceb42013925a4b264b5f3bd04682d402063084d

comment:8 Changed 4 years ago by andrew_b

  • Votes for changeset changed from ossi to ossi andrew_b
  • Branch state changed from on review to approved

comment:9 follow-up: ↓ 11 Changed 4 years ago by andrew_b

  • Status changed from accepted to testing
  • Votes for changeset changed from ossi andrew_b to committed-master
  • Resolution set to fixed
  • Branch state changed from approved to merged

comment:10 Changed 4 years ago by andrew_b

  • Status changed from testing to closed

comment:11 in reply to: ↑ 9 ; follow-ups: ↓ 13 ↓ 21 Changed 4 years ago by dnh

  • Priority changed from critical to trivial
  • Status changed from closed to reopened
  • Resolution fixed deleted
  • Cc dnh@… added

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.

Changed 4 years ago by dnh

vfs_fish_helpers_ls-percent.diff

comment:13 in reply to: ↑ 11 ; follow-up: ↓ 14 Changed 4 years ago by 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"?

comment:14 in reply to: ↑ 13 ; follow-up: ↓ 16 Changed 4 years ago by 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 ;)

comment:15 in reply to: ↑ 12 Changed 4 years ago by andrew_b

Replying to dnh:

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

I'm unable to reproduce that.

comment:16 in reply to: ↑ 14 ; follow-up: ↓ 17 Changed 4 years ago by 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.

Last edited 4 years ago by andrew_b (previous) (diff)

comment:17 in reply to: ↑ 16 Changed 4 years ago by 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.

comment:19 follow-up: ↓ 20 Changed 4 years ago by 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.

comment:20 in reply to: ↑ 19 Changed 4 years ago by 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.

comment:21 in reply to: ↑ 11 Changed 4 years ago by andrew_b

  • Status changed from reopened to closed
  • Resolution set to fixed

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().

Note: See TracTickets for help on using tickets.