Skip to content
Permalink
Browse files

Close code injection vulnerabilit in LIST and NLST.

This is quick piece of plywood over the hole.  It still allows
arbitrary ls switches (yuck), and breaks globbing.  LIST and NLST need
to be rewritten to not shell out to ls.

The vulnerability was discovered by Larry. W. Cashdollar.
  • Loading branch information
wconrad committed Mar 2, 2013
1 parent 21016c1 commit 828064f1a0ab69b2642c59cab8292a67bb44182c
@@ -1,4 +1,4 @@
### dev
### 0.2.2

Bug fixes

@@ -8,6 +8,9 @@ Bug fixes
PASS
* Open PASV mode data connection on same local IP as control connection.
This is required by RFC 1123.
* Disabled globbing in LIST (for now) due to code injection
vulnerability. This patch also disables globbing in NLST, but NLST
probably shouldn't do globbing.

Enhancements

@@ -103,6 +103,10 @@ _and_ for advertising to the client which IP to connect to. Binding
to 0.0.0.0 will work fine, but when the client tries to connect to
0.0.0.0, it won't get to the server.

LIST doesn't accept globs. It has other problems (it accepts
arbitrary ls arguments!) and needs to be rewritten to not shell out to
"ls".

## RUBY COMPATABILITY

The tests pass with these Rubies:
@@ -29,7 +29,7 @@ Commands supported:
CDUP Yes 0.1.0 Change to parent directory
CWD Yes 0.1.0 Change working directory
DELE Yes 0.1.0 Delete file
HELP Yes dev Help
HELP Yes 0.2.2 Help
LIST Yes 0.1.0 List directory
MKD Yes 0.2.1 Make directory
MODE Yes 0.1.0 Set transfer mode
@@ -52,7 +52,7 @@ Commands supported:
SMNT No --- Structure Mount
STAT No --- Server status
STOR Yes 0.1.0 Store file
STOU Yes dev Store with unique name
STOU Yes 0.2.2 Store with unique name
STRU Yes 0.1.0 Set file structure
Supports "File" structure only. "Record" and
"Page" are not supported
@@ -42,6 +42,7 @@ Feature: List
And the file list should contain "foo"

Scenario: Glob
Given PENDING "Disabled (for now) due to code injection vulnerability"
Given a successful login
And the server has file "foo"
And the server has file "bar"
@@ -41,15 +41,6 @@ Feature: Name List
Then the file list should be in short form
And the file list should contain "foo"

Scenario: Glob
Given a successful login
And the server has file "foo"
And the server has file "bar"
When the client successfully name-lists the directory "f*"
Then the file list should be in short form
And the file list should contain "foo"
And the file list should not contain "bar"

Scenario: Passive
Given a successful login
And the server has file "foo"
@@ -2,6 +2,7 @@
require 'memoizer'
require 'openssl'
require 'pathname'
require 'shellwords'
require 'socket'
require 'tmpdir'

@@ -206,6 +206,8 @@ class DiskFileSystem

module Ls

include Shellwords

def ls(ftp_path, option)
path = expand_ftp_path(ftp_path)
dirname = File.dirname(path)
@@ -214,11 +216,10 @@ def ls(ftp_path, option)
'ls',
option,
filename,
'2>&1',
].compact.join(' ')
].compact
if File.exists?(dirname)
list = Dir.chdir(dirname) do
`#{command}`
`#{shelljoin(command)} 2>&1`
end
else
list = ''

0 comments on commit 828064f

Please sign in to comment.
You can’t perform that action at this time.