Changed by slavazanko (@slavaz) on Mar 16, 2013 at 21:53 UTC (comment 1)

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

Changed by andrew_b (@aborodin) on Mar 17, 2013 at 5:02 UTC (comment 2)

• Component changed from mc-core to mc-vfs

Changed by andrew_b (@aborodin) on Jan 6, 2014 at 7:48 UTC (comment 3)

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

Changed by ossi (@ossilator) on Jan 6, 2014 at 8:55 UTC (comment 4)

• 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 by andrew_b (@aborodin) on Mar 14, 2014 at 4:46 UTC

• Attachment patch-ls-ub.diff (raw) added

Patch of Dieter Werner

Changed by andrew_b (@aborodin) on Mar 14, 2014 at 4:48 UTC (comment 5)

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

Changed by ossi (@ossilator) on Mar 14, 2014 at 8:05 UTC (comment 6)

• Votes 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 ...

Changed by andrew_b (@aborodin) on Mar 15, 2014 at 12:17 UTC (comment 7)

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

Branch: 2983_fish_filename_percent
[4ceb420]

Changed by andrew_b (@aborodin) on Mar 15, 2014 at 12:18 UTC (comment 8)

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

Changed by andrew_b (@aborodin) on Mar 15, 2014 at 12:19 UTC (comment 9)

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

Merged to master: [4ceb420]

Changed by andrew_b (@aborodin) on Mar 15, 2014 at 12:20 UTC (comment 10)

• Status changed from testing to closed

Changed by dnh (dnh@….org) on Mar 15, 2014 at 13:47 UTC (comment 9.11)

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

Replying to andrew_b:

Merged to master: [4ceb420]

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 by dnh (dnh@….org) on Mar 15, 2014 at 13:48 UTC

• Attachment vfs_fish_helpers_ls-percent.diff (raw) added

vfs_fish_helpers_ls-percent.diff

Changed by andrew_b (@aborodin) on Mar 15, 2014 at 14:26 UTC (comment 11.13)

Replying to dnh:

Replying to andrew_b:

Merged to master: [4ceb420]

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"?

Changed by dnh (dnh@….org) on Mar 15, 2014 at 15:34 UTC (comment 13.14)

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 ;)

Changed by andrew_b (@aborodin) on Mar 15, 2014 at 17:17 UTC (comment 12.15)

Replying to dnh:

BTW: what about <#3128> ?

I'm unable to reproduce that.

Changed by andrew_b (@aborodin) on Mar 15, 2014 at 17:21 UTC (comment 14.16)

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.

Changed by dnh (dnh@….org) on Mar 15, 2014 at 18:37 UTC (comment 16.17)

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.

Changed by ossi (@ossilator) on Mar 16, 2014 at 11:21 UTC (comment 19)

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.

Changed by andrew_b (@aborodin) on Mar 16, 2014 at 11:41 UTC (comment 19.20)

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.

Changed by andrew_b (@aborodin) on Mar 19, 2014 at 5:55 UTC (comment 11.21)

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