Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
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 828064f
Show file tree
Hide file tree
Showing 7 changed files with 16 additions and 15 deletions.
5 changes: 4 additions & 1 deletion Changelog.md
@@ -1,4 +1,4 @@
### dev
### 0.2.2

Bug fixes

Expand All @@ -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

Expand Down
4 changes: 4 additions & 0 deletions README.md
Expand Up @@ -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:
Expand Down
4 changes: 2 additions & 2 deletions doc/rfc-compliance.md
Expand Up @@ -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
Expand All @@ -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
Expand Down
1 change: 1 addition & 0 deletions features/ftp_server/list.feature
Expand Up @@ -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"
Expand Down
9 changes: 0 additions & 9 deletions features/ftp_server/name_list.feature
Expand Up @@ -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"
Expand Down
1 change: 1 addition & 0 deletions lib/ftpd.rb
Expand Up @@ -2,6 +2,7 @@
require 'memoizer'
require 'openssl'
require 'pathname'
require 'shellwords'
require 'socket'
require 'tmpdir'

Expand Down
7 changes: 4 additions & 3 deletions lib/ftpd/disk_file_system.rb
Expand Up @@ -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)
Expand All @@ -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 = ''
Expand Down

0 comments on commit 828064f

Please sign in to comment.